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.

Sign up for GitHub

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

Jump to bottom

Change (deprecated) KeyboardEvent.keyCode -> KeyboardEvent.key #18530

Merged
merged 4 commits into from Oct 4, 2018
Merged

Change (deprecated) KeyboardEvent.keyCode -> KeyboardEvent.key #18530

merged 4 commits into from Oct 4, 2018

Conversation

nainar
Copy link
Contributor

@nainar nainar commented Oct 3, 2018

Fixes #17553

KeyboardEvent.keyCode is deprecated in favor KeyboardEvent.key.

KeyboardEvent.key has broad support and a better API.
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key

@nainar nainar requested a review from cvializ October 3, 2018 19:56
cvializ
cvializ reviewed Oct 3, 2018
View reviewed changes
Copy link
Contributor

@cvializ cvializ left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event.key is a string type like "ArrowUp", so I think you'll also need to update src/key-codes.js (or maybe make a src/keys.js : P)

@cvializ cvializ assigned 040medien and unassigned 040medien Oct 3, 2018
@nainar
Copy link
Contributor Author

nainar commented Oct 3, 2018

Done! Should I be concerned that no test failed :/

Swapped out KeyCodes to be strings instead of numbers. Used the console output here: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key to get the new values

@cvializ
Copy link
Contributor

cvializ commented Oct 4, 2018

Hmm I checked and it doesn't look concerning. I think the tests didn't fail because your changes updated the property name to be correct in the tests, and the non-test values and properties matched in the mock event objects. It looks like the spec agrees with your results https://www.w3.org/TR/uievents-key/#named-key-attribute-values

cvializ
cvializ reviewed Oct 4, 2018
View reviewed changes
extensions/amp-story/1.0/test/test-amp-story.js Outdated
eventObj.keyCode = KeyCodes.RIGHT_ARROW;
eventObj.which = KeyCodes.RIGHT_ARROW;
eventObj.key = Keys.RIGHT_ARROW;
eventObj.which = Keys.RIGHT_ARROW;
Copy link
Contributor

@cvializ cvializ Oct 4, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec defines which to be a specifically numerical value. https://www.w3.org/TR/uievents/#dom-keyboardevent-which The test code should either remove which if it is not needed, or if it is needed, the value should conform to the spec. I am not sure why this code mocks the which property, since I don't see it used in non-test code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out keyCode and which might differ on some browsers. https://unixpapa.com/js/key.html

I thought it safest to keep the code check to numeric here.
Fixed in the latest patch.

Copy link
Contributor

@cvializ cvializ Oct 4, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh that must be why it's deprecated. Thanks for the fix, there are also 3 more instances of which in the test code

cvializ
cvializ approved these changes Oct 4, 2018
View reviewed changes
Copy link
Contributor

@cvializ cvializ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after the last 3 which assignments are updated

@nainar
Copy link
Contributor Author

nainar commented Oct 4, 2018

Made the changes in the other spots. Thanks for the review!

@nainar nainar merged commit 36fe72c into ampproject:master Oct 4, 2018
eduardogoncalves pushed a commit to eduardogoncalves/amphtml that referenced this pull request Oct 5, 2018
@nainar @eduardogoncalves
Change (deprecated) KeyboardEvent.keyCode -> KeyboardEvent.key (amppr…
7d5f746 
…oject#18530)

* Change KeyboardEvent.keyCode -> KeyboardEvent.key

* Linter issues

* Change KeyCodes enum to be strings

* which fix
@nainar nainar mentioned this pull request Oct 5, 2018
Merged
nainar added a commit to nainar/amphtml that referenced this pull request Oct 5, 2018
@nainar
Revert "Change (deprecated) KeyboardEvent.keyCode -> KeyboardEvent.key (
45d0f02 
ampproject#18530)"

This reverts commit 36fe72c.
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Oct 10, 2018
@nainar @torch2424
Change (deprecated) KeyboardEvent.keyCode -> KeyboardEvent.key (amppr…
b5de19d 
…oject#18530)

* Change KeyboardEvent.keyCode -> KeyboardEvent.key

* Linter issues

* Change KeyCodes enum to be strings

* which fix
@aghassemi aghassemi mentioned this pull request Oct 23, 2018
Closed
Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
@nainar @Enriqe
Change (deprecated) KeyboardEvent.keyCode -> KeyboardEvent.key (amppr…
a9fbfc3 
…oject#18530)

* Change KeyboardEvent.keyCode -> KeyboardEvent.key

* Linter issues

* Change KeyCodes enum to be strings

* which fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cvializ cvializ cvializ approved these changes

Assignees
No one assigned
Labels
cla: yes PR use: In Beta / Experimental
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nainar @cvializ @040medien @googlebot @ampprojectbot