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

Deprecation of keypress events according to UI event spec https://w3c.github.io/uievents/#keypress #9626

Open
level420 opened this issue Feb 12, 2019 · 28 comments

Comments

Projects
None yet
6 participants
@level420
Copy link
Member

commented Feb 12, 2019

Since Firefox with version 65 deprecates keydown events for non printable characters, we should discuss how to deal with this in the framework:

There also may be changes in future versions of the other browsers which will hit us from time to time in the cases the are tightening more to the specs:

A good point reagarding what keydown is offering currently in Firefox:

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

See also issue #9623 which started this discussion

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

As @marcelr wrote on gitter, it seems that some methods like qx.event.type.KeySequence.getKeyIdentifier and qx.event.type.KeySequence.getKey do not retreive the correct key anymore if used with keydown events.

@oetiker

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

I think this whole thing also raises a conceptual question for us ... for many users of qooxdoo we are providing a 'sane' way to creating web apps with a stable API, shielding users largely from browser differences and other hard to navigate portions of the web-world.

Every now and then the 'real world' does bleed through though and people are faced with having to change their applications to cater to browser changes. I think we should try to shield them as much as possible as long as they are using qx objects.

If a change is necessary it should come with a major qooxdoo release as all breaking changes do.

@johnspackman

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@oetiker said: I wonder if we could put something in place to warn authors about this problem ...maybe have the compailer complain if it sees keypress eventhandlers being assigned ?

Possibly, but I think it would be much easier (and more reliable) to implement by overriding addListener in qx.ui.core.Widget.

@oetiker my thoughts exactly. AIUI keyboard events was one of the big normalisations that Qooxdoo did because in the good ol' bad ol' days browsers had markedly different details for keyboard events. Having to track keyup/down manually is a pain, and seems like a textbook case for encapsulation in the framework.

Has anyone seen a rationale for browsers deprecating keypress now? It seems pretty dumb, because keyup/keydown are really quite low level for most application use cases...

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

@johnspackman see this comment: w3c/uievents#96 (comment)

keypress was not deprecated because of a lack of implementations (all browsers generate keypress). It was deprecated because its use case was subsumed by beforeinput, which is a more general way of detecting when the user has produced input that can affect the DOM.

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

We are already firing artificial keypress events for some browsers before keydown. See https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/event/handler/Keyboard.js#L331-L368

But we also use there deprecated methods to read the key code via KeyboardEvent.keyCode at this line:
https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/event/handler/Keyboard.js#L339 which should be replaced by KeyboardEvent.key as mentioned here: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent#Properties
But .keyCode was a number whereas .key is a string. This needs more investigation.

And we use our own key code to identifier translation here:
https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/event/util/Keyboard.js#L142-L158
and ignore presumably for cross browser compatibility reason the also deprecated KeyboardEvent.keyIdentifier (which is good!) and use a .keyCode to identifier lookup map for this translation here:
https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/event/util/Keyboard.js#L80

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

Here is a fiddle showing the available event properties for keydown, keypress and keyup events:

https://jsfiddle.net/level420/ebncthvo/28/

As you can see even IE11 does not offer KeyboardEvent.keyIdentifier anymore. It may have been removed in IE10 or earlier.

@johnspackman

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

I see the logic in removing keypress now, in that keydown provides greater range of functionality, but I'm not convinced that keypress is obsolete or that keydown is a "better" route to go (ie it seems more work for many use cases).

  • keypress been around for some time, in Qooxdoo, and for backwards compatibility we have an obligation to support it, even if it means fabricating the event completely from keydown & keyup

  • personally I quite like the higher level logic of being able to just get an event that says "A" was pressed and then "a" was pressed and then "Ctrl-A", while not having to track Shift and Control keys

  • the disadvantage of "keypress" is that all that higher-level encapsulation of modifier keys goes away if it's not a printable key, eg Home or UpArrow both need tracking separately, but then I guess if you care about those then you would be doing something more low level anyway.

How about if we:

  • fabricate "keypress", so that it continues to provide "A", "a", "Ctrl-A", etc as a single easy event and which is backwards compatible

  • update "keydown" and "keyup" to track the modifier keys so that in the keydown event for "A", the code can detect whether Shift/Ctrl/etc is pressed (eg if (evt.hasKeyModifier("Ctrl")){...}); we would still fire events for modifier keys being pressed/released. It would also be useful if the event could say whether whether the key being pressed is a "non-printable".

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

@johnspackman I think we never had one single keypress event for Ctrl+A, thus combined key press events. This belongs to qx.ui.command.Command and the logic therein, where event listener exist for keydown and keypress which compose an artificial data event when finding the desired key sequence, here first Ctrl and then A.

So for our considerations we could in first place defer handling of combined key sequences.

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

But in general IMHO we should completely disable native keypress event detection and generate artificial keypress events from keydown events. We should have getters for the newer undeprecated KeyboardEvent properties key and code which will retrieve consistent key names among browsers.

We should deprecate getKeyIdentifier in qooxdoo keyboard events, implementing it as a wrapper around the KeyboardEvent.key property.

If we still want to support getKeyCode for backwards compatibility, we would have translate it back from KeyboardEvent.key (a string value) as there is no native event property anymore, which will deliver a key code (an integer value) and is NOT deprecated.

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

And we should throw a deprecation warning in addListener if a keypress event listener is registered.

@johnspackman

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@johnspackman I think we never had one single keypress event for Ctrl+A ... This belongs to qx.ui.command.Command

