Fix block lock toolbar item stealing focus when mounted with StrictMode #57185
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
|
Size Change: +753 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Dec 18, 2023
| // We only want to allow this effeect to happen if the modal has been opened. | ||
| // The issue is when we're returning focus from the block lock modal to a toolbar, | ||
| // so it can only happen after a modal has been opened. Without this, the toolbar | ||
| // will steal focus on rerenders. |
There was a problem hiding this comment.
Based on this comment, I'm wondering if this fix will cover all use-cases. What happens if the modal has been opened and closed, could this effect trigger at another moment after the modal was closed (not right after).
There was a problem hiding this comment.
I agree this isn't a great fix. Definitely a hotfix at best. It's better than the current check, since it at least only surfaces in a rarer instance than the current check.
Rather than try to guess if we need to force focus to the toolbar (and where to focus), I think a better solution would be:
If the Lock Button has rendered, force it to stay in the toolbar until it is unmounted as a "lock" action button.
This would mean there would be an "Unlocked" icon until the block toolbar is has been unmounted, but it would improve the UX since the modal could always send focus back to the button that triggered it (and fix this focus stealing issue). Clicking the "Unlocked" icon would trigger the lock modal again. Again, this would only show after unlocking the block. It would not show for all toolbars.
There was a problem hiding this comment.
Agreed — this sounds like a good idea that we should try out separately from this hot fix.
youknowriad
approved these changes
Dec 19, 2023
ciampo
reviewed
Dec 19, 2023
| @@ -33,10 +34,23 @@ export default function BlockLockToolbar( { clientId, wrapperRef } ) { | |||
| useEffect( () => { | |||
| if ( isFirstRender.current ) { | |||
| isFirstRender.current = false; | |||
| hasModalOpened.current = false; | |||
There was a problem hiding this comment.
Since hasModalOpened.current is already initialized as false , is this line necessary?
| // We only want to allow this effeect to happen if the modal has been opened. | ||
| // The issue is when we're returning focus from the block lock modal to a toolbar, | ||
| // so it can only happen after a modal has been opened. Without this, the toolbar | ||
| // will steal focus on rerenders. |
There was a problem hiding this comment.
Agreed — this sounds like a good idea that we should try out separately from this hot fix.
ciampo
reviewed
Dec 19, 2023
Comment on lines
+41
to
+43
| if ( isModalOpen && ! hasModalOpened.current ) { | ||
| hasModalOpened.current = true; | ||
| } |
There was a problem hiding this comment.
(Optional) Putting together a couple of suggestions left above, we could also split the logic into three smaller useEffects — one for the "first render" logic, one for the "has modal ever opened" logic, and one for focus restoration
diff --git a/packages/block-editor/src/components/block-lock/toolbar.js b/packages/block-editor/src/components/block-lock/toolbar.js
index 3385dd1b15..2c69fe3143 100644
--- a/packages/block-editor/src/components/block-lock/toolbar.js
+++ b/packages/block-editor/src/components/block-lock/toolbar.js
@@ -28,42 +28,47 @@ export default function BlockLockToolbar( { clientId, wrapperRef } ) {
const shouldHideBlockLockUI =
! canLock || ( canEdit && canMove && canRemove );
- // Restore focus manually on the first focusable element in the toolbar
- // when the block lock modal is closed and the block is not locked anymore.
- // See https://github.com/WordPress/gutenberg/issues/51447
useEffect( () => {
if ( isFirstRender.current ) {
isFirstRender.current = false;
- hasModalOpened.current = false;
- return;
}
+ }, [] );
+ useEffect( () => {
if ( isModalOpen && ! hasModalOpened.current ) {
hasModalOpened.current = true;
}
+ }, [ isModalOpen ] );
+ // Restore focus manually on the first focusable element in the toolbar
+ // when the block lock modal is closed and the block is not locked anymore.
+ // See https://github.com/WordPress/gutenberg/issues/51447
+ useEffect( () => {
// We only want to allow this effeect to happen if the modal has been opened.
// The issue is when we're returning focus from the block lock modal to a toolbar,
// so it can only happen after a modal has been opened. Without this, the toolbar
// will steal focus on rerenders.
if (
- hasModalOpened.current &&
- ! isModalOpen &&
- shouldHideBlockLockUI
+ isFirstRender.current ||
+ ! hasModalOpened.current ||
+ isModalOpen ||
+ ! shouldHideBlockLockUI
) {
- focus.focusable
- .find( wrapperRef.current, {
- sequential: false,
- } )
- .find(
- ( element ) =>
- element.tagName === 'BUTTON' &&
- element !== lockButtonRef.current
- )
- ?.focus();
+ return;
}
+
+ focus.focusable
+ .find( wrapperRef.current, {
+ sequential: false,
+ } )
+ .find(
+ ( element ) =>
+ element.tagName === 'BUTTON' &&
+ element !== lockButtonRef.current
+ )
+ ?.focus();
// wrapperRef is a reference object and should be stable
- }, [ isModalOpen, shouldHideBlockLockUI, hasModalOpened, wrapperRef ] );
+ }, [ isModalOpen, shouldHideBlockLockUI, wrapperRef ] );
if ( shouldHideBlockLockUI ) {
return null;
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.
Potential fix to #57139. I'd like to use BlockToolbar and
<NavigableToolbar />instead, but haven't looked into if this is possible. This is just a quick patch with the current implementation.What?
Fixes block toolbar stealing focus on mount when using
<StrictMode />due touseEffectrunning twice.Why?
Fixing a bug exposed during
<StrictMode />testing.How?
Adds a condition to only send focus to the toolbar if the modal has been opened while the toolbar is mounted. This fixes the main issue and makes it unlikely to steal focus unexpectedly.
Testing Instructions
<PrivateBlockToolbar />from<BlockToolbarPopover />in a<StrictMode />component (import { StrictMode } from 'react';).Screenshots or screencast