Update the block overlay to rely on useDisabled hook #40649
Conversation
youknowriad
requested review from
Mamaduka,
talldan,
jameskoster and
Addison-Stavlo
youknowriad
requested review from
ajitbohra,
ellatrix and
tellthemachines
as code owners
| focusable.setAttribute( 'disabled', '' ); | ||
| } | ||
| export default function useDisabled( { | ||
| isDisabled: isDisabledProp = false, |
There was a problem hiding this comment.
This flag disables useDisabled. it's weird I know, I don't know if there's a better way to name this.
|
Size Change: -567 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
| @@ -54,10 +57,14 @@ const BLOCK_ANIMATION_THRESHOLD = 200; | |||
| * the ref if one is defined. | |||
| * @param {Object} options Options for internal use only. | |||
| * @param {boolean} options.__unstableIsHtml | |||
| * @param {boolean} options.isDisabled Whether the block should be disabled. | |||
There was a problem hiding this comment.
Another approach here would be a options.hasOverlay flag and bundle the call to useBlockOverlay here. I might switch to that as it avoids a lot of duplication.
|
In terms of the UI/UX this PR seems to behave identically to trunk. Nothing is disabled, but I cannot directly click to select children of template parts, reusable blocks, or navigations. |
|
I was wondering if it inadvertently fixes the browser issue we had in Safari where sometimes the inner block get selected. The disabling is implemented a bit differently (two levels of disabling) so it might help. |
|
It's a bit better, I can't directly select Link blocks (probably because they're inside the disabled Navigation), but the Site Title remains interactive: safari.mp4 |
|
Last night, I started testing this, and the overlay works better (compared to trunk) when there're RichText inner blocks. PR currently breaks Reusable block edit locking. Maybe I should remove it from the trunk and start working to implement it for all blocks. |
|
@Mamaduka and I did chat about this kind of approach a week or so ago, and a concern would be accessibility. Primarily keyboard navigation, but also because it changes the semantics of those elements. In testing, keyboard navigation seems not too bad. Using the down arrow works ok, I guess because as the outer block is focused, the inner blocks become enabled and can be navigated to. Using the up arrow now possibly works differently - it'll focus the outer block because the inner blocks can't be navigated to. I'm less sure whether the semantics would be an issue. Would need more experienced a11y feedback for that. On another matter, there does seem to be some weird glitching happening in the PR with blocks continually unmounting/remounting. I noticed this in the dev tools: Kapture.2022-04-28.at.13.55.57.mp4and also in Safari when you hover over the block toolbar items, the cursor continually changes from a pointer to a hand. And another issue is that navigation links have a slightly greyed-out style when disabled. |
I think that's not really unmouting/remounting but it's probably something to do with the "mutation observer" in useDisabled triggering too often. I'll have to check what's causing it (probably a loop in useDisabled). |
|
I fixed the loop issue, and I think it may have fixed the toolbar issue as well. |
|
@Mamaduka I fixed the reusable block locking, it's actually a start for its support in all blocks. Makes me wonder that an e2e test to check that the locking is working would be good. |
|
Thank you, @youknowriad. I started retesting this branch, and things look great. Can you rebase on top of the current trunk? A few selection fixes were merged, and I just want to test against them as well. I think this also fixed #35079. CleanShot.2022-05-05.at.10.39.05.mp4 |
| */ | ||
| import classnames from 'classnames'; | ||
|
|
||
| export default function BlockContentOverlay( { |
There was a problem hiding this comment.
Should we deprecate this? Directory search return single plugin - https://wpdirectory.net/search/01G29EAZQER3R2XCAGSMB7MRD5
There was a problem hiding this comment.
this was never stable in the first place. So I think it's fine to just remove.
There was a problem hiding this comment.
(The result in wp-directory is a plugin that show up in all Gutenberg API searches :P I think it's bundling our packages)
youknowriad
force-pushed
the
update/block-overlay
branch
from
7bb9963
to
de37f02
Compare
There was a problem hiding this comment.
Thanks for the rebase, @youknowriad 🙇
It is an excellent improvement for the block content overlay and can be merged.
I agree with @talldan that the overlay method would need more experienced a11y feedback if we want to use it for edit locking. At least to make sure content is accessible for screen readers.
P.S. I noticed a minor issue with a Reusable block. When editing is locked, the insertion point is visible sometimes, and you can insert blocks.
Screenshot
|
Thanks for working on improving this! I see two behavior issues that we were originally adamant about solving with the first overlay:
Up to you whether these are blockers to fix first or not. But this is working well in my testing beyond that! |
|
I think there's a regression here that I briefly mentioned earlier:
I'll add some more specific testing instructions, as originally this wasn't very clear.
Expected (In trunk): The paragraph block is selected and focused and the caret is visible in the paragraph block, pressing up again moves focus to the first paragraph block |
Thinking about this, I actually feel the behavior in this branch is probably the one that is more correct. When navigating to a reusable block, it's arguable whether you want to land directly in the inner paragraph. In fact, it's for this exact same reason we have the overlay on click on the reusable block. that arrow navigation behavior is just the reflexion of that overlay for keyboard navigation. |
This should be fixed in 5d7c549 |
mburridge
added
the
Needs Dev Note
|
Added label in case a dev note is needed for 6.1 release. |
closes #40845
Related #32163 (comment)
What?
This PR kind of introduces a "disabled" state/config for a block wrapper. when it's disabled it uses the
useDisabledhook to prevent editing or selecting anything inside it. Right now it's being used to replace the "Block Overlay" feature. (Reusable blocks, navigation blocks, template parts).Why?
We plan to expand the "disabling" behavior to a lot more use-cases:
and this can be seen as the first step towards that.
How?
This is composed of two changes:
Testing Instructions
1- Try navigation, reusable and template part blocks
2- See how the selection behaves (overlay..)