Skip to content

Update to ARIA 1.2; fix test failures #696

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

Merged
merged 7 commits into from
Jun 18, 2020
Merged

Conversation

jessebeach
Copy link
Collaborator

@jessebeach jessebeach commented Jun 12, 2020

See aria-query's update to ARIA 1.2: A11yance/aria-query#46

The changes

  1. scrollbar becomes an interactive element
  2. slider and spinbutton lose some required props
  3. aria-selected , aria-expanded, aria-invalid and aria-haspopup are no longer allowed on several roles.
  4. Fixes jsx-a11y/aria-props flags aria-details as invalid #644

@@ -192,7 +192,6 @@ const alwaysValid = [
{ code: '<div role="presentation" />' },
{ code: '<div role="region" />' },
{ code: '<div role="rowgroup" />' },
{ code: '<div role="scrollbar" />' },
Copy link
Member

Choose a reason for hiding this comment

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

is this a breaking change? or would you consider it a bugfix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ljharb Breaking change in behavior, but not API. This will be a Minor release. Some new violations will probably pop up for folks when they run their linter. Does this sound right?

Copy link
Member

Choose a reason for hiding this comment

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

semver for eslint rules is tricky; “more errors” is definitely at least a minor, but the real question is whether they have a reasonable expectation that the code shouldn’t be warned on. If this code getting a new warning will help them catch a bug instead of surprising them, then that seems nonmajor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If this code getting a new warning will help them catch a bug instead of surprising them, then that seems nonmajor?

Thinking on this more, Authors will most likely be removing overrides put in place because scrollbar wasn't considered an interactive element before now, so putting a click handler on one would have raised a lint warning. That'll be good to note in the release notes.

@jessebeach jessebeach force-pushed the fix-aria-1-2-test-failures branch from 1fa93d8 to f87561d Compare June 17, 2020 20:57
@jessebeach jessebeach changed the title Fix ARIA 1.2 test failures Update to ARIA 1.2; fix test failures Jun 17, 2020
@jessebeach
Copy link
Collaborator Author

jessebeach commented Jun 17, 2020

In testing the ARIA 1.2 upgrade in aria-query, I found a bug that aria-level wasn't included as an allowed (and required) prop on the heading role. (A11yance/aria-query@f1b8f11)

I found one instance of The attribute aria-haspopup is not supported by the role link. This role is implicit on the element a, which is neat.

I found one instance of Elements with the ARIA role "option" must have the following attributes defined: aria-selected.

And, The attribute aria-disabled is not supported by the role listitem. This role is implicit on the element li

And, The attribute aria-haspopup is not supported by the role tooltip

And, Elements with the ARIA role "checkbox" must have the following attributes defined: aria-checked

Errors/Warnings were virtually the same over 120,000 files. I'm confident in this upgrade.

@ljharb
Copy link
Member

ljharb commented Jun 17, 2020

woot, ship it (once tests pass)

@jessebeach jessebeach merged commit e215b87 into master Jun 18, 2020
@jessebeach jessebeach deleted the fix-aria-1-2-test-failures branch June 18, 2020 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jsx-a11y/aria-props flags aria-details as invalid
2 participants