Sdk: [next] keyCode values for KeyboardEvents created via KeyEvent in dartium are incorrect

[efortuna]

Currently if you programmatically create a new KeyEvent, the underlying KeyboardEvent that gets created on Dartium has a keyCode and charCode of 0 istead of the desired value. We'll need to make the proper calls into v8/DOM to actually set the correct value.

[sethladd]

See this conversation: https://groups.google.com/a/dartlang.org/forum/#!topic/web/2TKgTbv3trE

[kevmoo]

_Removed Area-HTML label._
_Added Area-Library, Library-Html labels._

[efortuna]

Issue #15233 has been merged into this issue.

[kasperl]

_Removed this from the Later milestone._
_Added Oldschool-Milestone-Later label._

[kasperl]

_Removed Oldschool-Milestone-Later label._

[alan-knight]

Still true in 1.8. Works in JS. Might be addressable by next rev.

---

_Removed Priority-Unassigned label._
_Added Priority-Medium label._
_Changed the title to: "[next] keyCode values for KeyboardEvents created via KeyEvent in dartium are incorrect"._

[dgrove]

_Set owner to @alan-knight._
_Removed Priority-Medium label._
_Added Priority-High, C9 labels._

[dgrove]

_Removed C9 label._
_Added C1 label._

[dgrove]

Alan - any updates on this?

[alan-knight]

I'm not entirely sure we can actually do this. Here's what I think I know.

Both keyCode and charCode are deprecated. See https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.keyCode

In Chrome's KeyboardEvent there are functions for keyCode and charCode, but they're not values you can set, they're derived from other attributes of the event to mimic Firefox and/or IE behaviour. We don't surface those, but presumably we could.

In JS those attributes are visible, but you can't change them.

In Dart2js we add a layer of duct tape to superimpose something we can change. Specifically, we do
  Object.defineProperty(e, 'keyCode', { get : function() {return this.keyCodeVal;}})
  e.keyCodeVal= ourDesiredValue;

We could conceivably add those to Dart, either at the Dart object level or at the DartKeyboardEvent in C++ level, but I'm not sure that would actually be seen by the code that does dispatching, or if it would go back to the Chrome C++. I have the same reservations about the dartj2s approach. It will presumably work for things that go through JS, but be ignored by anything that looks at the internal structure.

My inclination is to call this WontFix and make sure those APIs are at least properly marked deprecated in the docs if not removing them.

[efortuna]

Alan is correct. When I reported the bug, for a fix I meant we'd have to go spelunking into the actual chrome codebase to add a hook that would make this work. I believe I did something similar with Leaf (and briefly Terry) for a similar but different problem. Might check in with them.

[efortuna]

(Leaf, Terry, please see my comment above. I'm a little fuzzy on the details, but I remember we added some connector to JavaScript property accessors or something a while back.)

---

cc @leafpetersen.
cc @terrylucas.

[kasperl]

_Added this to the 1.10 milestone._

[alan-knight]

_Removed this from the 1.10 milestone._
_Removed Priority-High label._
_Added Priority-Medium label._

[georgelesica-wf]

It would be really nice to finally get a solution to this issue, especially since the recommended method (at the top of https://api.dartlang.org/1.13.1/dart-html/KeyEvent-class.html) appears to be broken in 1.13 (see below). Is there another workaround being considered or is there some kind of ETA on this?

Uncaught Unhandled exception:
InvalidStateError: Failed to execute 'dispatchEvent' on 'EventTarget': The event is already being dispatched.
#0      Blink_JsNative_DomException.callMethod (dart:_blink:17262)
#1      BlinkEventTarget.dispatchEvent_Callback_1_ (dart:_blink:4422)
#2      EventTarget.dispatchEvent (dart:html:17300)
#3      _CustomKeyEventStreamImpl.add (dart:html:44939)
#4      _KeyboardEventHandler.processKeyDown (dart:html:46680)
#5      wrap_event_listener.<anonymous closure>.<anonymous closure> (dart:html:1215)

[terrylucas]

After we roll Dartium forward to build 45/46 we'll re-look at all open Dartium bugs.

[vsmenon]

Just hit this in pageloader, which used this API for testing.

Should KeyEvent be deprecated in favor of KeyboardEvent? That appears to be the new standard, but it's not quite there on all browsers though:

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/KeyboardEvent

[kevmoo]

http://caniuse.com/#search=KeyboardEvent

Worth looking at the specific bits that are and aren't supported.

[alan-knight]

The important thing is probably KeyboardEvent.key, which would be an actual portable and standard way of identifying the key. If we're lucky, you'll even be able to set it when constructing the event. But it's only just gone into the bleeding edge of Safari. We may be able to hack around it by delegating that to keyIdentifier on Safari in the meantime.

[eukreign]

I'm trying to unit test some keyboard handling in my app. Is there any work around for this at all? I'm willing to re-architect my app just so I can generate keyboard events artificially. I see a bunch of unit tests in Dart SDK but they are failing... :-1:

https://github.com/dart-lang/sdk/blob/master/tests/html/keyboard_event_test.dart

Is there something that does work?

[georgelesica-wf]

@eukreign we abstracted keyboard events into a provider that generates KeyEvents (which can be created manually and then we have two implementations, one listens for keyboard events from the browser, and the other has some helper methods that allow you to produce "fake" events.

abstract class EventProvider {
  EventProvider();

  /// Stream that will be subscribed by the managers and fed by the provider.
  Stream<KeyEvent> get stream;
}

Then in the implementation that actually reads from the keyboard we have this code:

  void _handleKeyboardEvent(KeyboardEvent event) {
    KeyEvent keyEvent = new KeyEvent.wrap(event);
    _controller.add(keyEvent);
  }

So we still can't unit test those two lines above, but we CAN unit test all of our actual event handling logic.

As an aside, the library that all of this is part of will be open sourced very soon, probably next week.

[eukreign]

@georgelesica-wf Thanks for the tips. What I ended up doing is abstracting even further:

class Input extends HtmlElement {
    StreamController<KeyEvent> controller = new StreamController<KeyEvent>(sync: true);
    Stream<KeyEvent> get onKeyEvent => controller.stream;
    Input.created(): super.created() {
        onKeyDown.listen((KeyboardEvent e) {
            controller.add(new KeyEvent.wrap(e));
        });
    }
}

When I want to listen for KeyEvents I can do it like so (added bonus is that handleKeyboard will get a KeyEvent object):

input.onKeyEvent.listen(handleKeyboard);

When I want to test:

input.controller.add(new KeyEvent('keydown', charCode: key));

So far this has worked pretty well.

Three caveats with my approach:

1. I use a sync: true Stream because in unit tests I want to be able to test the side affect of the key event immediately after adding the event. Without sync: true the event will be delivered after the unit tests finished (and fail).

2. Only one listener is allowed because I don't use a broadcast stream but this can be fixed with a bit more code.

3. I only listen for onKeyDown, but this too can be improved with more code.