Disable lock button if user cannot control lock state #57274
Conversation
|
|
|
Size Change: +34 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e97c462. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7277770340
|
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| <ToolbarGroup className="block-editor-block-lock-toolbar"> | ||
| <ToolbarButton | ||
| disabled={ ! canLock } |
There was a problem hiding this comment.
@jeryj Is this accessible? Looks like the Button component was changed to make isFocusable possibly true by default?
There was a problem hiding this comment.
Button has not yet been updated. If we want to disable the button while keeping it focussable, we could add the __experimentalIsFocusable prop. Although we should probably be consistent with the rest of the toolbar buttons.
There was a problem hiding this comment.
The main block icon does the same thing - it's disabled and not focussable.
There was a problem hiding this comment.
I think we should. If we're going to show a button visually, it needs to be accessible. I think it's time to change that prop in the Button component to just default true. Could discuss that in an issue though.
There was a problem hiding this comment.
I can access it in VoiceOver using the Shift+control+option+arrows, which announces it as "Unlock, dimmed dialog popup collapsed," but it's not within the arrow keypress roving tabindex. Happy to change it to be a part of the roving tabindex though! Sorry if I got this one wrong! Happy to learn and correct it.
I think we can also use the [accessibleWhenDisabled](https://ariakit.org/reference/toolbar-item#accessiblewhendisabled) prop to leave it in the roving tabindex.
Which route would you recommend?
There was a problem hiding this comment.
@jeryj It's fine, it just goes to show you how Voiceover and Windows accessibility differs greatly. Either way sounds like it should work. I'll let you decide.
There was a problem hiding this comment.
I think adding it to the roving tabindex with accessibleWhenDisabled or __experimentalIsFocusable would be a good way to go. It's too easy to miss the information otherwise.
There was a problem hiding this comment.
__experimentalIsFocusable comes from the Button component from @wordpress/components, accessibleWhenDisabled comes from ariakit — I wouldn't know in advance which approach would work better. Could you try both and see if there are any differences?
There was a problem hiding this comment.
I tested it out. accessibleWhenDisabled works well with the toolbar. Using __experimentalIsFocusable doesn't work out of the box. It puts it at the end of the toolbar roving tabindex order or skips it altogether for some reason.
|
This is what users who cannot lock blocks will see now: |
|
Just to be sure I'm getting this right, in trunk at the moment if a user does not have the ability to move or customize a block, that it's essentially locked, we are not currently showing a lock icon like we do already when blocks are manually locked, and that this PR simply adds that lock button/icon, but in a disabled state. Is that right? If yes, then we do have mainly two ways to visualize this:
Without having had time to test today, it seems like this PR is doing the latter, perhaps it's also doing the former. My instinct would be to start with only the former — no color change, just an inert lock button with the default cursor. Showing at full opacity seems valuable to indicate: yes, this block is locked. The lack of a pointer cursor (we can even show a tooltip if need be) can be enough to explain why you can't make any changes. We can certainly lower the opacity too, as that's usually done with disabled buttons. But just like how we customize the opacity of the disabled "Saved ✔" button we have in the top toolbar, this particular button serves an informative purpose, and could benefit from full contrast. It's an instinct, not strongly held. |
Correct! We're providing indication that the block is locked. I'm fine with making it full contrast with a default cursor for now. I'll make the updates. |
The canLockBlocks setting allows a user to manage the block locked state. Admins always have it set to true, but some users are not allowed to unlock blocks. If they can't unlock a block, there is nothing in the toolbar that suggests a block is locked as there is for admins. I think it's useful to have some kind of indicator that the block is locked, so if a block is locked but a user cannot edit it, show a disabled lock icon in the toolbar.
jeryj
force-pushed
the
update/display-lock-ui
branch
from
e97c462
to
a1c04d2
Compare
|
@jasmussen I have it updated to the high contrast when disabled, but we've also added the
Reducing the opacity I feel is a better iteration than no indicator, so I'll commit that for now. |
This reverts commit a1c04d2.
|
I'd personally love to start here, full opacity. But if we have to reduce the opacity at a later time, it won't be the end of the world. In general, this is an existing example where the disabled button is focusable, but has full 4.6:1 legible small text contrast regardless:
|
I can make I think it would be difficult to figure out you're not allowed to unlock a block if the states look identical: 
|
Follow up to #57229
The
canLockBlockssetting allows a user to manage the block locked state. Admins always have it set to true, but some users are not allowed to unlock blocks. If they can't unlock a block, there is nothing in the toolbar that suggests a block is locked as there is for admins. I think it's useful to have some kind of indicator that the block is locked, so if a block is locked but a user cannot edit it, show a disabled lock icon in the toolbar.What?
Display a disabled lock icon in the toolbar if the toolbar is locked and a user can't control lock state.
Why?
It's helpful UI to know if the block is locked or not, even if you can't control that state.
How?
Check for
canLock, and disable the button if the user cannot control lock state.Testing Instructions
canLockBlockstofalseinpackages/block-editor/src/store/defaults.js.Testing Instructions for Keyboard
Screenshots or screencast