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.

Sign up for GitHub

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

Jump to bottom

fix(#4080): ComboBox: aXe throws an error when the virtualized listbox is scrollable #4081

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

fix(#4080): ComboBox: aXe throws an error when the virtualized listbox is scrollable #4081

wants to merge 13 commits into from
+29 −17

Conversation

majornista
Copy link
Collaborator

Closes #4080

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue #4080.
  • 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:

Adobe/Accessibility

@rspbot
Copy link

rspbot commented Feb 16, 2023

Build successful! 🎉

@devongovett
Copy link
Member

Wouldn't that only "fix" it if you aren't running aXe on mobile?

Also I think this makes the items inside the combobox tabbable in some cases, which probably isn't desirable. Seems wrong to me to make items that will receive focus via aria-activedescendant DOM focusable - the whole point of using that attribute is so they aren't. Can we file a bug against aXe for this?

@rspbot
Copy link

rspbot commented Feb 17, 2023

Build successful! 🎉

@majornista
Copy link
Collaborator Author

majornista commented Feb 17, 2023 •
edited
Loading

Wouldn't that only "fix" it if you aren't running aXe on mobile?

Also I think this makes the items inside the combobox tabbable in some cases, which probably isn't desirable. Seems wrong to me to make items that will receive focus via aria-activedescendant DOM focusable - the whole point of using that attribute is so they aren't. Can we file a bug against aXe for this?

This is kind of a peculiar case about which the WAI-ARIA spec is unclear. If we don't override role="combobox" with role="searchbox" at

react-spectrum/packages/@react-spectrum/combobox/src/MobileComboBox.tsx

Line 335 in 6e30b87

inputProps.role = 'searchbox';
, aXe does not throw the Scrollable region must have keyboard access error. I presume this is because aXe understands the relationship between a combobox and a listbox. Sticking with role="combobox" also means that we no longer need to remove aria-expanded from the input: #4078, which @jnurthen has logged as a WAI-ARIA spec issue: w3c/aria#1875.

I kind of think that if role="searchbox" accepts aria-autocomplete="list", aria-haspopup, aria-controls and aria-activedescendant, is should accept aria-haspopup, and aXe should treat the listbox as it would for the combobox pattern. I suspect aXe doesn't do this because we should be using role="combobox" for this sort of functionality. For example, in HTML5 a text input specifying a list using an IDREF for a datalist is exposed in the accessibility tree as a combobox.

Is it essential to override role="combobox" with role="searchbox" so that VoiceOver on iOS will announce the hint when focused on the input as "double tap to edit text" rather than "double tap to collapse?"

cc @jnurthen, @LFDanLu and @devongovett

@rspbot
Copy link

rspbot commented Feb 25, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 23, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 27, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 30, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 30, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented May 15, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented May 17, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented May 25, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented May 25, 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' }

@devongovett
Copy link
Member

also means that we no longer need to remove aria-expanded from the input

I think aria-expanded doesn't really make much sense for mobile combo box anyway, because there is no concept of expanding or collapsing it. The listbox with suggestions is a sibling of the search box, and doesn't expand or collapse, so it would always announce as expanded which isn't very useful.

Is it essential to override role="combobox" with role="searchbox" so that VoiceOver on iOS will announce the hint when focused on the input as "double tap to edit text" rather than "double tap to collapse?"

Feels weird to me since that action would not actually work as the screen reader is describing it. The expanding/collapsing aspect of a combobox seems to be an assumption of screen readers that doesn't apply here.

Regardless, I tested this in Chrome with VoiceOver and it seems that you cannot navigate with Control + Option + arrow keys anymore after this change. VoiceOver focuses the option element within the ListBox, which causes DOM focus to move away from the input, and the popover to close. Before, DOM focus would stay on the input and only the virtual cursor would move.

Also it wouldn't solve the original issue which was related to MobileComboBox because the change doesn't affect mobile devices (the isIOS and isAndroid checks). So it would only work if you were using some kind of mobile emulation mode potentially. Seems like more trouble than it's worth? Can we mark this as a false positive?

snowystinger
snowystinger reviewed Jun 23, 2023
View reviewed changes
packages/@react-spectrum/autocomplete/test/SearchAutocomplete.test.js
@@ -115,9 +115,13 @@ function testSearchAutocompleteOpen(searchAutocomplete, listbox, focusedItemInde
expect(items[1]).toHaveTextContent('Two');
expect(items[2]).toHaveTextContent('Three');

expect(listbox).not.toHaveAttribute('tabIndex');
expect(listbox).toHaveAttribute('tabIndex', typeof focusedItemIndex === 'undefined' ? '0' : '-1');
Copy link
Member

Choose a reason for hiding this comment

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

do we expect focusedItemIndex to be undefined at this point? or is this a React 16/17/18 difference?

@scottaohara scottaohara mentioned this pull request Jun 6, 2024
Open
@devongovett devongovett force-pushed the main branch 2 times, most recently from 8cb1a5d to 3013156 Compare July 23, 2024 22:43
@devongovett devongovett force-pushed the main branch 2 times, most recently from a79adcf to 3013156 Compare September 30, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snowystinger snowystinger snowystinger left review comments

@devongovett devongovett Awaiting requested review from devongovett

@reidbarber reidbarber Awaiting requested review from reidbarber

@LFDanLu LFDanLu Awaiting requested review from LFDanLu

@dannify dannify Awaiting requested review from dannify

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
accessibility bug Something isn't working rsp:ComboBox
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ComboBox: aXe throws an error when the virtualized listbox is scrollable
4 participants
@majornista @rspbot @devongovett @snowystinger