Font Library: Improve usability of font variant selection #56158
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mikachan
added
[Type] Enhancement
mikachan
requested review from
ajitbohra,
kevin940726 and
Mamaduka
as code owners
|
Size Change: +187 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
mikachan
changed the title
|
Flaky tests detected in 2b9f689. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7012622744
|
jffng
approved these changes
Nov 29, 2023
There was a problem hiding this comment.
Hey! This is testing well for me. I tried both the mouse and keyboard behavior, verified it's working as expected as well as no label is rendered in the Checkbox Control component:
Screen.Recording.2023-11-29.at.10.19.25.AM.mov
I left one small suggestion, other than that LGTM.
|
Oh one more thing I thought of when reviewing this PR — should we open an issue for more UX improvements? In particular, it could be useful to have a Select / Unselect all variants option. |
Yep, good idea! Opened an issue here: #56688 |
|
Noting that the WordPress Accessibility standards recommend to not use wrapping labels, see https://make.wordpress.org/accessibility/handbook/markup/core-accessibility-standards/#labeling This also causes a double labeling, as the label element still uses a Reported at #58299 |
|
While I do understand the good intent behind this change, I'm afraid I have to remark that:
For the greater good of the WordPress project I'm not sure pull requests that do not follow this project guidelines should be allowed. |
ciampo
reviewed
Jan 26, 2024
There was a problem hiding this comment.
Hey there 👋
Echoing @afercia 's message, we should be careful when making changes to public components APIs, especially when those changes have the potential to affect the component's accessibility negatively (for example, the snapshot diff in this PR already showed that the input in the test ended up unlabeled)
While the original intention behind these changes is understandable, it is now possible for consumers of CheckboxControl to avoid setting a label altogether, which goes against the efforts we've been making to improve controls accessibility.
Were any alternative approaches discussed?
ciampo
reviewed
Jan 26, 2024
| */ | ||
| label?: string; | ||
| label?: string | false; |
There was a problem hiding this comment.
We usually try to avoid mixing different primitive types on the same prop, where possible.
|
Hi @afercia and @ciampo, thanks for the feedback here. The intention here was only to improve the accessibility of the checkboxes in the font library, but I understand the points you've raised. It sounds like we need to revert the changes to the component, but we'll likely need to redesign the font library checkboxes in order for them to be accessible again. I'll add this work to the font library tracking issue. |
25 tasks
I'm not sure big changes are really needed. We just need to make sure the label is correctly associated to the checkbox. Please do not alter the component default behaevior. A correctly associated label is natively clickable. It's built-in accessibility. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This updates the font variant markup so that the checkbox controls are wrapped inside a label element. This means that any part of the font variant panel can be clicked to interact with the checkbox, rather than only the checkbox itself.
Why?
This improves the usability of these checkboxes, as you can click on a much larger area for the toggle interactions. It also should improve their accessibility, as previously these checkboxes did not include any labels. Now when a checkbox is read out by a screenreader, it will read out the font variant name.
How?
There are two main updates: updating the markup of the font variant selection for both the library and collection versions; and updating the
CheckboxControlcomponent to include the option for preventing a label from being rendered, in order to allow for a custom label to be used outside the component.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Recording.2023-11-15.at.16.45.14.mov
Screen.Recording.2023-11-15.at.16.48.28.mov