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.

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

Prevent exiting Zoom Out mode from stealing focus #60441

Merged
merged 6 commits into from
Apr 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion packages/block-editor/src/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,9 @@ function BlockListBlockProvider( props ) {
__unstableHasActiveBlockOverlayActive( clientId ) &&
! isDragging(),
initialPosition:
_isSelected && __unstableGetEditorMode() === 'edit'
_isSelected &&
( __unstableGetEditorMode() === 'edit' ||
__unstableGetEditorMode() === 'zoom-out' ) // Don't recalculate the initialPosition when toggling in/out of zoom-out mode
? getSelectedBlocksInitialCaretPosition()
: undefined,
isHighlighted: isBlockHighlighted( clientId ),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,16 @@ import { store as blockEditorStore } from '../../../store';
*/
export function useFocusFirstElement( { clientId, initialPosition } ) {
const ref = useRef();
const { isBlockSelected, isMultiSelecting } = useSelect( blockEditorStore );
const { isBlockSelected, isMultiSelecting, __unstableGetEditorMode } =
useSelect( blockEditorStore );

useEffect( () => {
// Check if the block is still selected at the time this effect runs.
if ( ! isBlockSelected( clientId ) || isMultiSelecting() ) {
if (
! isBlockSelected( clientId ) ||
isMultiSelecting() ||
__unstableGetEditorMode() === 'zoom-out'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we sill need these changes? In my testing it works fine without them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, yes. Maybe there's a better way though. If you remove that line, it breaks when:

  • Place cursor in a paragraph (or some block you can type in)
  • Click zoom out button
  • Focus is not on zoom out button

) {
return;
}

Expand Down Expand Up @@ -80,7 +85,6 @@ export function useFocusFirstElement( { clientId, initialPosition } ) {
return;
}
}

placeCaretAtHorizontalEdge( target, isReverse );
}, [ initialPosition, clientId ] );

Expand Down
64 changes: 64 additions & 0 deletions test/e2e/specs/site-editor/zoom-out.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/**
* WordPress dependencies
*/
const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' );

test.describe( 'Zoom Out', () => {
test.beforeAll( async ( { requestUtils } ) => {
await requestUtils.activateTheme( 'emptytheme' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scruffian We can leave the theme activation/deactvation part in the beforeAll/afterAll.

} );

test.beforeEach( async ( { admin, page, editor } ) => {
await admin.visitAdminPage( 'admin.php', 'page=gutenberg-experiments' );

const zoomedOutCheckbox = page.getByLabel(
'Test a new zoomed out view on'
);

await zoomedOutCheckbox.setChecked( true );
await expect( zoomedOutCheckbox ).toBeChecked();
await page.getByRole( 'button', { name: 'Save Changes' } ).click();

// Select a template part with a few blocks.
await admin.visitSiteEditor( {
postId: 'emptytheme//header',
postType: 'wp_template_part',
} );
await editor.canvas.locator( 'body' ).click();
} );

test.afterEach( async ( { admin, page } ) => {
await admin.visitAdminPage( 'admin.php', 'page=gutenberg-experiments' );
const zoomedOutCheckbox = page.getByLabel(
'Test a new zoomed out view on'
);
await zoomedOutCheckbox.setChecked( false );
await expect( zoomedOutCheckbox ).not.toBeChecked();
await page.getByRole( 'button', { name: 'Save Changes' } ).click();
} );

test.afterAll( async ( { requestUtils } ) => {
await requestUtils.activateTheme( 'twentytwentyone' );
} );

test( 'Zoom-out button should not steal focus when a block is focused', async ( {
page,
editor,
} ) => {
const zoomOutButton = page.getByRole( 'button', {
name: 'Zoom-out View',
exact: true,
} );

// Select a block for this test to surface the potential focus-stealing behavior
await editor.canvas.getByLabel( 'Site title text' ).click();

await zoomOutButton.click();

await expect( zoomOutButton ).toBeFocused();

await page.keyboard.press( 'Enter' );

await expect( zoomOutButton ).toBeFocused();
} );
} );
Loading