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

Add accessible labelling to submenu buttons. #36631

Merged
merged 3 commits into from
Nov 30, 2021
Merged

Conversation

tellthemachines
Copy link
Contributor

Description

Fixes #36598.

Submenu buttons, whether in Page List or the Submenu block, don't provide enough context about their use to screen readers. This PR adds an aria-label to the buttons with the item name, followed by "submenu".

In VoiceOver, this means that instead of just reading out "collapsed, button" it now reads out "[Page name], submenu, collapsed, button". I haven't checked NVDA/Windows yet.

How has this been tested?

Create a Submenu in a Navigation block, and/or add a Page List with submenus to the Navigation block.
Test with both "open on click" and the default open on hover option (with open on hover, make sure submenu indicators are enabled, otherwise the submenu just opens on focus instead of click).
Verify that screen readers read out the parent item label and the word "submenu" when submenu button is focused.

I'd appreciate especially testing on Windows screen readers!

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@tellthemachines tellthemachines added [Block] Page List Affects the Page List Block [Block] Submenu Affects the Submenu Block - for submenus in navigation [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Nov 19, 2021
@tellthemachines tellthemachines added this to 👀 Needs review in WordPress 5.9 Must-Haves via automation Nov 19, 2021
@carolinan
Copy link
Contributor

I believe the word submenu in the label needs to be translation ready?

Copy link
Contributor

@carolinan carolinan left a comment

Choose a reason for hiding this comment

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

Tested with NVDA on Windows using Chrome, Firefox and Opera.
The submenu toggle button works well for both the page list and submenu.
Both for the first level and submenus of submenu items.

Open on click disabled:
The button after the link is announced as "button collapsed [parent item name] submenu".

Open on click enabled:
The button is announced as; "button collapsed [parent item name] submenu".

The order is different if tabs are used to navigate, but the same information is included:
[parent item name] submenu button collapsed.

This can be approved when the tests pass.

@tellthemachines
Copy link
Contributor Author

The tests are finally passing 😅 I think this is ready for review again.

Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 .

The only thing I could nitpick on is whether we should make the manual concatenation of $label and __( 'submenu ') a translatable string instead. So that translators could re-order them based on language conventions, or add other punctuation marks if needed. I do think it might be over-engineered though and might not be needed.

@tellthemachines
Copy link
Contributor Author

Thanks for the review @kevin940726 ! Good point about the string concatenation. I don't know in depth how translation works, would translators be able to decide the order of translatable string and variable? It's worth looking into, perhaps as a further enhancement.

@tellthemachines tellthemachines merged commit f43cfbc into trunk Nov 30, 2021
WordPress 5.9 Must-Haves automation moved this from 👀 Needs review to ✅ Done Nov 30, 2021
@tellthemachines tellthemachines deleted the fix/submenu-buttons branch November 30, 2021 05:42
@github-actions github-actions bot added this to the Gutenberg 12.1 milestone Nov 30, 2021
@kevin940726
Copy link
Member

I don't know in depth how translation works, would translators be able to decide the order of translatable string and variable?

Yes, for example in this case, label + ' ' + __('submenu') is always gonna be translated like that. But if we use a variable to represent label inside the translation text, something like sprintf( __('%s submenu'), label ), then the translators will be able to rearrange them to submenu %s or even submenu: %s.

It's worth looking into, perhaps as a further enhancement.

Agreed!

@noisysocks noisysocks removed the Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 6, 2021
noisysocks pushed a commit that referenced this pull request Dec 6, 2021
* Add accessible labelling to submenu buttons.

* Make strings translatable.

* Fix php lint error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Page List Affects the Page List Block [Block] Submenu Affects the Submenu Block - for submenus in navigation [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

page list submenu button is missing an accessible name
4 participants