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

React dom invalid aria hook #7744

Merged
merged 5 commits into from Sep 28, 2016

Conversation

Projects
None yet
6 participants
@jessebeach
Member

jessebeach commented Sep 15, 2016

Currently, React treats all aria-* props as custom props. There are 46 ARIA attributes; we can easily enumerate all of them and verify that an aria-* prop on an element exists in the spec.

https://www.w3.org/TR/wai-aria-1.1/#states_and_properties

I've included the list of props from the upcoming ARIA 1.1 Working Draft. I work with one of the editors of this specification. It's currently taken about 3 years to update from 1.0 to 1.1. We should not expect any churn in this list of props in the short- or mid-term future.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 15, 2016

Member

I think this might be a move in the wrong direction, as ultimately we'd like to remove the attribute whitelist all together. This is why we've started warning for unknown DOM properties, as eventually we'd like to render all provided attributes. See #140 and specifically #140 (comment)

Member

aweary commented Sep 15, 2016

I think this might be a move in the wrong direction, as ultimately we'd like to remove the attribute whitelist all together. This is why we've started warning for unknown DOM properties, as eventually we'd like to render all provided attributes. See #140 and specifically #140 (comment)

@zpao

This comment has been minimized.

Show comment
Hide comment
@zpao

zpao Sep 15, 2016

Member

@aweary We should at least do this for 15.x. Not sure when 16 will happen but we can make the situation better short-term until we actually solve the attribute whitelist problem. (This is one of the downsides of discussions happening outside GitHub - we talked with Jesse and think we should do this as accessibility is an important thing to focus on)


These were always an afterthought from the get-go and while they probably shouldn't have been, we can and should change that. This is a great way to do it.

but…

What if we made these more first-class citizens so they actually fit in with the other props? It would be hard for the white-list removal situation, but maybe better for React. These attributes don't fit in with the rest at all. They have a dash in them and they aren't camelcase. It's because we just passed anything through (based on the data-* behavior). We could potentially make these fit in more naturally with an aria={{role: 'button', roleDescription: 'whatever'}}. It's more work and more special casing but might be the best experience.

I guess we could do that separately (we talked about it for data-* a while ago too). What you have here just better enforces what is already supported, so we can probably just do it.

Member

zpao commented Sep 15, 2016

@aweary We should at least do this for 15.x. Not sure when 16 will happen but we can make the situation better short-term until we actually solve the attribute whitelist problem. (This is one of the downsides of discussions happening outside GitHub - we talked with Jesse and think we should do this as accessibility is an important thing to focus on)


These were always an afterthought from the get-go and while they probably shouldn't have been, we can and should change that. This is a great way to do it.

but…

What if we made these more first-class citizens so they actually fit in with the other props? It would be hard for the white-list removal situation, but maybe better for React. These attributes don't fit in with the rest at all. They have a dash in them and they aren't camelcase. It's because we just passed anything through (based on the data-* behavior). We could potentially make these fit in more naturally with an aria={{role: 'button', roleDescription: 'whatever'}}. It's more work and more special casing but might be the best experience.

I guess we could do that separately (we talked about it for data-* a while ago too). What you have here just better enforces what is already supported, so we can probably just do it.

@ghost ghost added the CLA Signed label Sep 15, 2016

@jessebeach

This comment has been minimized.

Show comment
Hide comment
@jessebeach

jessebeach Sep 15, 2016

Member

@aweary that's a great discussion, thank you for linking me to it. I understand the hesitation to add yet another whitelist. In this case, we have a finite and known set of attributes; one that is arguably an extension of the DOM spec (or should have been 😄).

@zpao You highlight an issue that leads to frustrating variations in the components I encounter: ariaDescribedBy, ariaDescribedBy, ariaHasPopup, ariaHaspopup. At the very least, I'd like to provide developers with a last line of defense against using invalid and non-existent aria-* attributes in their code. The ARIA spec is specialized; React can provide bumpers for it, just like it does for HTML in general.

Member

jessebeach commented Sep 15, 2016

@aweary that's a great discussion, thank you for linking me to it. I understand the hesitation to add yet another whitelist. In this case, we have a finite and known set of attributes; one that is arguably an extension of the DOM spec (or should have been 😄).

@zpao You highlight an issue that leads to frustrating variations in the components I encounter: ariaDescribedBy, ariaDescribedBy, ariaHasPopup, ariaHaspopup. At the very least, I'd like to provide developers with a last line of defense against using invalid and non-existent aria-* attributes in their code. The ARIA spec is specialized; React can provide bumpers for it, just like it does for HTML in general.

@ghost ghost added the CLA Signed label Sep 15, 2016

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 15, 2016

