-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Data views: Use aria-disabled on disabled checkboxes and add tooltips #59364
Conversation
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. |
Size Change: +441 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} } | ||
/> | ||
<Tooltip | ||
text={ disabled ? __( 'Bulk actions are not applicable' ) : null } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional copy suggestion - feel free to ignore.
text={ disabled ? __( 'Bulk actions are not applicable' ) : null } | |
text={ disabled ? __( 'Bulk actions not available.' ) : null } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe only indirectly. On the face of it, we should be able to pass a child to the tooltip. I tried forwarding the ref in CheckboxControl for fun, didn't seem to make a difference. It's Ariakit.TooltipAnchor that's doing the complaining it seems. It definitely doesn't like @mirka Would you know the best way to approach this? |
I removed the tooltip for now. I don't think it should hold up including this important a11y update in 6.5 (if there's still time 🤞). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the noop
logic to the onChange
callback. This needs to be done manually when using aria-disabled
.
Thank you ❤️ I think that means we can simplify the |
onChange={ () => { | ||
if ( disabled ) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we consider whether this behavior make sense for all usage of CheckboxControl
and whether we should bake it in for any usage of the disabled
prop? (Obviously not a blocker)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably makes sense for aria-disabled
(it's handled automatically when using disabled
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the checkbox is disabled, click or keyboard actions shouldn't trigger an actual change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently when a checkbox is aria-disabled
, click and keyboard interactions will toggle the checkmark (visually at least). I don't think you'd ever want that, so it makes sense to add this to the component some how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was more: automatically use aria-disabled
and prevent onChange
when disabled
is passed. Like we do when __experimentalIsFocusable
is true in buttons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an issue for this at #59411. Ideally we would address it in the CheckboxControl component first, but if we don't have time I'm fine with cleaning up later.
As for the default focusability of a disabled component, I'm kind of leaning toward sticking to the standard browser behavior (which is to not be focusable). Of course we would also provide an opt-in prop to maintain focusability, as in Button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first change is important because disabled checkboxes cannot be focused. For users of assistive technologies this can result in a broken experience as the checkboxes disappear with no explanation.
aria-disabled
means the element can still be focused, but communicates that it's not interaction.
The second change clarifies why checkboxes are not interactive in this context. I'm unsure how helpful this is so would welcome feedback.
We have two issues here, and we should be careful not to conflate them. Disabled widgets can be confusing for anyone, whether they are AT users or not, and we should take care to communicate why they are disabled. So your second point is definitely valid, even if a tooltip is not necessarily the best way to do it.
The other issue is how AT users access disabled widgets in the first place. If widgets are always disabled (for the duration of the user session, at least), and as such do not actually "disappear with no explanation" then using disabled
can be a viable technique. AT generally has a different way of tracking controls than just those that are 'focusable', so this doesn't have to be a blocker.
So my first question is, are we anticipating that the 'disabled' state of a collection item is going to change arbitrarily, such that focus might be lost if a control becomes disabled?
And secondly, what reasons are there for a collection item not having bulk actions? Will there ever be anything other than the current 🔒 - this 𝑥 cannot be edited
?
I don't think so. Eligibility for bulk actions on theme-supplied templates is a state that can change, but it's not something that would happen while a user has focussed the checkbox specifically. They'd first have to select, then perform the bulk action. Or just perform the reset action on that specific record. Perhaps I'm missing a nuance in the question?
Currently they all amount to the same reason – records that represent items in the file system (a theme's patterns, templates, etc.) cannot be edited directly. It's hard to say if there will be other reasons in the future. |
I have some thoughts.
Thanks. |
We can add an ES Lint rule to do that: #59518 |
Any thoughts on whether we should merge this, or wait for #59411? |
I think it's fine to merge. |
Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: andrewhayward <andrewhayward@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: alexstine <alexstine@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org>
I just cherry-picked this PR to the update/packages-6.5-rc1 branch to get it included in the next release: 89cd003 |
Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: andrewhayward <andrewhayward@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: alexstine <alexstine@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org>
What?
aria-disabled
instead ofdisabled
on checkboxes in data views.Add a tooltip to explain why the checkbox is disabled.Removed.Why?
The first change is important because
disabled
checkboxes cannot be focused. For users of assistive technologies this can result in a broken experience as the checkboxes disappear with no explanation.aria-disabled
means the element can still be focused, but communicates that it's not interaction.The second change clarifies why checkboxes are not interactive in this context. I'm unsure how helpful this is so would welcome feedback.
How?
Use
aria-disabled
instead ofdisabled
on checkboxes in data views. Wrap checkboxes in aTooltip
instance.Testing Instructions