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

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

Changes from 1 commit
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
Loading