Skip to content

Updating docs copy for accessibility false positives #4856

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 10 commits into from
Aug 7, 2023

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Aug 2, 2023

Closes

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

RSP

@rspbot
Copy link

rspbot commented Aug 2, 2023

@LFDanLu LFDanLu marked this pull request as ready for review August 2, 2023 20:52

### False positives

There are a number of known accessibility false positives in React Aria and React Spectrum, currently documented here in our [wiki](https://github.com/adobe/react-spectrum/wiki/Known-accessibility-false-positives).
Copy link
Member Author

Choose a reason for hiding this comment

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

IMO we should continue to maintain this wiki as our source of truth for these accessibility data attributes rather than maintain a table of them in this page here, open to opinions though

@rspbot
Copy link

rspbot commented Aug 2, 2023


### False positives

Picker may trigger a [known accessibility false positive](https://github.com/adobe/react-spectrum/wiki/Known-accessibility-false-positives#picker)
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a short explanation of what the issue is and why it's a false positive here?

There are a number of known accessibility false positives in React Aria and React Spectrum, currently documented here in our [wiki](https://github.com/adobe/react-spectrum/wiki/Known-accessibility-false-positives).
These are commonly caught by automated accessibility testing tools and can cause unnecessary noise in your own accessibility audits.
To facilitate the suppression of these false positives, the data attribute `data-a11y-ignore` is included on the problematic elements
with the relevant `AXE` rule set as its value. See [here](https://github.com/adobe/react-spectrum/blob/9a9327fae5b5a466f7a1394a82669ea19858356d/.storybook/preview.js#L17-L26)
Copy link
Member

Choose a reason for hiding this comment

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

I kinda think we should show a code example inline here rather than linking. We might also want to show how to do it for pa11y as well, as that was what was requested in the original issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a code example for the Storybook test runner config. As for pa11y, strangely enough it doesn't actually catch the HiddenSelect false positive when I run the cli using npx pa11y --runner axe 'https://reactspectrum.blob.core.windows.net/reactspectrum/ca8f4230c5efb9f248f0a028257ac60093706f78/storybook/iframe.html?providerSwitcher-express=false&providerSwitcher-toastPosition=bottom&providerSwitcher-locale=&providerSwitcher-theme=&providerSwitcher-scale=&args=&id=picker--default&viewMode=story'. Not sure what tool the console error the original issue comes from.

I can add a blurb still with a tentative way to suppress the error in pa11y but I'm a bit hesitant to do so without being able to reproduce the error in pa11y in the first place

@rspbot
Copy link

rspbot commented Aug 3, 2023

@rspbot
Copy link

rspbot commented Aug 3, 2023

reidbarber
reidbarber previously approved these changes Aug 4, 2023
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM

yihuiliao
yihuiliao previously approved these changes Aug 4, 2023
@rspbot
Copy link

rspbot commented Aug 4, 2023

@rspbot
Copy link

rspbot commented Aug 4, 2023

@LFDanLu LFDanLu dismissed stale reviews from yihuiliao and reidbarber via cea18ed August 4, 2023 22:46
@rspbot
Copy link

rspbot commented Aug 4, 2023

with the relevant `AXE` rule set as its value. Below is a list of the currently available data selectors and their equivalent `AXE` rules:

```tsx
rules: [
Copy link
Member

Choose a reason for hiding this comment

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

might need to wrap this in another {} object. the syntax coloring might be confused.

@rspbot
Copy link

rspbot commented Aug 4, 2023

@rspbot
Copy link

rspbot commented Aug 7, 2023

@rspbot
Copy link

rspbot commented Aug 7, 2023

@rspbot
Copy link

rspbot commented Aug 7, 2023

## API Changes

unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }

@LFDanLu LFDanLu merged commit a5c6977 into main Aug 7, 2023
@LFDanLu LFDanLu deleted the followup_hidden_select branch August 7, 2023 16:55
@dannify dannify removed the release label Aug 13, 2024
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.

6 participants