fix: KeyboardEvent.keyCode is deprecated #681
Conversation
|
@barmac I can't add "needs review" label to PR |
|
That is fine. We have an automation for this :) |
|
Do I need to add linked issue to this repo for tag automation? |
|
It's OK, we don't need the label to review the PR. |
There was a problem hiding this comment.
While I believe the general direction of the PR, it does not replace keyCode with supported properties in the whole codebase. Notice that SearchPad is left intact: https://github.com/bpmn-io/diagram-js/blob/develop/lib/features/search-pad/SearchPad.js#L94
kerlon5
force-pushed
the
keyboardutils
branch
from
0fc86de to
b2031c8
Compare
|
I will look into this again this week. Sorry for delay. |
There was a problem hiding this comment.
While I understand that keyCode is deprecated, and we don't support IE anymore, we shouldn't make changes which break the features for the old browsers.
If we are to merge the changes, we should check both the key and the code properties. Note that there are also some IE specifics which we could account for: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key#examples
We could also keep support for keyCode but start using code in parallel to fix the problems with non-latin keyboards.
Also, ideally I'd like the features within the library to rely on isKey in keyboard util instead of having which, key etc. checks scattered around the codebase.
lib/features/dragging/Dragging.js
Outdated
| @@ -282,7 +282,7 @@ export default function Dragging(eventBus, canvas, selection, elementRegistry) { | |||
|
|
|||
| function checkCancel(event) { | |||
|
|
|||
| if (event.which === 27) { | |||
| if (event.code === 'Escape') { | |||
There was a problem hiding this comment.
This will break the feature for IE entirely: https://caniuse.com/keyboardevent-code
lib/features/search-pad/SearchPad.js
Outdated
| @@ -93,45 +93,38 @@ SearchPad.prototype._bindEvents = function() { | |||
| // navigate results | |||
| listen(this._container, SearchPad.INPUT_SELECTOR, 'keydown', function(e) { | |||
|
|
|||
| // up | |||
| if (e.keyCode === 38) { | |||
| if (e.code === 'ArrowUp') { | |||
There was a problem hiding this comment.
This will break the feature for IE entirely: https://caniuse.com/keyboardevent-code
This fixes the issue of not detecting key strokes on non-latin keyboards. BREAKING CHANGE: * Keyboard-related features no longer use KeyboardEvent#keyCode. Use a polyfill (e.g. [keyboardevent-key-polyfill](https://www.npmjs.com/package/keyboardevent-key-polyfill) if you need to support old browsers. Co-authored-by: Maciej Barelkowski <[email protected]>
barmac
force-pushed
the
keyboardutils
branch
from
b2031c8 to
ef05569
Compare
|
So there is actually a polyfill for KeyboardEvent#key which I linked :) |
use KeyboardEvent.code instead
it is used to fix camunda/camunda-modeler/2724
I'm not sure, do you need an old check by KeyboardEvent.keyCode? Then we can use triple check