Fix the shadows Range control accessibility and usability #63908
Conversation
afercia
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
afercia
changed the title
|
Size Change: +2.46 kB (+0.14%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
I'm happy to try this. |
|
What about removing the title to keep the height as small as possible?
For example, the color picker doesn't have a title. I think this may not be an accessibility issue as long as the screen reader announces that the popover has been opened (the button has been expanded) and announces the label of the first focusable element inside the popover. |
|
If we're fixing a11y issues with labels and such, those PRs should only consist of those changes — they'd be much faster to wrap up/review if so. If we're making design changes as well, we need design reviews as well, which can often slow things down, as additional feedback/iterations are not uncommon. Just a note.
The Font Size control also uses this; it's an established (and expected) UX pattern.
However, I'm in favor of further simplifying this control, to not use isCustomInput and just surface the UnitControls for each (in a grid). This would not be dissimilar to the UX of border and radius controls (where we do not have additional RangeControl before custom inputs.
Just making the list of controls into a longer visual list of items does not improve usability of this particular control. |
|
That also works for me. Users can drag on unit controls to achieve a similar experience to range sliders. |
|
Thanks all for your review and feedback. Some points:
To me, any panel, popover, modal dialog, etc. should have a visible title. I do know there is large inconsistency about this in the editor, as I mentioned in the related issue. Still, clearly identifying what a panel, popover, modal dialog is about is not just a thing for screen readers, it's about general usability for all users.
Thanks for reminding me this point. I'm afraid I forgot about it, after more than 10 years of contribution to WordPress. I marked both this PR and the related issue with the 'Needs Design Feedback' label because as an experienced contributor to WordPress I think this issue needs a design change to be solved. So I'm not sure I understand how your consideration above can be any useful. I'm intentionally proposing a design change. Just a note. Besides that, I think you're missing that accessibility is not just about 'labels and such'. When the design itself is less than ideal and impacts accessibility and / or usability, the design needs to be changed. Range controls specifically are designed to allow users to fine tune a value in a defined range. Placing a range control in a spot with very limited horizontal space defeats the benefit of a range control in the first place. The current grid layout makes each range 'rail' have a width of 116 pixels, which is insufficient to be able to easily fine-tune a value. As such, the grid layout is, to me, just a wrong design and impacts usability to start with.
I'm not sure I understand why a longer list would be a problem. The point here is allowing the range controls to have more horizontal space. No gric. More height, yes. Users can always scroll. Anyways, further simplifying by removing the range control and only using the UnitControls makes sens to me. In this case, the grid layout could stay. |
afercia
force-pushed
the
fix/shadow-input-control-labeling
branch
from
a877444 to
31ba0ff
Compare
|
Flaky tests detected in 31ba0ff. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10579609113
|
afercia
changed the title
|
In the last commit I entirely removed the RangeControl and only kept the UnitControl. Honestly, I'm not sure I like it. Removing the RangeControl objectively reduces the ease of use of these settings. I'd still be in favor of keeping the combo RangeControl / UnitControl and remove the grid layout to allow more horizontal space. Also, I would like to fix the inconsistency about the placement of the toggle to switch from preset to custom values. This toggle is placed in different ways across the editor UI. This kind of inconsistencies add unnecessary mental processing and cognitive load for users. Summed to dozens of other small inconsistencies, the current UI is in a state where users are forced to continually process the UI, which doesn't help to provide a good user experience. I think all these inconsistencies should be fixed one by one to provide an UI that is consistent and predictable. Screenshot: before, after, and a comparison with other range controls where the toggle is placed after the control:
|
|
Last commit still needs a new review. On of my questions still needs a reply from design:
I'm not sure I understand why a longer list would be a problem. The point here is allowing the range controls to have more horizontal space. No gric. More height, yes. Users can always scroll. To me, the best option here would be to just place the four controls vertically stacked, to take the whole available width that is necessary for a range control. I don't understand how a grid layout can ebe beneficial in any way for users. I'd appreciate some feedback thanks. @t-hamano @WordPress/gutenberg-design |
|
Configuring shadows generally involves working with small values which is a bit fiddly with the range inputs. They're also limited by an arbitrary min/max which is restrictive on those occasions where you want to use larger values. In my opinion it seems fine to remove them in this case, and just use the regular unit inputs. |
There was a problem hiding this comment.
From a code perspective, this variable can be removed since it is only used by the shadow control:
I'm not sure I understand why a longer list would be a problem.
I can't give a logical reason for this 😅 Maybe I prefer popover content to be as compact as possible. That said, the background image popover is tall, so being tall in itself may not be a problem:
I think we can go ahead with either approach, the current one using a grid layout, or the vertical approach.
| <HStack align="flex-end"> | ||
| <UnitControl | ||
| label={ label } | ||
| __next40pxDefaultSize | ||
| value={ value } | ||
| onChange={ onValueChange } | ||
| className="edit-site-global-styles__shadow-editor-control" | ||
| /> | ||
| </HStack> |
There was a problem hiding this comment.
| <HStack align="flex-end"> | |
| <UnitControl | |
| label={ label } | |
| __next40pxDefaultSize | |
| value={ value } | |
| onChange={ onValueChange } | |
| className="edit-site-global-styles__shadow-editor-control" | |
| /> | |
| </HStack> | |
| <UnitControl | |
| label={ label } | |
| __next40pxDefaultSize | |
| value={ value } | |
| onChange={ onValueChange } | |
| /> |
- I think the
HStackcomponent is no longer necessary. - The class name doesn't seem to be used.
Yes I totally agree that range inputs by their own nature need some large horizontal space to be used effectively.
Good point, I think this also emerged in nother issues e.g. block margin where a range is used but the value set by users may need to be greater than the range limit. Range inputs usage should be reserver only to values that have a fixed min and max values, because... well it's a range. I think range inputs usage should be audited across the entire UI but that's something for a new issue. |
Yes but I do see other controls use the same pattern, moving all these objects with lots of values to a separate variable and file. I think it greatly helps readability. See the Spacing sizes control and the Box control (all, axial, etc.). |
|
Last commit removes the unnecessary HStack and className. |
afercia
force-pushed
the
fix/shadow-input-control-labeling
branch
from
f688d8d to
cff1bad
Compare
|
Seems good to me: 
|
Thanks. Any objection to get a final review and hopefully approval to merge? |
|
Thanks @t-hamano good catch. |
Fixes #63899
What?
The sliders (Range controls) in the popover to edit the global styles Shadows are unlabeled.
Also, the design is inconsistent: the toggle button to switch to the 'custom value' input is placed after the visual labels. This is inconsistent as in many other similar controls the toggle button is placed after the control.
It appears this has been done only for visual purposes, to lay out the 4 controls in a 2 x 2 grid. Which is arguable because the length of the sliders is very small and setting accurately a specific value may be a difficult task for some users. These sliders need more horizontal room.
Also to mention: the visual labels are actually H2 headings and have a slightly different styling (e.g. the color) compared to the labels used in similar controls.
Why?
All controls must be labeled.
For better usability and accessibility, controls should be used consistently and avoid custom design.
How?
Removes the headings that were used as visusal labels in favor of real label elements.
Removes the 2 x 2 grid layout in favor od displaying the controls full widh in 4 rows.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Before and after: