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

List View: Account for text fields in shortcut handler #61583

Merged
merged 2 commits into from
May 13, 2024
Merged

List View: Account for text fields in shortcut handler #61583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cf4504
List View: Account for text fields in shortcut handler
Mamaduka May 10, 2024
fc44acd
Update and fix e2e tests
Mamaduka May 11, 2024
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
6 changes: 6 additions & 0 deletions packages/block-editor/src/components/list-view/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { BACKSPACE, DELETE } from '@wordpress/keycodes';
import isShallowEqual from '@wordpress/is-shallow-equal';
import { __unstableUseShortcutEventMatch as useShortcutEventMatch } from '@wordpress/keyboard-shortcuts';
import { speak } from '@wordpress/a11y';
import { isTextField } from '@wordpress/dom';

/**
* Internal dependencies
Expand Down Expand Up @@ -181,6 +182,11 @@ function ListViewBlock( {
return;
}

// Retain the default behavior for text fields.
if ( isTextField( event.target ) ) {
return;
}
Comment on lines +185 to +188
Copy link
Member Author

Choose a reason for hiding this comment

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

@mcsf, @andrewserong, what do you think about this solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind it, but for completeness' sake this is what my local branch looks like:

diff --git a/packages/block-editor/src/components/block-rename/modal.js b/packages/block-editor/src/components/block-rename/modal.js
index f3db0d6c362..0ca42aacad4 100644
--- a/packages/block-editor/src/components/block-rename/modal.js
+++ b/packages/block-editor/src/components/block-rename/modal.js
@@ -67,6 +67,11 @@ export default function BlockRenameModal( {
 			focusOnMount="firstContentElement"
 			aria={ { describedby: descriptionId } }
 			size="small"
+			onKeyDown={ function stopKeyPropagation( event ) {
+				if ( event.key === 'Backspace' || event.key === 'Delete' ) {
+					event.stopPropagation();
+				}
+			} }
 		>
 			<form
 				onSubmit={ ( e ) => {

with the current draft commit message:

Naive fix, just to get the ball rolling.

The rationale here is that the Modal component is the boundary that
corresponds to the user's mental modal of "UI boundary". So intercept
key presses at that boundary, preventing those events from reaching
ListViewBlock (since #61130, where this bug was introduced), where they
will cause the current block to be deleted.

Both approaches probably carry some unknowns, I'll let you follow your intuition. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I also considered fixing the issue locally for the "Rename" modal, but the same issue will affect other modals (e.g., "Create pattern") with text fields.

My thought was that consumers shouldn't care how list view handles shortcuts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me


// If multiple blocks are selected, deselect all blocks when the user
// presses the escape key.
if (
Expand Down
44 changes: 19 additions & 25 deletions test/e2e/specs/editor/various/block-renaming.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,26 +36,10 @@ test.describe( 'Block Renaming', () => {
page,
pageUtils,
} ) => {
// Turn on block list view by default.
await editor.setPreferences( 'core', {
showListViewByDefault: true,
} );
Comment on lines -40 to -42
Copy link
Member Author

Choose a reason for hiding this comment

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

Just setting the preference doesn't open the list view. It needs a page refresh or manual trigger.


const listView = page.getByRole( 'treegrid', {
name: 'Block navigation structure',
} );

await editor.insertBlock( {
name: 'core/group',
attributes: { content: 'First Paragraph' },
} );
await editor.insertBlock( { name: 'core/group' } );
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to just insert a group block instead of converting it.

@getdave, do we need to test a nested block, or is it okay this way?


// Select via keyboard.
await pageUtils.pressKeys( 'primary+a' );

// Convert to a Group block which supports renaming.
await editor.clickBlockOptionsMenuItem( 'Group' );

await editor.clickBlockOptionsMenuItem( 'Rename' );

const renameMenuItem = page.getByRole( 'menuitem', {
Expand Down Expand Up @@ -107,11 +91,18 @@ test.describe( 'Block Renaming', () => {
'false'
);

// Check custom name reflected in List View.
listView.getByRole( 'link', {
name: 'My new name',
await pageUtils.pressKeys( 'access+o' );
const listView = page.getByRole( 'treegrid', {
name: 'Block navigation structure',
} );

await expect(
listView.getByRole( 'link', {
name: 'My new name',
} ),
'should reflect custom name in List View'
).toBeVisible();

await expect.poll( editor.getBlocks ).toMatchObject( [
{
name: 'core/group',
Expand All @@ -123,7 +114,8 @@ test.describe( 'Block Renaming', () => {
},
] );

// Re-trigger the rename dialog.
// Re-trigger the rename dialog from the List View.
await listView.getByRole( 'button', { name: 'Options' } ).click();
await renameMenuItem.click();

// Expect modal input to contain the custom name.
Expand All @@ -142,10 +134,12 @@ test.describe( 'Block Renaming', () => {

await saveButton.click();

// Check the original block name to reflected in List View.
listView.getByRole( 'link', {
name: 'Group',
} );
Comment on lines -145 to -148
Copy link
Member Author

Choose a reason for hiding this comment

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

Just constructing a locator doesn't test anything, so the test never asserted values in the List View. This is fixed now.

await expect(
listView.getByRole( 'link', {
name: 'Group',
} ),
'should reflect original name in List View'
).toBeVisible();

// Expect block to have no custom name (i.e. it should be reset to the original block name).
await expect.poll( editor.getBlocks ).toMatchObject( [
Expand Down
Loading