Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Keep Lock button it in the toolbar until unmounted #57229

Merged
merged 3 commits into from
Jan 4, 2024
Merged

Keep Lock button it in the toolbar until unmounted #57229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89621c4
If the lock button has shown in the toolbar, keep it in the toolbar u…
jeryj Dec 19, 2023
2672ad1
Remove unused prop and switch selectedBlockClientId[0] to equivalent …
jeryj Dec 19, 2023
7123d3a
Add e2e test to verify focus from modal behavior
jeryj Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 15 additions & 47 deletions packages/block-editor/src/components/block-lock/toolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,78 +3,46 @@
*/
import { __ } from '@wordpress/i18n';
import { ToolbarButton, ToolbarGroup } from '@wordpress/components';
import { focus } from '@wordpress/dom';
import { useReducer, useRef, useEffect } from '@wordpress/element';
import { lock } from '@wordpress/icons';
import { lock, unlock } from '@wordpress/icons';

/**
* Internal dependencies
*/
import BlockLockModal from './modal';
import useBlockLock from './use-block-lock';

export default function BlockLockToolbar( { clientId, wrapperRef } ) {
const { canEdit, canMove, canRemove, canLock } = useBlockLock( clientId );
export default function BlockLockToolbar( { clientId } ) {
const { canLock, isLocked } = useBlockLock( clientId );

const [ isModalOpen, toggleModal ] = useReducer(
( isActive ) => ! isActive,
false
);

const lockButtonRef = useRef( null );
const isFirstRender = useRef( true );
const hasModalOpened = useRef( false );
const hasLockButtonShown = useRef( false );

const shouldHideBlockLockUI =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a use-case where folks don't want people to be able to unlock and I believe there's a setting for that, so we need to make sure that this setting is still supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior on trunk is to not show the Lock Icon at all if canLockBlocks is false. So, the block is locked, but the user has no indicator. I'll match the behavior on trunk for now, then follow up with another PR that displays the lock icon without it opening a modal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the idea on showing a disabled lock icon for locked blocks when the user cannot edit it: #57274

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeryj, that's how I initially implemented the toolbar button but then reverted the change based on suggestion - #39566 (comment).

! 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
// If the block lock button has been shown, we don't want to remove it
// from the toolbar until the toolbar is rendered again without it.
// Removing it beforehand can cause focus loss issues, such as when
// unlocking the block from the modal. It needs to return focus from
// whence it came, and to do that, we need to leave the button in the toolbar.
useEffect( () => {
if ( isFirstRender.current ) {
isFirstRender.current = false;
return;
}

if ( isModalOpen && ! hasModalOpened.current ) {
hasModalOpened.current = true;
}

// We only want to allow this effect 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
) {
focus.focusable
.find( wrapperRef.current, {
sequential: false,
} )
.find(
( element ) =>
element.tagName === 'BUTTON' &&
element !== lockButtonRef.current
)
?.focus();
if ( isLocked ) {
hasLockButtonShown.current = true;
}
// wrapperRef is a reference object and should be stable
}, [ isModalOpen, shouldHideBlockLockUI, wrapperRef ] );
}, [ isLocked ] );

if ( shouldHideBlockLockUI ) {
if ( ! canLock || ( ! isLocked && ! hasLockButtonShown.current ) ) {
return null;
}

return (
<>
<ToolbarGroup className="block-editor-block-lock-toolbar">
<ToolbarButton
ref={ lockButtonRef }
icon={ lock }
label={ __( 'Unlock' ) }
icon={ isLocked ? lock : unlock }
label={ isLocked ? __( 'Unlock' ) : __( 'Lock' ) }
onClick={ toggleModal }
aria-expanded={ isModalOpen }
aria-haspopup="dialog"
Expand Down
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/block-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ export function PrivateBlockToolbar( {
<BlockSwitcher clientIds={ blockClientIds } />
{ ! isMultiToolbar && (
<BlockLockToolbar
clientId={ blockClientIds[ 0 ] }
wrapperRef={ toolbarWrapperRef }
clientId={ blockClientId }
/>
) }
<BlockMover
Expand Down
6 changes: 6 additions & 0 deletions test/e2e/specs/editor/various/block-locking.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ test.describe( 'Block Locking', () => {
await page.click( 'role=checkbox[name="Lock all"]' );
await page.click( 'role=button[name="Apply"]' );

await expect(
page
.getByRole( 'toolbar', { name: 'Block tools' } )
.getByRole( 'button', { name: 'Lock' } )
).toBeFocused();

expect( await editor.getEditedPostContent() )
.toBe( `<!-- wp:paragraph {"lock":{"move":false,"remove":false}} -->
<p>Some paragraph</p>
Expand Down
Loading