[Writing Flow]: Try to fix multi-selection with shift+click
#40687
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Priority] High
ntsekouras
requested review from
Mamaduka,
youknowriad,
mtias,
talldan,
gziolo,
ndiego,
jorgefilipecosta,
fabiankaegy,
annezazu and
priethor
ntsekouras
commented
Apr 28, 2022
| const { | ||
| isSelectionEnabled, | ||
| getBlockParents, |
There was a problem hiding this comment.
These changes are unrelated - just some cleaning up.
|
Size Change: +116 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
priethor
approved these changes
Apr 28, 2022
There was a problem hiding this comment.
Thanks for the quick fix, @ntsekouras ❤️
I tested the use cases mentioned in #40636 and the issue is solved. I'm approving from a functional perspective but a technical review would be appreciated.
|
Thanks for testing @priethor! A technical review is required here for sure! 😄 Also in the end of reviews I'll add some e2e tests. |
talldan
approved these changes
Apr 29, 2022
| // If the selection has changed and we had pressed `shift+click`, | ||
| // we need to check if in an element that doesn't support | ||
| // selection has been clicked. | ||
| if ( event.shiftKey ) { |
There was a problem hiding this comment.
I wonder if this code should also check that it was a mouse event. While shiftKey doesn't seem to be defined for selectionchange events, I'm pretty sure it can be for other keyboard events. Event payloads can also be quite inconsistent between browsers.
There was a problem hiding this comment.
I added an extra check about being a mouseup event for now and let's see how this goes 😄
packages/block-editor/src/components/writing-flow/use-selection-observer.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/writing-flow/use-selection-observer.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/writing-flow/use-selection-observer.js
Outdated
Show resolved
Hide resolved
|
This is great, thanks for fixing so quickly @ntsekouras! |
adamziel
pushed a commit
that referenced
this pull request
Apr 29, 2022
* [Writing Flow]: Try to fix multi-selection with `shift+click` * change check with collapsed selection * address feedback * target specific mouse event with shift * add e2e tests
|
I cherry picked this change into |
adamziel
removed
the
Backport to WP 6.7 Beta/RC
mcsf
reviewed
May 4, 2022
Comment on lines
+131
to
+135
| // Handle the case when we have a non-selectable block | ||
| // selected and click another one. | ||
| if ( startClientId !== selectedClientId ) { | ||
| startClientId = selectedClientId; | ||
| } |
There was a problem hiding this comment.
This block could just as well be refactored to:
startClientId = selectedClientId;… as there is no cost nor any side effect to the assignment.
And this brings me to asking the broader question: couldn't we simplify the logic here into something more straightforward, with fewer conditionals, as close as possible to:
if ( isClickShift ) { // This is the only branching that matters
startClientId = getBlockSelectionStart();
endClientId = getBlockClientId( event.target );
}wdyt, @ntsekouras?
30 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes: #40636
From the issue:
Partial selection across blocks is handled mostly with the help of getSelection API. This creates some problems for blocks that have no text selection(ex Spacer) and things get even more complicated when we have blocks that have a Placeholder state, which actually contains text nodes. See @priethor 's comment:
How?
At first I tried adding some changes in use-click-selection, but couldn't make it work well at all, because selection changes in other events after that, and there are even more nuances to that for handling RichText selection first, before
use-selection-observerkicks in.I don't think this solution is perfect and handles all cases, and that's why suggestions and feedback is more than welcome. I have added comments in the code that will be helpful hopefully 😄
Testing Instructions
shift+clickBefore
before.mov
After
after.mov