Modern-ify KeyEvent handling.

Modern-ify KeyEvent handling.

BUG=

Committed: https://code.google.com/p/dart/source/detail?r=20481

[Emily Fortuna, 2013-03-21 20:53:43]

This is not the design you suggested in the email, because I was trying to keep
the usage as similar to regular events as possible. But I'm open to changing it
if you prefer.

[Emily Fortuna, 2013-03-22 00:38:58]

I'm not convinced this is really better except I added more classes, but here it
is.

[Emily Fortuna, 2013-03-22 17:05:56]

Andrei, 
Can you take a look at this? Pete is out today and next week. 
Some background -- personally I prefer Patch 1. Pete wanted me to make the
KeyHandler private and add the static variables to KeyEvent, hence Patch 2, but
it doesn't really work in this case.

I think the best compromise is something between Patch 1 and possibly adding the
static variables to KeyEvent from Patch2, but not making the KeyEventHandler
private.

[Andrei Mouravski, 2013-03-22 19:12:33]

Here's what I think this should look like, and I think it should be a breaking
change following what mouseWheel does:

Note: I've made everything private, but if you really want to expose addKeyDown,
etc. then make these public. I think it's wrong to add events to the stream in
that way. Probably should be using a StreamController.

Also, anywhere I say "$foo" I mean to replace with one of [press, down, up].

class _KeyEventStreamProvider<T extends Event> extends
_CustomEventStreamProvider<T> {
  Stream<T> forTarget(EventTarget e, {bool useCapture: false}) => new
_KeyEventStream(e, _eventTypeGetter(e), useCapture);
}

class _KeyEventStream<T extends Event> extends _EventStream<T> {
...
}


Element:
  static const EventStreamProvider<KeyEvent> key$FooEvent = const
_CustomEventStreamProvider<KeyEvent>(() => '$foo');

  Stream<KeyEvent> get onKey$Foo => key$FooEvent.forTarget(this);



I got tired of trying to transcribe this, since that's basically your CL. Just
follow the logic all the way down and you should be at a good place.

https://codereview.chromium.org/12419011/diff/12001/tests/html/interactive_te...
File tests/html/interactive_test.dart (right):

https://codereview.chromium.org/12419011/diff/12001/tests/html/interactive_te...
tests/html/interactive_test.dart:122: document.body.innerHtml =
What are you actually testing here?

https://codereview.chromium.org/12419011/diff/12001/tests/html/keyboard_event...
File tests/html/keyboard_event_test.dart (right):

https://codereview.chromium.org/12419011/diff/12001/tests/html/keyboard_event...
tests/html/keyboard_event_test.dart:28:
KeyEvent.keyDownEvent.forTarget(document.body).listen(keydownHandlerTest);
Is this actually testing anything? Can't you do something?

https://codereview.chromium.org/12419011/diff/12001/tools/dom/src/dart2js_Key...
File tools/dom/src/dart2js_KeyEvent.dart (right):

https://codereview.chromium.org/12419011/diff/12001/tools/dom/src/dart2js_Key...
tools/dom/src/dart2js_KeyEvent.dart:56: // it the default keyboard event
handling, move these methods over to Element.
Create a bug tracking this and add it to the code.

[Emily Fortuna, 2013-03-22 22:53:58]

On 2013/03/22 19:12:33, Andrei Mouravski wrote:
> class _KeyEventStreamProvider<T extends Event> extends
> _CustomEventStreamProvider<T> {
>   Stream<T> forTarget(EventTarget e, {bool useCapture: false}) => new
> _KeyEventStream(e, _eventTypeGetter(e), useCapture);
> }
> 
> class _KeyEventStream<T extends Event> extends _EventStream<T> {
> ...
> }

I can certainly rename _KeyboardEventHandler to _KeyEventStreamProvider, but
other than that, that's what I already have. _KeyEventStream is exactly what
_controller.stream is. If that new class is private, and (the private)
StreamController already does all the functionality for producing a stream, why
add another class that is tightly coupled only to _KeyEventStreamProvider?

> Element:
>   static const EventStreamProvider<KeyEvent> key$FooEvent = const
> _CustomEventStreamProvider<KeyEvent>(() => '$foo');
> 
>   Stream<KeyEvent> get onKey$Foo => key$FooEvent.forTarget(this);

There are already onKeyPress, onKeyUp, onKeyDown methods on Element -- the built
in ones. We can't make that change unless we fully get rid of the lower level
version.

For the intents of making forward progress, let's first check this in in a
non-breaking-change form, and then we can discuss whether we want to make a full
breaking change in the future.

https://codereview.chromium.org/12419011/diff/12001/tests/html/interactive_te...
File tests/html/interactive_test.dart (right):

https://codereview.chromium.org/12419011/diff/12001/tests/html/interactive_te...
tests/html/interactive_test.dart:122: document.body.innerHtml =
On 2013/03/22 19:12:33, Andrei Mouravski wrote:
> What are you actually testing here?

It is a visual test -- (hence in the "interactive_test", which can only be run
manually). You can't programmatically construct keyboard events. So I could tell
the user "type a 't'" and test for the expected values in the key*HandlerTest or
just print them all out an let the user visually inspect them. I found letting
you type anything in this "test" to be more useful for debugging issues.

