Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[next] keyCode values for KeyboardEvents created via KeyEvent in dartium are incorrect #13902

Closed
efortuna opened this issue Oct 8, 2013 · 22 comments
Assignees
Labels
area-library closed-obsolete Closed as the reported issue is no longer relevant library-html P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@efortuna
Copy link
Contributor

efortuna commented Oct 8, 2013

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
Copy link
Contributor

sethladd commented Dec 2, 2013

@kevmoo
Copy link
Member

kevmoo commented Apr 7, 2014

Removed Area-HTML label.
Added Area-Library, Library-Html labels.

@efortuna
Copy link
Contributor Author

Issue #15233 has been merged into this issue.

@kasperl
Copy link

kasperl commented Jul 10, 2014

Removed this from the Later milestone.
Added Oldschool-Milestone-Later label.

@kasperl
Copy link

kasperl commented Aug 4, 2014

Removed Oldschool-Milestone-Later label.

@alan-knight
Copy link
Contributor

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
Copy link
Contributor

dgrove commented Jan 26, 2015

Set owner to @alan-knight.
Removed Priority-Medium label.
Added Priority-High, C9 labels.

@dgrove
Copy link
Contributor

dgrove commented Jan 26, 2015

Removed C9 label.
Added C1 label.

@dgrove
Copy link
Contributor

dgrove commented Feb 4, 2015

Alan - any updates on this?

@alan-knight
Copy link
Contributor

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
Copy link
Contributor Author

efortuna commented Feb 5, 2015

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
Copy link
Contributor Author

efortuna commented Feb 5, 2015

(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
Copy link

kasperl commented Mar 20, 2015

Added this to the 1.10 milestone.

@alan-knight
Copy link
Contributor

Removed this from the 1.10 milestone.
Removed Priority-High label.
Added Priority-Medium label.

@georgelesica-wf
Copy link

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
Copy link
Contributor

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

@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed triaged labels Feb 29, 2016
@kevmoo kevmoo removed the Type-Defect label Mar 1, 2016
@vsmenon
Copy link
Member

vsmenon commented Nov 18, 2016

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
Copy link
Member

kevmoo commented Nov 18, 2016

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

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

@alan-knight
Copy link
Contributor

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
Copy link
Contributor

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... 👎

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

Is there something that does work?

@georgelesica-wf
Copy link

@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
Copy link
Contributor

eukreign commented Nov 21, 2016

@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.

@matanlurey matanlurey added the closed-obsolete Closed as the reported issue is no longer relevant label Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-library closed-obsolete Closed as the reported issue is no longer relevant library-html P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests