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

Changed event.js to add "code" property to keyboard event object #3998

Merged
merged 1 commit into from Apr 17, 2018

Conversation

Projects
None yet
4 participants
@tmybr11
Copy link
Contributor

commented Mar 3, 2018

Summary

Added the "code" property to the Event object passed to keyboard-triggered event handlers, as suggested on #3978.

Checklist

@dmethvin
Copy link
Member

left a comment

LGTM. We're not testing all properties of jQuery.Event so I think the lack of a test is fine. As for docs, can you add a PR to update this line in the API docs?

@mgol

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

We're not testing all properties of jQuery.Event

Shouldn't we test them all? (this is not related to this PR, just a general question)

@mgol

mgol approved these changes Mar 4, 2018

@dmethvin

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

Not all properties that we lazy-copy are on all events that the browser can generate, and (at least last I checked) not all the properties are supported by all browsers. We'd need a matrix of event names and valid properties for the event by browser that could cover at least most of them.

To ensure we copy correctly from a native event we would need to dispatch a native event, and the mechanism to do that is browser-dependent as well right now--some support the newer event constructors and some only support the DOM2 document.createEvent() interface. I am not sure it's even possible to set all the properties we want to test through either.

@mgol

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

@dmethvin My main worry is that some future refactor that will move this code to a different place will lose one of the properties and we won't notice. To prevent that we don't really need to test with real event objects, just mocking it would suffice.

Obviously, that would amount to just duplicating the event name list in tests without testing the substance but it still seems a little bit more secure than what we have now.

@tmybr11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2018

I will update the API docs asap and I can also add a new test if you think it will be necessary for this update.

@timmywil

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

We could just do a super-dumb test and make sure that all of these properties are actually on the jQuery.Event prototype.

@mgol

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

@timmywil Yeah, something like that would be enough here.

@tmybr11 What we're talking about applies to all properties so your PR is fine, the test we're talking about is a matter for another PR/commit.

@mgol mgol added the Needs review label Mar 5, 2018

@dmethvin dmethvin removed the Needs review label Mar 8, 2018

@dmethvin dmethvin merged commit 899c56f into jquery:master Apr 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@GulajavaMinistudio GulajavaMinistudio referenced this pull request Apr 19, 2018

Merged

Update upstream #45

0 of 4 tasks complete

@lock lock bot locked as resolved and limited conversation to collaborators Oct 14, 2018

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