#3578 Add the ability to add a data attribute to <HiddenSelect /> to handle false a11y positives #4759

aloysb · 2023-07-12T13:07:44Z

This PR simply add the ability to expose a data-attribute on the <HiddenElement /> of useSelect.

This is useful when using a tool such as AXE or pa11y giving you false positive - the data attribute is a good way to force the tool to ignore the false positive.

✅ 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:

Use the following code (from the useSelect documentation example), ensuring that the props contains {hasA11yIgnoreDataAttr: true}
Ensure that the hidden select element has the data attribute data-rsp-a11y-ignore.
Ensure unit tests are passing (yarn jest)

import {HiddenSelect, useSelect} from '@react-aria/select';
import {Item} from '@react-stately/collections';
import {useSelectState} from '@react-stately/select';

// Reuse the ListBox, Popover, and Button from your component library. See below for details.
import {ListBox, Popover, Button} from 'your-component-library';

function Select(props) {
  // Create state based on the incoming props
  let state = useSelectState(props);

  // Get props for child elements from useSelect
  let ref = React.useRef(null);
  let {
    labelProps,
    triggerProps,
    valueProps,
    menuProps
  } = useSelect(props, state, ref);

  return (
    <div style={{display: 'inline-block'}}>
      <div {...labelProps}>{props.label}</div>
      <HiddenSelect
        isDisabled={props.isDisabled}
        state={state}
        triggerRef={ref}
        label={props.label}
        name={props.name} />
      <Button
        {...triggerProps}
        buttonRef={ref}
        style={{height: 30, fontSize: 14}}>
        <span {...valueProps}>
          {state.selectedItem
            ? state.selectedItem.rendered
            : 'Select an option'
          }
        </span>
        <span
          aria-hidden="true"
          style={{paddingLeft: 5}}>
          ▼
        </span>
      </Button>
      {state.isOpen &&
        <Popover state={state} triggerRef={ref} placement="bottom start">
          <ListBox
            {...menuProps}
            state={state} />
        </Popover>
      }
    </div>
  );
}

<Select label="Favorite Color">
  <Item>Red</Item>
  <Item>Orange</Item>
  <Item>Yellow</Item>
  <Item>Green</Item>
  <Item>Blue</Item>
  <Item>Purple</Item>
  <Item>Black</Item>
  <Item>White</Item>
  <Item>Lime</Item>
  <Item>Fushsia</Item>
</Select>

🧢 Your Project:

N/A

This allows to use the data attribute as a reference for other tools to ignore a11y issues.

This solves adobe#3578 Some tools such as AXE or pa11y might give false positive on hidden select. This add a new prop to expose a data-attribute (dataset) that can be used to ignore these false positives.

reidbarber

Thanks for the contribution!!

There were a few lint issues that you can see when you run yarn lint. Fixing those should fix the build.

packages/@react-aria/select/src/HiddenSelect.tsx

packages/@react-aria/select/test/HiddenSelect.test.tsx

packages/@react-aria/select/src/HiddenSelect.tsx

reidbarber

Also, I think we can just always add the new data attribute, no prop needed to enable.

There is no harm in making this available, as discussed in the PR review.

aloysb · 2023-07-14T07:28:10Z

@reidbarber I've made the data-attribute always available; there is no harm in doing so.
I agree with you, there is no need to hide it behind a flag.

I've also merged in the latest master, but maybe you prefer it rebased?
Let me know.

