Make KeyboardEvent cross-browser consistent.

Make KeyboardEvent cross-browser consistent.

BUG=

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

[Emily Fortuna, 2012-11-28 22:58:12]

New and improved! It's a separate CL because I've switched over to git. :-)

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/dart2js/html_...
File sdk/lib/html/dart2js/html_dart2js.dart (left):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/dart2js/html_...
sdk/lib/html/dart2js/html_dart2js.dart:6445: * Represents an HTML <div> element.
these are "removed" because of Andrei's dartdoc change that doesn't work on the
mac yet. His CL is in progress here: https://codereview.chromium.org/11413197/
As soon as that change is in, these will reappear. Same for MenuElement.

https://codereview.chromium.org/11416249/diff/4001/tests/html/keyboard_event_...
File tests/html/keyboard_event_test.dart (right):

https://codereview.chromium.org/11416249/diff/4001/tests/html/keyboard_event_...
tests/html/keyboard_event_test.dart:11: useHtmlConfiguration();
again, not a real test. more for user testing than anything else. I'll remove
this from the CL before committing.

[blois, 2012-11-29 00:03:12]

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
File sdk/lib/html/src/KeyboardEventController.dart (right):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:97: void add(Function callback) {
void callback(KeyEvent)?

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:107: for(var callback in
_callbacks) {
Normally need to copy this list to allow removing while iterating.

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:114: void remove(Function
callback) {
void callback(KeyEvent)?

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:115: _callbacks =
_callbacks.filter((element) => element != callback);
Not removeAt? This creates a new list...

Should probably remove all of the event listeners when the # of callbacks goes
to 0 (and re-add when non-zero).

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/dart2js_K...
File sdk/lib/html/src/dart2js_KeyEvent.dart (right):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/dart2js_K...
sdk/lib/html/src/dart2js_KeyEvent.dart:56: void noSuchMethod(InvocationMirror
invocation) {
Are there performance implications of this? Might want to check with Stephen.

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/templates/htm...
File sdk/lib/html/templates/html/dart2js/impl_KeyboardEvent.darttemplate
(right):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/templates/htm...
sdk/lib/html/templates/html/dart2js/impl_KeyboardEvent.darttemplate:24: var
function = JS('dynamic', '#.initKeyboardEvent || #.initKeyEvent', this,
Under what cases is it initKeyboardEvent vs initKeyEvent?

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/templates/htm...
sdk/lib/html/templates/html/dart2js/impl_KeyboardEvent.darttemplate:32: int get
keyCode => $dom_keyCode;
Why hide then re-expose it?

[Emily Fortuna, 2012-11-29 01:10:36]

all comments addressed.
PTAL

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
File sdk/lib/html/src/KeyboardEventController.dart (right):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:97: void add(Function callback) {
On 2012/11/29 00:03:12, blois wrote:
> void callback(KeyEvent)?

Done.

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:107: for(var callback in
_callbacks) {
On 2012/11/29 00:03:12, blois wrote:
> Normally need to copy this list to allow removing while iterating.

No removing is happening here... just calling the callbacks...?

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:114: void remove(Function
callback) {
On 2012/11/29 00:03:12, blois wrote:
> void callback(KeyEvent)?

Done.

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:115: _callbacks =
_callbacks.filter((element) => element != callback);
On 2012/11/29 00:03:12, blois wrote:
> Not removeAt? This creates a new list...
> 
> Should probably remove all of the event listeners when the # of callbacks goes
> to 0 (and re-add when non-zero).

Done.

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/dart2js_K...
File sdk/lib/html/src/dart2js_KeyEvent.dart (right):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/dart2js_K...
sdk/lib/html/src/dart2js_KeyEvent.dart:56: void noSuchMethod(InvocationMirror
invocation) {
On 2012/11/29 00:03:12, blois wrote:
> Are there performance implications of this? Might want to check with Stephen.

Yep. Removed.

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/templates/htm...
File sdk/lib/html/templates/html/dart2js/impl_KeyboardEvent.darttemplate
(right):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/templates/htm...
sdk/lib/html/templates/html/dart2js/impl_KeyboardEvent.darttemplate:24: var
function = JS('dynamic', '#.initKeyboardEvent || #.initKeyEvent', this,
On 2012/11/29 00:03:12, blois wrote:
> Under what cases is it initKeyboardEvent vs initKeyEvent?

initKeyboardEvent == webkit browsers.
initKeyEvent == firefox

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/templates/htm...
sdk/lib/html/templates/html/dart2js/impl_KeyboardEvent.darttemplate:32: int get
keyCode => $dom_keyCode;
On 2012/11/29 00:03:12, blois wrote:
> Why hide then re-expose it?

At your request from way back when, I hid it on UIEvent, and so it is only
exposed in KeyboardEvent because keyCode and charCode don't make sense in the
other UIEvent contexts.

[blois, 2012-11-29 17:06:34]

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
File sdk/lib/html/src/KeyboardEventController.dart (right):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:107: for(var callback in
_callbacks) {
On 2012/11/29 01:10:37, Emily Fortuna wrote:
> On 2012/11/29 00:03:12, blois wrote:
> > Normally need to copy this list to allow removing while iterating.
> 
> No removing is happening here... just calling the callbacks...?

Right- if the code being called back removes itself while being called, you get
an error. It's a common thing for events like this to have to clone the list.

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:115: _callbacks =
_callbacks.filter((element) => element != callback);
On 2012/11/29 01:10:37, Emily Fortuna wrote:
> On 2012/11/29 00:03:12, blois wrote:
> > Not removeAt? This creates a new list...
> > 
> > Should probably remove all of the event listeners when the # of callbacks
goes
> > to 0 (and re-add when non-zero).
> 
> Done.

What I meant by remove all event listeners is that the processKey* listeners
should be removed when nothing else is listening here. So rather than hooking
them up in the constructor, hook them up in add when the first callback is
added, then unhook them in remove when the last one is removed.

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/templates/htm...
File sdk/lib/html/templates/html/dart2js/impl_KeyboardEvent.darttemplate
(right):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/templates/htm...
sdk/lib/html/templates/html/dart2js/impl_KeyboardEvent.darttemplate:24: var
function = JS('dynamic', '#.initKeyboardEvent || #.initKeyEvent', this,
On 2012/11/29 01:10:37, Emily Fortuna wrote:
> On 2012/11/29 00:03:12, blois wrote:
> > Under what cases is it initKeyboardEvent vs initKeyEvent?
> 
> initKeyboardEvent == webkit browsers.
> initKeyEvent == firefox

Can you add a brief comment for that?

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/templates/htm...
sdk/lib/html/templates/html/dart2js/impl_KeyboardEvent.darttemplate:32: int get
keyCode => $dom_keyCode;
On 2012/11/29 01:10:37, Emily Fortuna wrote:
> On 2012/11/29 00:03:12, blois wrote:
> > Why hide then re-expose it?
> 
> At your request from way back when, I hid it on UIEvent, and so it is only
> exposed in KeyboardEvent because keyCode and charCode don't make sense in the
> other UIEvent contexts.

Oh yeah :)

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/KeyboardE...
File sdk/lib/html/src/KeyboardEventController.dart (right):

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:113: /** Remove the given callback
from the listeners list. */
Can also use /// for single line comments.

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:116: for (i = 0; i <
_callbacks.length; i++) {
indexof?

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:280: int
normalizeKeyCodes(KeyboardEvent event) {
Does this need to be public?

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:297: /** Handle keydown events. */
private? (for all _process methods).

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/dart2js_K...
File sdk/lib/html/src/dart2js_KeyEvent.dart (right):

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/dart2js_K...
sdk/lib/html/src/dart2js_KeyEvent.dart:57: bool get cancelBubble =>
_parent.cancelBubble;
I believe that this should have a setter as well.

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/dart2js_K...
sdk/lib/html/src/dart2js_KeyEvent.dart:79: bool get returnValue =>
_parent.returnValue;
Should have a setter too.

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/dart2js_K...
sdk/lib/html/src/dart2js_KeyEvent.dart:92: void $dom_initUIEvent(String type,
bool canBubble, bool cancelable,
Probably just throw an exception from these $dom methods as they should not be
used on this.

https://codereview.chromium.org/11416249/diff/1015/tests/html/keyboard_event_...
File tests/html/keyboard_event_test.dart (right):

https://codereview.chromium.org/11416249/diff/1015/tests/html/keyboard_event_...
tests/html/keyboard_event_test.dart:14: print('keycode ${e.keyCode}');
Can you add a comment as to why this doesn't do much?

[Emily Fortuna, 2012-11-29 19:19:49]

Take 2. 
PTAL.

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
File sdk/lib/html/src/KeyboardEventController.dart (right):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:107: for(var callback in
_callbacks) {
On 2012/11/29 17:06:34, blois wrote:
> On 2012/11/29 01:10:37, Emily Fortuna wrote:
> > On 2012/11/29 00:03:12, blois wrote:
> > > Normally need to copy this list to allow removing while iterating.
> > 
> > No removing is happening here... just calling the callbacks...?
> 
> Right- if the code being called back removes itself while being called, you
get
> an error. It's a common thing for events like this to have to clone the list.

I see. Changed. But how could this happen since Dart is single threaded?

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:115: _callbacks =
_callbacks.filter((element) => element != callback);
On 2012/11/29 17:06:34, blois wrote:
> On 2012/11/29 01:10:37, Emily Fortuna wrote:
> > On 2012/11/29 00:03:12, blois wrote:
> > > Not removeAt? This creates a new list...
> > > 
> > > Should probably remove all of the event listeners when the # of callbacks
> goes
> > > to 0 (and re-add when non-zero).
> > 
> > Done.
> 
> What I meant by remove all event listeners is that the processKey* listeners
> should be removed when nothing else is listening here. So rather than hooking
> them up in the constructor, hook them up in add when the first callback is
> added, then unhook them in remove when the last one is removed.

sgtm.

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/templates/htm...
File sdk/lib/html/templates/html/dart2js/impl_KeyboardEvent.darttemplate
(right):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/templates/htm...
sdk/lib/html/templates/html/dart2js/impl_KeyboardEvent.darttemplate:24: var
function = JS('dynamic', '#.initKeyboardEvent || #.initKeyEvent', this,
On 2012/11/29 17:06:34, blois wrote:
> On 2012/11/29 01:10:37, Emily Fortuna wrote:
> > On 2012/11/29 00:03:12, blois wrote:
> > > Under what cases is it initKeyboardEvent vs initKeyEvent?
> > 
> > initKeyboardEvent == webkit browsers.
> > initKeyEvent == firefox
> 
> Can you add a brief comment for that?

Done.

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/KeyboardE...
File sdk/lib/html/src/KeyboardEventController.dart (right):

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:113: /** Remove the given callback
from the listeners list. */
On 2012/11/29 17:06:34, blois wrote:
> Can also use /// for single line comments.

Yeah... I thought it was better to have consistency throughout the file though
than mix comment styles.

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:116: for (i = 0; i <
_callbacks.length; i++) {
On 2012/11/29 17:06:34, blois wrote:
> indexof?

Done.

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:280: int
normalizeKeyCodes(KeyboardEvent event) {
On 2012/11/29 17:06:34, blois wrote:
> Does this need to be public?

Nope. fixed.

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:297: /** Handle keydown events. */
On 2012/11/29 17:06:34, blois wrote:
> private? (for all _process methods).

I was keeping them public in the event that people wanted to override these
methods. Ideally we'll fix them up as people find i18n inconsistencies, but this
also gives users an option to continue making forward progress while the fix is
still being made. Would you prefer I not do that?

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/dart2js_K...
File sdk/lib/html/src/dart2js_KeyEvent.dart (right):

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/dart2js_K...
sdk/lib/html/src/dart2js_KeyEvent.dart:57: bool get cancelBubble =>
_parent.cancelBubble;
On 2012/11/29 17:06:34, blois wrote:
> I believe that this should have a setter as well.

oops, good catch!

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/dart2js_K...
sdk/lib/html/src/dart2js_KeyEvent.dart:79: bool get returnValue =>
_parent.returnValue;
On 2012/11/29 17:06:34, blois wrote:
> Should have a setter too.

Done.

https://codereview.chromium.org/11416249/diff/1015/sdk/lib/html/src/dart2js_K...
sdk/lib/html/src/dart2js_KeyEvent.dart:92: void $dom_initUIEvent(String type,
bool canBubble, bool cancelable,
On 2012/11/29 17:06:34, blois wrote:
> Probably just throw an exception from these $dom methods as they should not be
> used on this.

Done.

https://codereview.chromium.org/11416249/diff/1015/tests/html/keyboard_event_...
File tests/html/keyboard_event_test.dart (right):

https://codereview.chromium.org/11416249/diff/1015/tests/html/keyboard_event_...
tests/html/keyboard_event_test.dart:14: print('keycode ${e.keyCode}');
On 2012/11/29 17:06:34, blois wrote:
> Can you add a comment as to why this doesn't do much?

Sure... I can just delete the test, because it really isn't even testing
anything automatically. Do you think I should keep it? It's essentially a no-op
without user interaction.

[blois, 2012-11-29 21:04:55]

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
File sdk/lib/html/src/KeyboardEventController.dart (right):

https://codereview.chromium.org/11416249/diff/4001/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:107: for(var callback in
_callbacks) {
On 2012/11/29 19:19:49, Emily Fortuna wrote:
> On 2012/11/29 17:06:34, blois wrote:
> > On 2012/11/29 01:10:37, Emily Fortuna wrote:
> > > On 2012/11/29 00:03:12, blois wrote:
> > > > Normally need to copy this list to allow removing while iterating.
> > > 
> > > No removing is happening here... just calling the callbacks...?
> > 
> > Right- if the code being called back removes itself while being called, you
> get
> > an error. It's a common thing for events like this to have to clone the
list.
> 
> I see. Changed. But how could this happen since Dart is single threaded?

Scenario: user presses the delete button, user code gets that event, removes the
UI element and during clean removes it's handler from this.

https://codereview.chromium.org/11416249/diff/1015/tests/html/keyboard_event_...
File tests/html/keyboard_event_test.dart (right):

https://codereview.chromium.org/11416249/diff/1015/tests/html/keyboard_event_...
tests/html/keyboard_event_test.dart:14: print('keycode ${e.keyCode}');
On 2012/11/29 19:19:50, Emily Fortuna wrote:
> On 2012/11/29 17:06:34, blois wrote:
> > Can you add a comment as to why this doesn't do much?
> 
> Sure... I can just delete the test, because it really isn't even testing
> anything automatically. Do you think I should keep it? It's essentially a
no-op
> without user interaction.
It may be handy to keep around for debugging since we don't have good tests. Is
there an alternate place for it?

https://codereview.chromium.org/11416249/diff/12003/sdk/lib/html/src/Keyboard...
File sdk/lib/html/src/KeyboardEventController.dart (right):

https://codereview.chromium.org/11416249/diff/12003/sdk/lib/html/src/Keyboard...
sdk/lib/html/src/KeyboardEventController.dart:135: _callbacks.removeAd(index);
removeAt?

https://codereview.chromium.org/11416249/diff/12003/sdk/lib/html/src/Keyboard...
sdk/lib/html/src/KeyboardEventController.dart:139:
target.on.keyDown.remove(processKeyDown);
_target?

https://codereview.chromium.org/11416249/diff/12003/sdk/lib/html/src/Keyboard...
sdk/lib/html/src/KeyboardEventController.dart:140:
target.on.keyPress.remove(processKeyPress);
Dart does not (currently?) have unique closure identity, meaning that
processKeyPress will be different here than where it was added.

You need to assign the method to an instance member, then add/remove that
instance member.

[Emily Fortuna, 2012-11-30 00:08:52]

ptal.

https://codereview.chromium.org/11416249/diff/12003/sdk/lib/html/src/Keyboard...
File sdk/lib/html/src/KeyboardEventController.dart (right):

https://codereview.chromium.org/11416249/diff/12003/sdk/lib/html/src/Keyboard...
sdk/lib/html/src/KeyboardEventController.dart:135: _callbacks.removeAd(index);
On 2012/11/29 21:04:55, blois wrote:
> removeAt?

Eeek you can tell I pushed this new code in right before my interview training.
sorry about that.

https://codereview.chromium.org/11416249/diff/12003/sdk/lib/html/src/Keyboard...
sdk/lib/html/src/KeyboardEventController.dart:139:
target.on.keyDown.remove(processKeyDown);
On 2012/11/29 21:04:55, blois wrote:
> _target?

Done.

https://codereview.chromium.org/11416249/diff/12003/sdk/lib/html/src/Keyboard...
sdk/lib/html/src/KeyboardEventController.dart:140:
target.on.keyPress.remove(processKeyPress);
On 2012/11/29 21:04:55, blois wrote:
> Dart does not (currently?) have unique closure identity, meaning that
> processKeyPress will be different here than where it was added.
> 
> You need to assign the method to an instance member, then add/remove that
> instance member.

Done.

[blois, 2012-11-30 00:21:02]

https://codereview.chromium.org/11416249/diff/7006/sdk/lib/html/src/KeyboardE...
File sdk/lib/html/src/KeyboardEventController.dart (right):

https://codereview.chromium.org/11416249/diff/7006/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:98:
_initializeAllEventListeners();
No need to do this in constructor, since it occurs in add.

https://codereview.chromium.org/11416249/diff/7006/sdk/lib/html/src/KeyboardE...
sdk/lib/html/src/KeyboardEventController.dart:107: _keyDown = processKeyDown;
Would be safer to assign these in the constructor, because if you call this
twice then you'll lose your old events.

[Emily Fortuna, 2012-11-30 00:31:44]

try this again

[blois, 2012-11-30 00:43:12]

lgtm