Add combobox role to TextInput with suggestions #6079

eatyourgreens · 2022-04-20T11:24:17Z

Accessibility improvements to TextInput when suggestions are present, adding ARIA roles and states for a combobox with a list of selectable options.

What does this PR do?

Add the combobox and listbox roles to TextInput, plus their required ARIA states, when suggestions are present. The changes should follow the WAI-ARIA 1.2 design pattern for a combobox with manual autocomplete.
https://w3c.github.io/aria-practices/examples/combobox/combobox-autocomplete-list.html

Where should the reviewer start?

The changes are all in src/components/js/TextInput/Textinput.js and the TextInput snapshots.

What testing has been done on this PR?

Storybook: I've tried out the TextInput with suggestions and with a default suggestion in Safari/VoiceOver and Firefox/VoiceOver. I don't have access to a Windows machine with a screenreader, so I haven't tested JAWS or NVDA.

How should this be manually tested?

Use the TextInput stories with and without a screenreader. Screenreaders should announce the controls as a combobox, with options, for the stories where the text input has suggestions. Otherwise, the behaviour of the text input should not have changed.

I've also made a codepen with the WAI-ARIA 1.2 example combobox, which I found useful to compare screenreader behaviour.
https://codepen.io/eatyourgreens/full/abEQrBg

Do Jest tests follow these best practices?

screen is used for querying.
The correct query is used. (Refer to this list of queries)
userEvent is used in place of fireEvent.
asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

No.

Should this PR be mentioned in the release notes?

Yes, maybe as accessibility improvements for text inputs.

Is this change backwards compatible or is it a breaking change?

It should be backwards compatible. One possible breaking change is that VoiceOver opens a combobox's options with ctrl-alt-Space. I noticed, while testing in VoiceOver, that pressing Space selects the current active option so there could be a conflict of keyboard handlers there.

jcfilben

Screenreader output looks good. One thing I noticed was that previously when using a screenreader you could select one of the suggestions with 'ctrl-option-space' (when using voice over on mac). In this pull request when I hit 'ctrl-option-space' nothing happens. I think we should preserve the behavior of 'ctrl-option-space' selecting an option

eatyourgreens · 2022-04-21T06:55:53Z

@jcfilben ctrl-opt-space is the command to list the listbox options in VoiceOver, so I think there’s a clash there.

You can compare it against the WAI-ARIA 1.2 example here: https://codepen.io/eatyourgreens/full/abEQrBg

EDIT: I'm seeing different behaviour for ctrl-option-space between the codepen example and the Grommet text input. In Grommet, ctrl-option-space does nothing but in the codepen example it confirms my current selection.

eatyourgreens · 2022-04-21T09:48:36Z

Although I'm unable to test in JAWS and NVDA, I found a comprehensive set of combobox tests on https://a11ysupport.io. It looks like VoiceOver/Safari has partial support for selecting an option, but support is good in JAWS and NVDA.

jcfilben · 2022-04-27T20:17:30Z

Looking through the tests on the a11ysupport site I couldn't find the one where they are testing selecting an option from the listbox, there are a lot of tests so I definitely could have missed it. Can you send the name of the test you are looking at that shows partial support for selecting an option with voice over?

Looking at these listbox examples on w3.org I am able to navigate with ctrl-option-(up or down arrow) and then select an option with ctrl-option-space. From the examples I have seen ctrl-option-space is used to select an option, I haven't seen it used to list the options in the listbox. (Testing with voiceover in chrome browser)

eatyourgreens · 2022-04-27T21:32:51Z

My mistake. I was looking at the tests for aria-activedescendant, which should track the active option when you navigate the list.
https://a11ysupport.io/tests/apg__aria-1-2-combobox-with-list-autocomplete-example/aria__aria-activedescendant_attribute/convey_value/vo_macos/safari

When I use ctrl-opt-shift on the w3c example, VoiceOver announces “confirm Alaska, State, list box popup” but the Grommet TextInput does nothing. The only difference that jumps out to me is that the Grommet implementation has buttons inside the options. Maybe my using a clickable button inside a clickable option is causing problems? I could see the equivalent HTML causing problems, because of nested clickable elements.

<select>
  <option><button>Alaska</button></option>
  …
</select>

eatyourgreens · 2022-04-27T22:02:57Z

I saw that Grommet Select uses listbox, so I’ll take a look at that to see how the option roles are handled. I thought a listbox was required to have options as children but I am not 100% sure of that now.

jcfilben · 2022-04-27T22:29:43Z

src/js/components/TextInput/TextInput.js

+                      id={`${id}-listbox-option-${index}`}
+                      role="option"
+                      aria-selected={selected ? 'true' : undefined}


I'm thinking we should move role="option" and aria-selected={selected ? 'true' : undefined} to be on the Button instead of the li. Also not sure if we need to have the id, I wasn't seeing any difference in screenreader output with vs without it

Moving role="option" to the button allows the screenreader users to select an option with ctrl-option-space

The ID links up aria-activedescendant on the containing combobox, so that changes can be announced even though keyboard focus is actually in the text input.

I’m not sure about having a list item role that then contains an option. I think the options might have to be direct children of the listbox, or if not then the default role of the li elements might need to be changed too.

Great that this works in VoiceOver, though. Hopefully it’s valid ARIA too.

If VO had better support for activedescendant, I think some of the live region updates could be removed too. I’m finding they sometimes lag behind the actual active option, or at other times I hear the active option announced multiple times.

JAWS and NVDA should pick up the active option, via aria-activedescendant, and announce it when you navigate the list with the arrow keys.

At Zooniverse, we use the Storybook a11y addon to test for things like invalid role nesting, or invalid role and state combinations.

I can give this change a try to see if the validator complains.

eatyourgreens · 2022-04-28T11:10:36Z

@jcfilben thanks for the additional testing in VoiceOver. I found this in the ARIA spec for listboxes.

Each option in the listbox has role option and is contained in or owned by either:

The element with role listbox.

An element with role group that is contained in or owned by the element with role listbox.

https://w3c.github.io/aria-practices/#listbox_roles_states_props

So the options must be direct children of the listbox, or maybe I should change the li roles to group instead of listitem. Otherwise, I think the list of options could be using invalid markup.

Another option would be to remove the buttons completely, and just have a styled list. That seems like a major change to me. I'm not sure what the implications would be.

eatyourgreens · 2022-04-28T14:45:30Z

I added @storybook/addon-a11y to my local copy of the Grommet storybook. It finds one violation in the Suggestions story for TextInput: there's no label for the input element.

It doesn't find any ARIA problems on this branch, so I think this PR might be ready.

jcfilben · 2022-04-28T15:27:52Z

src/js/components/TextInput/TextInput.js

-                        active={activeSuggestionIndex === index}
+                        id={`${id}-listbox-option-${index}`}
+                        role="option"
+                        aria-selected={selected ? 'true' : undefined}


Can we update aria-selected to just be true or false instead of true or undefined? Setting aria-selected to undefined means the option is not selectable (see https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-selected#values)

Suggested change

aria-selected={selected ? 'true' : undefined}

aria-selected={selected}

Sure. I copied this from the WAI-ARIA example but hadn't seen that note on MDN.

I think aria-selected must be a string, not a boolean attribute, though.

I've updated the options to add aria-selected='false' as the default state.

jcfilben · 2022-04-28T15:29:35Z

I added @storybook/addon-a11y to my local copy of the Grommet storybook. It finds one violation in the Suggestions story for TextInput: there's no label for the input element.

I haven't used the addon-a11y tool before but it looks interesting! I'm thinking it could be useful to add to our storybook to help catch more accessibility issues

eatyourgreens · 2022-04-28T16:00:48Z

I haven't used the addon-a11y tool before but it looks interesting! I'm thinking it could be useful to add to our storybook to help catch more accessibility issues.

It's very useful. One other thing it does is it adds a colour-blindness simulator to the Storybook canvas toolbar.

jcfilben

Looks good to me!

ericsoderberghp

I really like seeing this addition. Thanks for working on it. One small comment.

ericsoderberghp · 2022-04-29T18:52:25Z

src/js/components/TextInput/TextInput.js

@@ -327,6 +327,8 @@ const TextInput = forwardRef(
          {...dropProps}
        >
          <ContainerBox
+            id={`${id}-listbox`}


I think we should include these id elements conditionally, only if the caller provides an id, otherwise we are likely to be rendering multiple elements in the DOM with the same id. This would follow the pattern we have at line 321.

Oh, that’s a good point. If the option IDs are omitted, then aria-activedescendent would have to be removed, which then means the active option can’t be announced by a screen reader.

Similarly, a combobox must be linked to a listbox via aria-controls, so removing the listbox ID would also remove the combobox role from the text input.

Basically, I think all the combobox props are conditional on IDs being defined.

Is there any way that we could generate a unique ID by default for a component, if one hasn’t been passed in? ARIA pretty much relies on element IDs to link up related DOM nodes.

We haven't gone down that path so far. And instead pursuing things like eslint plugins to warn when an id is missing, notably in input + label situations.

https://github.com/grommet/eslint-plugin-grommet

Sounds good. I've added IDs to the storybook, and conditional checks so that the combobox role and states are only added to the text input if the component has both id and suggestions. I've added conditional checks to the listbox and option IDs too.

Add the `combobox` and `listbox` roles to TextInput, plus their required ARIA states, when suggestions are present. The changes should follow the WAI-ARIA 1.2 design pattern for a combobox with manual autocomplete. https://w3c.github.io/aria-practices/examples/combobox/combobox-autocomplete-list.html

Check for a supplied ID. Only add the combobox roles and states, and listbox element IDs, if the component has an ID.

eatyourgreens force-pushed the textInput-combobox branch from da0328f to 4adfbc6 Compare April 20, 2022 11:29

jcfilben reviewed Apr 20, 2022

View reviewed changes

eatyourgreens mentioned this pull request Apr 21, 2022

Text task with suggestions should be marked up as a combobox text input zooniverse/front-end-monorepo#3031

Closed

eatyourgreens force-pushed the textInput-combobox branch from f12e64b to 35ead4e Compare April 25, 2022 15:37

jcfilben reviewed Apr 27, 2022

View reviewed changes

eatyourgreens force-pushed the textInput-combobox branch from 35ead4e to 5758840 Compare April 28, 2022 13:53

jcfilben reviewed Apr 28, 2022

View reviewed changes

eatyourgreens force-pushed the textInput-combobox branch from 60a0a7c to 367ee13 Compare April 29, 2022 16:17

eatyourgreens requested a review from jcfilben April 29, 2022 16:37

jcfilben approved these changes Apr 29, 2022

View reviewed changes

jcfilben requested a review from ericsoderberghp April 29, 2022 17:57

ericsoderberghp reviewed Apr 29, 2022

View reviewed changes

eatyourgreens added 5 commits April 29, 2022 22:30

Move option role and states to Button

1010ab4

Set aria-selected on all the listbox options

98d63a7

Add IDs to stories

43d76f3

Add conditional checks for component id

fc58d66

Check for a supplied ID. Only add the combobox roles and states, and listbox element IDs, if the component has an ID.

eatyourgreens requested a review from ericsoderberghp April 29, 2022 22:49

ericsoderberghp approved these changes May 3, 2022

View reviewed changes

ericsoderberghp added 2 commits May 2, 2022 20:44

Merge branch 'master' into textInput-combobox

87dd1a6

Merge branch 'master' into textInput-combobox

7446c5a

ericsoderberghp merged commit 05de690 into grommet:master May 3, 2022

eatyourgreens deleted the textInput-combobox branch May 3, 2022 16:04

eatyourgreens mentioned this pull request Jun 29, 2023

App-project: Refactor collections modal and modify collection names zooniverse/front-end-monorepo#4866

Merged

11 tasks

eatyourgreens mentioned this pull request Aug 8, 2023

Simple Dropdown: type-to-complete search? zooniverse/front-end-monorepo#5130

Closed

This was referenced Sep 26, 2023

Revise Simple Dropdown ADR: no max limit on items zooniverse/front-end-monorepo#5128

Closed

Add search to SimpleDropdown. zooniverse/front-end-monorepo#5382

Merged

eatyourgreens mentioned this pull request Sep 25, 2024

DataChart - detail is not accessible via keyboard when using voiceover #7319

Open

	aria-selected={selected ? 'true' : undefined}
	aria-selected={selected}

Add combobox role to TextInput with suggestions #6079

Add combobox role to TextInput with suggestions #6079

Uh oh!

Conversation

eatyourgreens commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

Uh oh!

jcfilben left a comment

Choose a reason for hiding this comment

Uh oh!

eatyourgreens commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eatyourgreens commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcfilben commented Apr 27, 2022

Uh oh!

eatyourgreens commented Apr 27, 2022

Uh oh!

eatyourgreens commented Apr 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eatyourgreens Apr 27, 2022 • 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!

eatyourgreens Apr 27, 2022 • 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!

eatyourgreens commented Apr 28, 2022

Uh oh!

eatyourgreens commented Apr 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eatyourgreens Apr 28, 2022 • 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!

jcfilben commented Apr 28, 2022

Uh oh!

eatyourgreens commented Apr 28, 2022

Uh oh!

jcfilben left a comment

Choose a reason for hiding this comment

Uh oh!

ericsoderberghp 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!

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!

Uh oh!

eatyourgreens commented Apr 20, 2022 •

edited

Loading

eatyourgreens commented Apr 21, 2022 •

edited

Loading

eatyourgreens commented Apr 21, 2022 •

edited

Loading

eatyourgreens Apr 27, 2022 •

edited

Loading

eatyourgreens Apr 27, 2022 •

edited

Loading

eatyourgreens Apr 28, 2022 •

edited

Loading