Linting issues should be also resolved. (We'll see once the CI has finished!)

This fixes an issue in the docs where the component was being parsed when it shouldn't have been parsed.

reidbarber · 2023-07-14T16:18:49Z

GET_BUILD

rspbot · 2023-07-14T16:33:21Z

Build successful! 🎉

reidbarber

Looks great! Just a small tweak to the docs to reflect the updates and I'm good to approve.

packages/@react-aria/select/docs/useSelect.mdx

packages/@react-aria/select/src/HiddenSelect.tsx

LFDanLu · 2023-07-14T22:36:46Z

packages/@react-aria/select/src/HiddenSelect.tsx

@@ -87,7 +87,8 @@ export function useHiddenSelect<T>(props: AriaHiddenSelectProps, state: SelectSt
      name,
      size: state.collection.size,
      value: state.selectedKey ?? '',
-      onChange: (e: React.ChangeEvent<HTMLSelectElement>) => state.setSelectedKey(e.target.value)
+      onChange: (e: React.ChangeEvent<HTMLSelectElement>) => state.setSelectedKey(e.target.value),
+      ['data-rsp-a11y-ignore']: true


Think we can get rid of this one too. I figure people can blanket filter out the container div for aXe and pa11y via data-rsp-a11y-ignore (its the one triggering the "ARIA hidden element must not be focusable or contain focusable elements" false positive) and thus we don't need the attribute on the select props. We could even move this line onto the containerProps so people using the hook instead of HiddenSelect benefit too.

Did you find that you needed the data attribute on the select element?

packages/@react-aria/select/src/HiddenSelect.tsx

packages/@react-aria/select/test/HiddenSelect.test.tsx

LFDanLu

Thanks for the contribution! I went in and updated it so the data attribute is just on the container props and verified that we could actually use the attribute to suppress the false positive in our own storybook, hope you don't mind. Apologies about highjacking the PR, we wanted to try and get it merged for our testing session on Monday.

Lemme know if you had any thoughts about my comment here, happy to update in a followup if it turns out we need the attribute on more that just the container

LFDanLu · 2023-07-17T17:09:10Z

GET_BUILD

LFDanLu · 2023-07-17T17:11:39Z

packages/@react-aria/select/docs/useSelect.mdx

+### False positive on `<HiddenSelect />` in accessibility tools
+
+When using &lt;<TypeLink links={docs.links} type={docs.exports.HiddenSelect} />&gt; some accessibility tools such as AXE or pa11y might give a false positive.
+The data attribute `data-rsp-a11y-ignore` is included on the hidden select element and can be used to ignore the false positives in these tools.


For the team, open to opinions on where this information should go. Perhaps we should add a new section in this docs page for accessibility gotchas (or simply accessiblity?)

I'm fine with where it is, but not against a dedicated accessibility section either.

+1 to Reid's comment. I think this is fine where it is but a new section for accessibility gotchas would be nice

Alright, I'll approve this PR and make a followup one moving it to a separate accessibility section.

It definitely shouldn't be under "Internationalization" IMO, seems unrelated to that. Also we will need to cover this on all of the other select pages: RAC Select and RSP Picker. If it is going to be a general way for us to indicate false positives, we may also want to document it on a more general page such as the "Accessibility" one.

good point about covering this on the other Select pages. By the "Accessibility" page I assume you are referencing this? If so, I think that is a good idea.

@abitbetterthanyesterday IMO this is creeping in scope a bit, would you like to take this on? I'd be perfectly happy making followup PR for this as well instead.

rspbot · 2023-07-17T17:20:59Z

Build successful! 🎉

LFDanLu

LGTM, thanks once again for the contribution!

devongovett · 2023-07-19T22:37:24Z

packages/@react-aria/select/src/HiddenSelect.tsx

@@ -71,7 +71,8 @@ export function useHiddenSelect<T>(props: AriaHiddenSelectProps, state: SelectSt
  return {
    containerProps: {
      ...visuallyHiddenProps,
-      'aria-hidden': true
+      'aria-hidden': true,
+      ['data-rsp-a11y-ignore']: true


This probably should not have rsp in the name because it's not spectrum specific, this will also affect React Aria. Maybe just data-a11y-ignore?

devongovett · 2023-07-19T22:40:44Z

packages/@react-aria/select/docs/useSelect.mdx

+### False positive on `<HiddenSelect />` in accessibility tools
+
+When using &lt;<TypeLink links={docs.links} type={docs.exports.HiddenSelect} />&gt; some accessibility tools such as AXE or pa11y might give a false positive.
+The data attribute `data-rsp-a11y-ignore` is included on the hidden select element and can be used to ignore the false positives in these tools.


It definitely shouldn't be under "Internationalization" IMO, seems unrelated to that. Also we will need to cover this on all of the other select pages: RAC Select and RSP Picker. If it is going to be a general way for us to indicate false positives, we may also want to document it on a more general page such as the "Accessibility" one.

aloysb · 2023-07-20T02:05:42Z

Hm, feel free to take it on. I have a bit on my plate right now so you might be more available. Thanks for all your help.

…

On Thu, Jul 20, 2023, 9:46 AM Daniel Lu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ***@***.***/select/docs/useSelect.mdx <#4759 (comment)> : > +### False positive on `<HiddenSelect />` in accessibility tools + +When using <<TypeLink links={docs.links} type={docs.exports.HiddenSelect} />> some accessibility tools such as AXE or pa11y might give a false positive. +The data attribute `data-rsp-a11y-ignore` is included on the hidden select element and can be used to ignore the false positives in these tools. good point about covering this on the other Select pages. By the "Accessibility" page I assume you are referencing this <https://react-spectrum.adobe.com/react-aria/accessibility.html#accessibility>? If so, I think that is a good idea. @abitbetterthanyesterday <https://github.com/abitbetterthanyesterday> IMO this is creeping in scope a bit, would you like to take this on? I'd be perfectly happy making followup PR for this as well instead. — Reply to this email directly, view it on GitHub <#4759 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHYZ4XIXBDCMG3WUFCX443XRBWU7ANCNFSM6AAAAAA2HO4A7M> . You are receiving this because you were mentioned.Message ID: ***@***.***>

aloysb · 2023-07-26T22:33:00Z

Is there anything left I can do to get this in?
Sorry it has grown out of scope a bit so fell off the wagon 🙂

LFDanLu · 2023-07-26T22:53:29Z

I'm hoping to get this reviewed by the team and merged in as is by early next week, then will open a followup PR shortly after addressing the rest of the comments.

reidbarber

LGTM

ktabors · 2023-08-01T01:28:01Z

packages/@react-aria/select/docs/useSelect.mdx

+### False positive on `<HiddenSelect />` in accessibility tools
+
+When using &lt;<TypeLink links={docs.links} type={docs.exports.HiddenSelect} />&gt; some accessibility tools such as AXE or pa11y might give a false positive.
+The data attribute `data-rsp-a11y-ignore` is included on the hidden select element and can be used to ignore the false positives in these tools.


Suggested change

The data attribute `data-rsp-a11y-ignore` is included on the hidden select element and can be used to ignore the false positives in these tools.

The data attribute `data-a11y-ignore` is included on the hidden select element and can be used to ignore the false positives in these tools.

It got renamed right?

I'll update in a followup

ktabors · 2023-08-01T01:36:07Z

GET_BUILD

rspbot · 2023-08-01T01:48:35Z

Build successful! 🎉

rspbot · 2023-08-01T01:48:37Z

## 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' }

devongovett

thanks for the contribution!

addressed

Aloys Berger added 2 commits July 11, 2023 05:48

Add option to add data attribute to hidden select

cd888f1

This allows to use the data attribute as a reference for other tools to ignore a11y issues.

Add the ability to add a data-attr to handle a11y false positive

5ea07de

This solves adobe#3578 Some tools such as AXE or pa11y might give false positive on hidden select. This add a new prop to expose a data-attribute (dataset) that can be used to ignore these false positives.

aloysb closed this Jul 12, 2023

aloysb reopened this Jul 12, 2023

Aloys Berger and others added 2 commits July 12, 2023 23:11

Improve grammar, fix typos

b8b7680

Merge branch 'main' into 3578-add-data-attr-to-hidden-select

4f00197

reidbarber requested changes Jul 12, 2023

View reviewed changes

reidbarber reviewed Jul 12, 2023

View reviewed changes

aloysb and others added 2 commits July 14, 2023 17:07

Merge branch 'adobe:main' into 3578-add-data-attr-to-hidden-select

48c8826

Make the aria-rs-a11y-ignore attribute always available

9c5035a

There is no harm in making this available, as discussed in the PR review.

Fix docs building issue

c75fe76

This fixes an issue in the docs where the component was being parsed when it shouldn't have been parsed.

reidbarber requested changes Jul 14, 2023

View reviewed changes

packages/@react-aria/select/docs/useSelect.mdx Outdated Show resolved Hide resolved

Update packages/@react-aria/select/docs/useSelect.mdx

be4c0d6

reidbarber reviewed Jul 14, 2023

View reviewed changes

packages/@react-aria/select/src/HiddenSelect.tsx Outdated Show resolved Hide resolved

Update packages/@react-aria/select/src/HiddenSelect.tsx

cd41037

LFDanLu reviewed Jul 14, 2023

View reviewed changes

reidbarber previously approved these changes Jul 14, 2023

View reviewed changes

reidbarber reviewed Jul 14, 2023

View reviewed changes

packages/@react-aria/select/src/HiddenSelect.tsx Outdated Show resolved Hide resolved

Update packages/@react-aria/select/src/HiddenSelect.tsx

efc869a

reidbarber dismissed their stale review via efc869a July 14, 2023 22:43

reidbarber previously approved these changes Jul 14, 2023

View reviewed changes

reidbarber reviewed Jul 14, 2023

View reviewed changes

packages/@react-aria/select/test/HiddenSelect.test.tsx Outdated Show resolved Hide resolved

reidbarber dismissed their stale review via 818f2c1 July 14, 2023 22:51

reidbarber and others added 3 commits July 14, 2023 17:51

Update packages/@react-aria/select/test/HiddenSelect.test.tsx

818f2c1

Move data attribute to container and add ignore rule to storybook axe

0e169ab

Merge branch 'main' into 3578-add-data-attr-to-hidden-select

a1c77d7

LFDanLu previously approved these changes Jul 15, 2023

View reviewed changes

LFDanLu reviewed Jul 17, 2023

View reviewed changes

reidbarber previously approved these changes Jul 17, 2023

View reviewed changes

LFDanLu previously approved these changes Jul 17, 2023

View reviewed changes

Merge branch 'main' into 3578-add-data-attr-to-hidden-select

86972ce

LFDanLu requested a review from majornista July 17, 2023 23:56

yihuiliao previously approved these changes Jul 19, 2023

View reviewed changes

yihuiliao added 2 commits July 19, 2023 14:28

Merge branch 'main' into 3578-add-data-attr-to-hidden-select

d34b10b

Merge branch 'main' into 3578-add-data-attr-to-hidden-select

0b6cc5f

devongovett reviewed Jul 19, 2023

View reviewed changes

updating aria-attribute

d9ed730

LFDanLu dismissed stale reviews from yihuiliao, reidbarber, and themself via d9ed730 July 21, 2023 21:17

LFDanLu added 2 commits July 21, 2023 14:27

fix tests

84d5acd

Merge branch 'main' into 3578-add-data-attr-to-hidden-select

da4b452

reidbarber approved these changes Jul 31, 2023

View reviewed changes

Merge branch 'main' into 3578-add-data-attr-to-hidden-select

493554f

ktabors reviewed Aug 1, 2023

View reviewed changes

devongovett approved these changes Aug 1, 2023

View reviewed changes

Merge branch 'main' into 3578-add-data-attr-to-hidden-select

55348d8

LFDanLu merged commit 3aa6dbe into adobe:main Aug 1, 2023

	The data attribute `data-rsp-a11y-ignore` is included on the hidden select element and can be used to ignore the false positives in these tools.
	The data attribute `data-a11y-ignore` is included on the hidden select element and can be used to ignore the false positives in these tools.

#3578 Add the ability to add a data attribute to <HiddenSelect /> to handle false a11y positives #4759

#3578 Add the ability to add a data attribute to <HiddenSelect /> to handle false a11y positives #4759

Conversation

aloysb commented Jul 12, 2023

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

reidbarber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reidbarber left a comment

Choose a reason for hiding this comment

Uh oh!

aloysb commented Jul 14, 2023

Uh oh!

reidbarber commented Jul 14, 2023

Uh oh!

rspbot commented Jul 14, 2023

Uh oh!

reidbarber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

LFDanLu commented Jul 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rspbot commented Jul 17, 2023

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

devongovett Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aloysb commented Jul 20, 2023 via email

Uh oh!

aloysb commented Jul 26, 2023

Uh oh!

LFDanLu commented Jul 26, 2023

Uh oh!

reidbarber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktabors commented Aug 1, 2023

Uh oh!

rspbot commented Aug 1, 2023

Uh oh!

rspbot commented Aug 1, 2023

devongovett Jul 19, 2023 •

edited

Loading