tailwindlabs / headlessui

Completely unstyled, fully accessible UI components, designed to integrate beautifully with Tailwind CSS.
https://headlessui.com
MIT License
25.8k stars 1.07k forks source link

VoiceOver doesn't read the Combobox.Option (single value selection) #2129

Closed afercia closed 1 year ago

afercia commented 1 year ago

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

v1.7.1

What browser are you using?

Safari and VoiceOver (macOS)

Reproduction URL

Demo page https://headlessui.com/react/combobox or run the playground and go to http://localhost:3000/combobox/combobox-with-pure-tailwind

Describe your issue

Hello. Thank you for making headlessui components as accessible as possible, much appreciated.

I read some related issues / PRs and it seems to me there's a bit of misunderstanding about the usage of aria-selected on the combobox listbox. See for example https://github.com/tailwindlabs/headlessui/pull/1812, https://github.com/tailwindlabs/headlessui/issues/1599, and https://github.com/tailwindlabs/headlessui/pull/1243.

Safari and VoiceOver (macOS) don't read the Combobox.Option because the aria-selected usage in a combobox listbox is supposed to be a bit different from the one in a standard listbox. It's a subtle difference but in all the comboboxes I've helped to build in the past it always turned out that other screen readers (NVDA, JAWS) are happy with aria-activedescendant and do not care much about aria-selected. Instead, VoiceOver refuses to read the options if aria-selected is omitted or used incorrectly.

In the ARIA authoring practices, the example that is closest to the headlessui Combobox (single selection) is the Editable Combobox with List Autocomplete.

Quoting from the Listbox Popup Role, Property, State section of the W3C combobox example:

  • [aria-selected="true"] Specified on an option in the listbox when it is visually highlighted as selected.
  • [aria-selected="true"] Occurs only when an option in the list is referenced by aria-activedescendant.

That is: in this pattern, aria-selected needs to be only set on the currently 'highlighted' option. It doesn't have anything to do with the 'selection' state, which is a concept that is extraneous to the W3C combobox pattern.

This is also mentioned in the main page of the Combobox pattern: https://www.w3.org/WAI/ARIA/apg/patterns/combobox/#wai-aria-roles-states-and-properties-6

For a combobox that controls a listbox, grid, or tree popup, when a suggested value is visually indicated as the currently selected value, the option, gridcell, row, or treeitem containing that value has aria-selected set to true.

This is different from the (stand alone) Listbox pattern, where aria-selected is indeed used for the selection state.

In short: in the Combobox Listbox, aria-selected should be based on the active prop rather than the selected prop. Of course, the multiselectable pattern needs to be handled differently as well as the stand-alone Listbox.

Note: in https://github.com/tailwindlabs/headlessui/issues/1599#issuecomment-1235421018 it is mentioned that this woudl make VoiceOver announce every active option as selected. While slightly annoying, this is up to the screen reader implementation and it's in a way the expected behavior. Other screen readers opted to not announce selected, for example NVDA doesn't, but again this is a vendor's choice. Regardless, aria-selected=true needs to be set only on the highlighted option referenced by aria-activedescendant.

To reproduce the bug:

W3C example:

afercia commented 1 year ago

I'm attaching a few animated GIFs to better illustrate. Although there's no audio, you can see what VoiceOver reads in the VoiceOver caption panel.

W3C example of a Combobox list box (no multi selection). See aria-selected="true" is set only on the highlighted option and omitted from all the other options:

combobox-list

Demo page: options aren't read out:

bug

W3C demo: options are read out:

w3

Playground page running locally, where I changed aria-selected to be based on active ? true : undefined rather than on the selected prop. Of course this is just a quick try for testing purposes. A complete fix would need more work. With this simple change in place VoiceOver now reads out the option:

fix

afercia commented 1 year ago

For reference: See some tests data on a11ysupport.io: https://a11ysupport.io/tests/apg__aria-1-2-combobox-with-list-autocomplete-example#sr-feature-index-5 where the announcement of 'selected' is considered the expected behavior. VoiceOver and Narrator do announce selected and are marked as green. All other tested implementation opted to not announce it.

afercia commented 1 year ago

Worth mentioning that to fully meet the ARIA pattern, there are a couple things to improve:

afercia commented 1 year ago

There's another thing to mention about VoiceOver: it is known to fail anyways when the combobox has no value, or at least no value when the listbox is open. See the 'partial' support for the aria-activedescendant attribute: https://a11ysupport.io/tests/apg__aria-1-2-combobox-with-list-autocomplete-example#sr-feature-index-0

This is really a VoiceOver bug.

In my testing, there is a way to fix it but it would require some manipulation of the input value, which is sort-of a DOM method. I do realize such a fix wouldn't be entirely desirable in this kind of components. The fox would be very small though and could live in a sort of separated DOM utils. I'm mentioning it in case uyou may want to consider it.

Basically, VoiceOver fails when the Listbox opens and the input has no value. This can be reproduced on the W3C example at https://www.w3.org/WAI/ARIA/apg/example-index/combobox/combobox-autocomplete-list.html

In the headlessui demo page at https://headlessui.com/react/combobox

I found that 'clearing' and repopulating the input value on the fly when the listbox opens makes VoiceOver behave way better. In the OpenCombobox method, before declaring and assigning activeOptionIndex, something along these lines woul dwork:

    let input = state.dataRef.current.inputRef.current;
    let inputValue = input.value;
    input.value = '';
    input.value = inputValue;

Maybe coded a bit better and elegantly than an a11y specialist like me is able to do 🙂

As said, I do realize this is sort of DOM manipulation that shouldn't live in a component. Also, it's a workaround to fix a specific screen reader bug. I doubt Apple will ever fix this upstream so maybe worth considering it. Keeping it in a separate DOM utility method may alleviate the impact on the code cleanliness.

afercia commented 1 year ago

I apologize because this is a real TL;DR. A very long reading. Also, I'm afraid I won't have time to contribute with a pull request but do please ping me if any help is needed. Thanks for your patience and an early Happy New Year everyone! 🎉

RobinMalfait commented 1 year ago

Hey! Thank you for your bug report! Much appreciated! 🙏

This is a very in-depth bug report and I really appreciate that! I will dig in the first thing on Monday in the New Year. Thanks again!

terracoda commented 4 weeks ago

I heard from Apple's Web Accessibility Team yesterday that aria-activedescendent issues have been addressed in Sequoia.