https://codereview.chromium.org/12419011/diff/12001/tests/html/keyboard_event...
File tests/html/keyboard_event_test.dart (right):

https://codereview.chromium.org/12419011/diff/12001/tests/html/keyboard_event...
tests/html/keyboard_event_test.dart:28:
KeyEvent.keyDownEvent.forTarget(document.body).listen(keydownHandlerTest);
On 2013/03/22 19:12:33, Andrei Mouravski wrote:
> Is this actually testing anything? Can't you do something?

KeyboardEvents cannot be programmatically constructed. See comments on lines
21-25. This is all I can do -- make sure it doesn't throw an exception on
creation. Hence the "interactive_test".

[blois, 2013-03-25 07:01:20]

https://codereview.chromium.org/12419011/diff/23001/sdk/lib/html/dartium/html...
File sdk/lib/html/dartium/html_dartium.dart (left):

https://codereview.chromium.org/12419011/diff/23001/sdk/lib/html/dartium/html...
sdk/lib/html/dartium/html_dartium.dart:33535: -class KeyboardEventController {
I think that KeyboardEventController and the EventStreamProvider should be
separate classes. And the provider just creates a controller when needed.

See comment below in forTarget.

https://codereview.chromium.org/12419011/diff/23001/sdk/lib/html/dartium/html...
File sdk/lib/html/dartium/html_dartium.dart (right):

https://codereview.chromium.org/12419011/diff/23001/sdk/lib/html/dartium/html...
sdk/lib/html/dartium/html_dartium.dart:33591: +  Stream<KeyEvent>
forTarget(EventTarget e, {bool useCapture: false}) {
What I imagined was that forTarget would do something like-

return new _KeyboardEventController.keydown(target).stream;

[Emily Fortuna, 2013-03-25 17:29:15]

https://codereview.chromium.org/12419011/diff/23001/sdk/lib/html/dartium/html...
File sdk/lib/html/dartium/html_dartium.dart (right):

https://codereview.chromium.org/12419011/diff/23001/sdk/lib/html/dartium/html...
sdk/lib/html/dartium/html_dartium.dart:33591: +  Stream<KeyEvent>
forTarget(EventTarget e, {bool useCapture: false}) {
On 2013/03/25 07:01:20, blois wrote:
> What I imagined was that forTarget would do something like-
> 
> return new _KeyboardEventController.keydown(target).stream;

How is this better from my existing KeyboardEventStream class which does the
same thing?

[Emily Fortuna, 2013-03-25 17:35:52]

https://codereview.chromium.org/12419011/diff/23001/sdk/lib/html/dartium/html...
File sdk/lib/html/dartium/html_dartium.dart (right):

https://codereview.chromium.org/12419011/diff/23001/sdk/lib/html/dartium/html...
sdk/lib/html/dartium/html_dartium.dart:33591: +  Stream<KeyEvent>
forTarget(EventTarget e, {bool useCapture: false}) {
On 2013/03/25 17:29:15, Emily Fortuna wrote:
> On 2013/03/25 07:01:20, blois wrote:
> > What I imagined was that forTarget would do something like-
> > 
> > return new _KeyboardEventController.keydown(target).stream;
> 
> How is this better from my existing KeyboardEventStream class which does the
> same thing?

Especially since this class is now private. I purposely created the
KeyboardEvenStream below to much more closely mimic the current way of
establishing event listeners to the streams like the rest of the html library.
Also note the KeyboardEventController class no longer exists.

[Emily Fortuna, 2013-03-25 17:37:52]

Andrei is out, too, today, I think? Adding a reviewer who is here.

[Andrei Mouravski, 2013-03-25 19:05:46]

lgtm

[Emily Fortuna, 2013-03-25 20:17:02]

Committed patchset #3 manually as r20481 (presubmit successful).

[blois, 2013-03-26 17:46:20]

https://codereview.chromium.org/12419011/diff/23001/sdk/lib/html/dartium/html...
File sdk/lib/html/dartium/html_dartium.dart (right):

https://codereview.chromium.org/12419011/diff/23001/sdk/lib/html/dartium/html...
sdk/lib/html/dartium/html_dartium.dart:33591: +  Stream<KeyEvent>
forTarget(EventTarget e, {bool useCapture: false}) {
On 2013/03/25 17:35:52, Emily Fortuna wrote:
> On 2013/03/25 17:29:15, Emily Fortuna wrote:
> > On 2013/03/25 07:01:20, blois wrote:
> > > What I imagined was that forTarget would do something like-
> > > 
> > > return new _KeyboardEventController.keydown(target).stream;
> > 
> > How is this better from my existing KeyboardEventStream class which does the
> > same thing?
> 
> Especially since this class is now private. I purposely created the
> KeyboardEvenStream below to much more closely mimic the current way of
> establishing event listeners to the streams like the rest of the html library.
> Also note the KeyboardEventController class no longer exists.

The primary thing is that there's the static _KeyboardEventHandler on KeyEvent,
and having these two merged causes conflicts in that shared instance.

Separating these would remove the per-element state from that static and ideally
we can add that to Element and make it even closer to the rest of the events.