-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Writing flow: absorb partial multi selection dispatching #47525
Conversation
Size Change: +596 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Flaky tests detected in f19a197. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7220611211
|
25a9b42
to
4a6bf42
Compare
4a6bf42
to
70bc49f
Compare
50f26dc
to
4a1ec8e
Compare
@@ -84,6 +88,16 @@ export default function useDragSelection() { | |||
return; | |||
} | |||
|
|||
// Abort if we are already multi-selecting. | |||
if ( isMultiSelecting() ) { | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the reason we can't continue with the selection after having started a multi selection, right?
Screen.Recording.2023-12-12.at.11.18.11.AM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this :)
It's actually a bit broken in trunk too
e45afef
to
14da1e1
Compare
position: { x: -1, y: 0 }, | ||
// Use force since it's outside the bounding box of the element. | ||
force: true, | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I am merely making things consistent here with the Esc shortcut:
gutenberg/packages/block-editor/src/components/block-tools/index.js
Lines 163 to 170 in 395b18f
// If there is more than one block selected, select the first | |
// block so that focus is directed back to the beginning of the selection. | |
// In effect, to the user this feels like deselecting the multi-selection. | |
if ( clientIds.length > 1 ) { | |
selectBlock( clientIds[ 0 ] ); | |
} else { | |
clearSelectedBlock(); | |
} |
I think this PR introduced a very small regression for the "block select" metric. It's like 8% maybe. Could be acceptable though. |
What?
Currently we dispatch selection start/end from the rich text instances for partial selection. This PR moves this logic to writing flow, which is responsible for partial selection handling.
Why?
Currently each rich text instance adds a global selection change listener. With this change, only two selection change handlers remain: one in writing flow and one for the selected rich text element.
It should improve both load and typing performance. We currently do not measure the
selectionchange
event though. This can happen async when typing fast, but when it happens, it will block the next event.Additionally it makes writing flow and rich text more independent. We shouldn't have writing flow logic inside rich text.
It also fixes this issue:
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast