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

[infra] Disallow usage of legacy key attribute #16222

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jugglinmike
Copy link
Contributor

The keyCode attribute of KeyboardEvent objects is not suitable for
conformance tests. Replace most existing references with the standard
key attribute. Persist those references whose intent is expressly to
test the legacy API. Introduce a new linting rule to disallow further
references.

These features were never formally specified and the current browser
implementations vary in significant ways. The large amount of legacy
content, including script libraries, that relies upon detecting the
user agent and acting accordingly means that any attempt to formalize
these legacy attributes and events would risk breaking as much content
as it would fix or enable. Additionally, these attributes are not
suitable for international usage, nor do they address accessibility
concerns.

Therefore, this specification does not normatively define the events
and attributes commonly employed for handling keyboard input, though
they MAY be present in user agents for compatibility with legacy
content. Authors SHOULD use the key attribute instead of the
charCode and keyCode attributes.

https://www.w3.org/TR/uievents/#legacy-key-attributes

The `keyCode` attribute of KeyboardEvent objects is not suitable for
conformance tests. Replace most existing references with the standard
`key` attribute. Persist those references whose intent is expressly to
test the legacy API. Introduce a new linting rule to disallow further
references.

> These features were never formally specified and the current browser
> implementations vary in significant ways. The large amount of legacy
> content, including script libraries, that relies upon detecting the
> user agent and acting accordingly means that any attempt to formalize
> these legacy attributes and events would risk breaking as much content
> as it would fix or enable. Additionally, these attributes are not
> suitable for international usage, nor do they address accessibility
> concerns.
>
> Therefore, this specification does not normatively define the events
> and attributes commonly employed for handling keyboard input, though
> they MAY be present in user agents for compatibility with legacy
> content. Authors SHOULD use the key attribute instead of the
> `charCode` and `keyCode` attributes.

https://www.w3.org/TR/uievents/#legacy-key-attributes
@foolip
Copy link
Member

foolip commented Apr 3, 2019

@garykac I see that https://w3c.github.io/uievents/#legacy-KeyboardEvent is marked as non-normative, which normally I would interpret as meaning these bits shouldn't be supported, tests shouldn't depend on it, and it'd be fine to add historical.html tests that fail if they are supported.

In this case that doesn't seem quite right to me, some of these are APIs that all browsers have and always will have. Sharing tests for those seems like a good thing, right?

@andreastt
Copy link
Member

I don’t believe we can make this change to the WebDriver specifically because they still need to run against IE. @jimevans will be able to confirm, but I suspect this will break IEDriver.

Besides this, I also have the same question as @foolip: if browsers implement this and have no intention of unshipping these for web compatibility reasons, isn’t it super-important for us to test this?

@andreastt andreastt removed their request for review April 3, 2019 11:30
@foolip
Copy link
Member

foolip commented Apr 3, 2019

Some issues with discussion around this are Specify MouseEvent.which and Require support or non-support for legacy init*Event.

@jimevans
Copy link
Contributor

jimevans commented Apr 3, 2019

I don’t think there would be negative impact to the IE driver from this change, but I would echo the concerns in previous comments about making sure web compatibility is maintained.

@jugglinmike
Copy link
Contributor Author

Web compatibility is indeed a concern. That's why I included the spec note in the commit message, and more importantly, why I persisted the property references in uievents/keyboard/key.js (see the modifications to WPT's linting safe list). For all other cases, though, the property is not directly related to the feature under test, so the reliance on a deprecated API doesn't seem warranted.

@andreastt @jimevans Concerning the WebDriver tests specifically: the references appear to be used only to display debugging information, so this change shouldn't affect results in any configuration. Removing them seemed preferable to adding another exception to the safe list.

@mjzffr mjzffr removed their request for review August 27, 2019 15:11
@gsnedders gsnedders removed their assignment Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants