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.

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

CustomSelectControl: set aria-hidden to empty option list #21298

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Mar 31, 2020

Description

CustomSelectControl renders the listbox containing the options even when the control is not open. That's causing Orca (Linux screen reader) to read Blank every time it's navigated through.

In parallel, using an a11y audit tool like AXE, shows an error because of that:
imatge

This PR adds aria-hidden={ true } to the options list when it's empty, so it's ignored by assistive technologies.

How has this been tested?

  • In the storybook, I navigated through the CustomSelectControl with the screen reader to verify there were no regressions and it didn't read Blank.
  • I did the same with the Font Size Picker.

@Aljullu Aljullu added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] UI Components Impacts or related to the UI component system labels Mar 31, 2020
@Aljullu Aljullu self-assigned this Mar 31, 2020
@github-actions
Copy link

github-actions bot commented Mar 31, 2020

Size Change: +4 B (0%)

Total Size: 835 kB

Filename Size Change
build/components/index.js 198 kB +4 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/index.js 106 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.05 kB 0 B
build/block-library/editor.css 7.05 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/style-rtl.css 7.14 kB 0 B
build/block-library/style.css 7.14 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.54 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 27.7 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 11 kB 0 B
build/edit-site/style-rtl.css 5.26 kB 0 B
build/edit-site/style.css 5.25 kB 0 B
build/edit-widgets/index.js 8.33 kB 0 B
build/edit-widgets/style-rtl.css 5 kB 0 B
build/edit-widgets/style.css 5 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.3 kB 0 B
build/editor/style-rtl.css 3.27 kB 0 B
build/editor/style.css 3.27 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.32 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@youknowriad youknowriad requested review from diegohaz and ItsJonQ April 1, 2020 08:05
@youknowriad youknowriad added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Apr 1, 2020
@ItsJonQ
Copy link

ItsJonQ commented Apr 1, 2020

@Aljullu Thanks for working on this!

I just tested it locally. Unfortunately, I did find an interaction issue :(.

After selecting an item (keyboard or mouse), the focus doesn't return back to the Button.
Perhaps, internally, downshift requires the element with {...menuProps} present in order for it to manage focus jumping.

Let me know if you're experiencing the same issue

Thanks!

@Aljullu Aljullu changed the title CustomSelectControl: don't render option list if empty CustomSelectControl: set aria-hidden to empty option list Apr 2, 2020
@Aljullu
Copy link
Contributor Author

Aljullu commented Apr 2, 2020

Oh, right, I completely missed that.

Another approach I had in mind was to use aria-hidden instead of not rendering the list. I tested it and it seems to fix the focus issue. I updated this PR accordingly. @ItsJonQ willing to take another look?

@Aljullu Aljullu force-pushed the fix/hide-empty-custom-select-control-list branch 2 times, most recently from 31a8712 to 7fdff59 Compare April 2, 2020 09:33
@Aljullu Aljullu force-pushed the fix/hide-empty-custom-select-control-list branch from 7fdff59 to 9281c52 Compare April 24, 2020 14:56
@Aljullu
Copy link
Contributor Author

Aljullu commented Apr 24, 2020

I rebased this branch to fix the conflicts that have recently appeared.

Copy link
Member

@diegohaz diegohaz left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks @Aljullu! :)

@Aljullu Aljullu merged commit b4f0a07 into master Apr 30, 2020
@Aljullu Aljullu deleted the fix/hide-empty-custom-select-control-list branch April 30, 2020 12:22
@github-actions github-actions bot added this to the Gutenberg 8.1 milestone Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants