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

Fix the Lock Block modal dialog group labelling and semantics #47248

Closed
afercia opened this issue Jan 18, 2023 · 4 comments · Fixed by #62795
Closed

Fix the Lock Block modal dialog group labelling and semantics #47248

afercia opened this issue Jan 18, 2023 · 4 comments · Fixed by #62795
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block editor /packages/block-editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jan 18, 2023

Description

When locking a block, a modal dialog opens. So far, so good.
There are a few accessibility issues to fix in the modal dialog content though.

1
The checkboxes within the modal are grouped in a div element with role=group and aria-labelledby. Turns out the aria-labelledby ID reference value points to the first checkbox label text. As a result, the actual name of the group is 'Lock all':

<div
role="group"
aria-labelledby={ instanceId }
className="block-editor-block-lock-modal__options"
>
<CheckboxControl
__nextHasNoMarginBottom
className="block-editor-block-lock-modal__options-title"
label={
<span id={ instanceId }>{ __( 'Lock all' ) }</span>
}

When tabbing into the group, screen readers will announce the group name 'Lock all", which is wrong. The complete announcement would be something like: Lock all, unticked, thickbox, Lock all, group:

Screenshot 2023-01-18 at 15 06 02

Instead, the group name should clarify what all the three checkboxes are about. That's something along the lines of 'Choose what to restrict or lock' (better wording needed).

Worth also reminding that a <fieldset> element with a visible <legend> element is always preferable to the ARIA pattern with role=group. One more thing to remind is that the labelling of a fieidset / group should be short: it's the group name. It's not the place for long descriptions or instructions.

2
The first checkbox <label> element contains a <span> element, not sure why. Please remove it. If an ID is needed, that should be set on the <label> element itself but it's likely that to fix the previous point an aria-label should be used in place of aria-labelledby so the ID won't be necessary any longer.

3
Checkboxes nesting.
The first checkbox 'Lock all' is logically a 'first-level' option that contains tho logical 'sub-options'. However, only the second and third checkboxes are list items in a single-level unordered list. There's no great value in using a list this way. If the goal is to convey the logical nesting of the options, the 'Lock all' checkbox should be within a list and the other two checkboxes should be in a nested list.
Also, worth reminding that Safari doesn't expose to screen readers the list role when the list is styled. VoiceOver doesn't announce the list as a list, while other screen readers do. To cover this Safari + VoiceOver inconsistency, each ul element should have a (technically redundant) role=list attribute.

Step-by-step reproduction instructions

  • Edit a post.
  • Select a paragraph block and choose 'Lock' from its toolbar > Options menu.
  • A 'Lock paragraph' modal dialog opens.
  • Use the Chrome dev tools > accessibility tan to inspect the div element with role=group.
  • Observe the div accessible name is 'Lock all'.
  • Inspect the first checkbox.
  • Observe its associated label element contains an unnecessary span element.
  • Inspect the other two checkboxes.
  • Observe they're within an unordered list, while the first checkbox isn't.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [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). [Package] Block editor /packages/block-editor [a11y] Labelling labels Jan 18, 2023
@Mamaduka Mamaduka self-assigned this Mar 29, 2023
@Mamaduka
Copy link
Member

Thank you for the great feedback, @afercia 🙇

If I recall correctly, I borrowed the markup from Preferences > Block Visiblity UI. Now I realize it didn't translate correctly for this model.

Question: Should we use the paragraph text as the <legend> value or shorten it?

This is the current text - "Choose specific attributes to restrict or lock all available options."

@afercia
Copy link
Contributor Author

afercia commented Apr 11, 2023

@Mamaduka thanks for the ping.

Question: Should we use the paragraph text as the value or shorten it?

A shorter text for the legend would be ideal. A longer description may be provided separately, if needed.

@afercia
Copy link
Contributor Author

afercia commented Aug 29, 2023

@Mamaduka @ciampo any objections if I assign this issue to myself and try a fix?

@Mamaduka
Copy link
Member

@afercia, no objections 🙇 Sorry, that I couldn't find time to work on it.

@Mamaduka Mamaduka assigned afercia and unassigned Mamaduka Aug 29, 2023
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block editor /packages/block-editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants