search

qooxdoo / qooxdoo

qooxdoo - Universal JavaScript Framework
http://qooxdoo.org
Other
764 stars 260 forks source link

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

Open level420 opened 5 years ago

level420 commented 5 years ago

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 commented 5 years ago

See also issue #9623 which started this discussion

level420 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

Some more reading about keypress vs keydown: https://www.mutuallyhuman.com/blog/2018/03/27/keydown-is-the-only-keyboard-event-we-need/

level420 commented 5 years ago

@johnspackman see this comment: https://github.com/w3c/uievents/issues/96#issuecomment-235394767

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

How about if we:

level420 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

johnspackman commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

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

level420 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

level420 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

I've created a new issue for this problem:

https://github.com/qooxdoo/qooxdoo/issues/9643

Will provide a pull request later...