Member

Yes, I wasn't aware you guys discussed this already 😄 I agree, accessibility should always be a focus, so this looks good to me overall.

We could potentially make these fit in more naturally with an aria={{role: 'button', roleDescription: 'whatever'}}. It's more work and more special casing but might be the best experience.

I really like this idea for aria- attributes, it makes it clear that accessibility is something React considers important and makes it more consistent with the rest of the React API.

Member

aweary commented Sep 15, 2016

Yes, I wasn't aware you guys discussed this already 😄 I agree, accessibility should always be a focus, so this looks good to me overall.

We could potentially make these fit in more naturally with an aria={{role: 'button', roleDescription: 'whatever'}}. It's more work and more special casing but might be the best experience.

I really like this idea for aria- attributes, it makes it clear that accessibility is something React considers important and makes it more consistent with the rest of the React API.

Show outdated Hide outdated src/renderers/dom/shared/__tests__/ReactDOMInvalidARIAHook-test.js
'tag. For details, see https://fb.me/invalid-aria-prop'
);
});
});

This comment has been minimized.

@aweary

aweary Sep 15, 2016

Member

Can we also add a test verifying we get a warning when passing in a valid aria- attribute with invalid casing?

@aweary

aweary Sep 15, 2016

Member

Can we also add a test verifying we get a warning when passing in a valid aria- attribute with invalid casing?

This comment has been minimized.

@jessebeach

jessebeach Sep 15, 2016

Member

Certainly. Good call.

@jessebeach

jessebeach Sep 15, 2016

Member

Certainly. Good call.

@aweary

I think this is good to go pending the requested change

Show outdated Hide outdated src/renderers/dom/shared/__tests__/ReactDOMInvalidARIAHook-test.js
expect(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Invalid aria prop `aria-hasPopup` on <div> tag. ' +
'For details, see https://fb.me/invalid-aria-prop'
);

This comment has been minimized.

@aweary

aweary Sep 15, 2016

Member

Since we're using injectDOMPropertyConfig these aria- properties should be populated in DOMProperty.getPossibleStandardName. ReactDOMUnknownPropertyHook uses that to make a recommendation when warning for improperly cased props.

Can you do the same in ReactDOMInvalidARIAHook? That way if someone passes down aria-hasPopup it will ask if they meant aria-haspopup.

@aweary

aweary Sep 15, 2016

Member

Since we're using injectDOMPropertyConfig these aria- properties should be populated in DOMProperty.getPossibleStandardName. ReactDOMUnknownPropertyHook uses that to make a recommendation when warning for improperly cased props.

Can you do the same in ReactDOMInvalidARIAHook? That way if someone passes down aria-hasPopup it will ask if they meant aria-haspopup.

This comment has been minimized.

@jessebeach

jessebeach Sep 16, 2016

Member

Great suggestion. I added the changes to address this change request.

@jessebeach

jessebeach Sep 16, 2016

Member

Great suggestion. I added the changes to address this change request.

@aweary

aweary approved these changes Sep 26, 2016

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 26, 2016

Member

@jessebeach looks great, thank you!

Member

aweary commented Sep 26, 2016

@jessebeach looks great, thank you!

@aweary aweary added this to the 15-next milestone Sep 26, 2016

@ghost ghost added the CLA Signed label Sep 26, 2016

@aweary aweary merged commit 59ff774 into facebook:master Sep 28, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 87.904%
Details
@MichielBijl

This comment has been minimized.

Show comment
Hide comment
@MichielBijl

MichielBijl Sep 28, 2016

Thank you all for the good discussion and getting this into React 🎉

Thank you all for the good discussion and getting this into React 🎉

@zpao zpao modified the milestones: 15-next, 15.4.0 Oct 4, 2016

zpao added a commit that referenced this pull request Oct 4, 2016

React dom invalid aria hook (#7744)
* Add a hook that throws a runtime warning for invalid WAI ARIA attributes and values.

* Resolved linting errors.

* Added a test case for many props.

* Added a test case for ARIA attribute proper casing.

* Added a warning for uppercased attributes to ReactDOMInvalidARIAHook

(cherry picked from commit 59ff774)

acusti added a commit to brandcast/react that referenced this pull request Mar 15, 2017

React dom invalid aria hook (#7744)
* Add a hook that throws a runtime warning for invalid WAI ARIA attributes and values.

* Resolved linting errors.

* Added a test case for many props.

* Added a test case for ARIA attribute proper casing.

* Added a warning for uppercased attributes to ReactDOMInvalidARIAHook
@yangfan0095

This comment has been minimized.

Show comment
Hide comment

thank you @jessebeach

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