Oh :(

FWIW our own docs say in https://www.qooxdoo.org/5.0.1/pages/desktop/ui_interaction.html:

The keypress event is repeated during the time the key is pressed. That's why keypress is the best candidate for most action related keyboard events. Only use keyup and keydown when you really depend on the status of the key

Also there is the keyinput event in qx.ui.core.Widget, the docs say:

This event is fired if the pressed key or keys result in a printable character. Since the character is not necessarily associated with a single physical key press, the event does not have a key identifier getter. This event gets repeated if the user keeps pressing the key(s).

Reading the code in qx.event.handler.Keyboard, it seems that in Qooxdoo, keypress is for non-printable and keyinput is for printable

@derrell

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

There are likely a great many apps that depend on this functionality that has dated back in qooxdoo as far as I can remember. Although it's not unreasonable to require a small amount of porting of code to a new major qooxdoo release, we should make it as easy as possible.

Furthermore, one of the very nice things about qooxdoo is its extra events and event data that make writing code far easier. A different example is the "execute" event that qooxdoo provides, which is fired regardless of what type of operation actually caused, e.g., a button press; whether it be a click, a tap, etc. In the current case, providing as much information as possible to the user, when the event occurs, and not requiring the user to then make a number of additional calls to figure out what really happened, is something we should strive for. Telling the user that Ctrl-A was pushed, vs 'A' or 'a', is highly useful. I say this because it seems we need to make some changes, and we should make them with the most possible usefulness to users.

@cboulanger

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

So if I understand the discussion correctly, there seems to be an emerging consensus for continuing to fire the "keypress" event. Since it is a qooxdoo-event (which behaves somewhat differently anyways) and not a fake DOM Event, it seems to me that we don't even need a deprecation notice.

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

For the KeyboardEvent.key to KeyboardEvent.keyCode backwards compatibility translatione, we could have something like this: https://stackoverflow.com/a/51414061 where a the key is translated back to a key code.

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

As I've understood from reading the specs at https://w3c.github.io/uievents/#keypress the keypress event will disappear at some point in the future completely.

@cboulanger

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

So, given that there might be broken apps as long as we don't fix master and provide a backport - how should we proceed? I really don't have any preference, as long as we do it as backwards-compatible as possible...

@derrell

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

My feeling is that we should provide the prior functionality regardless of whether the browsers support it natively or not. It's a useful feature, in addition to simply needing (IMO) to keep this for backward compatability.

@cboulanger

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

I am really not qualified to work on this bug, but to move this forward: Is the following correct?

  • the place to fix this is here:
    _fireSequenceEvent : function(domEvent, type, keyIdentifier)
  • in there, we check if we are on Firefox, and, when receiving a native "keydown" event, dispatch a qx "keypress" event, artificially creating the necessary data so that things stay the way they have been.

Please correct me - maybe with some pseudo code so that we can arrive at an implementation ASAP.

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

@cboulanger the first step for a quick fix would be adapt qx.event.handler.Keyboard._emulateKeyPress and add those keycodes where we need to fire the artificial keypress event. And we have to distinguish there gecko version lower than 65 and 65 and above.

I've created a fiddle (https://jsfiddle.net/level420/ebncthvo/28/) which may help to find the keycode values for this.

In the longer term I think all browsers (besides IE11) will follow the new w3c UI event spec, stop firing more and more keypress events, in the end stopping to fire keypress events in general.

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

So the long term solution would be to completely omit listening for keypress events and allways fire an artificial keypress event. This way we will be independent from this step by step deprecations related to the keypress event.

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

I'd like to work on this a bit today and present a starting point for the quick fix as a PR.

@level420

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

Here we go with the first PR: 9629

For the non printable events: the logic in qx.event.handler.Keyboard already includes everything for this situation and fires already an artificial keypress event.

But a lot of logic in that code is based on the deprecated KeyboardEvent.keyCode and KeyboardEvent.charCode properties which may disappear in the future for all key events.
See:

What we need is to use instead the properties KeyboardEvent.key and KeyboardEvent.code instead and to offer the getters getKeyand getCodefor them in the corresponding qooxdoo events.

@cboulanger

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

Thank you @level420 for addressing the issue. A more robust and future-proof solution is necessary but it is good to have a quick fix that can also be added to a 5.0.3 release.

@ronkie

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Hi,

Since FF updated to 66, keypress event for some printable chars (namely from p to z) started to generate incorrect key identifier. For instance, "r" would have "F3" when calling event.getKeyIdentifier()

Looks like it is related to the change in FF where they wanted to unify keypress event behaviour:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/66
"From 66 onwards, when the KeyboardEvent.keyCode property of the keypress event object is 0, the value will be the same as KeyboardEvent.charCode. Conversely, when charCode is 0, it will be the same as keyCode. This mirroring behavior matches other browsers and is expected to solve most associated compatibility issues"

I guess the easy way to fix this would be to introduce a piece of code in Keyboard.js __onKeyPress for FF from 66 and later that would match one for Chrome, as it supposed to be the same now, though in the future .code and .key might have to be used indeed...

@ronkie

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

I was about to make a reproducible example for this in playground, but looks like it is reproduced by simply trying to type "v" there in the code. When typing "v", nothing happens (no "v" symbol is typed), and instead console is opened.

There are also errors in the console:
051553 playground.Application[18-0]: GlobalError: document.msElementsFromPoint is not a function
http://www.qooxdoo.org/current/playground/script/playground.js:210:98564,http://www.qooxdoo.org/current/playground/script/playground.js:210:97595,http://www.qooxdoo.org/current/playground/script/playground.js:210:95951

But I guess they are not related to this issue.

I used playground with this url:
http://demo.qooxdoo.org/current/playground/

Which is 5.0.2 I believe.

@ronkie

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

I've created a new issue for this problem:

#9643

Will provide a pull request later...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.