-
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
Managing multi-block selection with the keyboard #3038
Conversation
editor/utils/dom.js
Outdated
* @param {Boolean} selector The selector to match | ||
* @return {Element} A node if one is found matching the selector, otherwise null | ||
*/ | ||
export function closest( node, selector ) { |
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'm sure this is implemented somewhere else in a node_module that we are probably importing.
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.
Indeed: "element-closest": "2.0.2",
It is used in Editable
. Because it's implemented as a polyfill* on Element, it has to be used the following way:
import 'element-closest';
doFoo( myElement.closest( mySelector ) );
*: not a huge fan of that, but still: we can get rid of this local implementation and its tests.
Codecov Report
@@ Coverage Diff @@
## master #3038 +/- ##
=========================================
- Coverage 31.43% 31.24% -0.2%
=========================================
Files 222 222
Lines 6333 6373 +40
Branches 1125 1136 +11
=========================================
Hits 1991 1991
- Misses 3650 3679 +29
- Partials 692 703 +11
Continue to review full report at Codecov.
|
A few points here
|
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 left some more micro-level comments inline. For the general picture:
This makes WritingFlow a connected component (to redux), which might not be desirable.
I have no qualms with connecting WritingFlow per se, but I do want to make sure whatever design we end up with is sustainable in Gutenberg. One thought is that there is a lot of state-dependent manipulation going on in WritingFlow and perhaps it's a good time to fold that under a dispatchable action to be picked up by one or more handlers in editor/effects
; I am not sure of this either but it could force us to distill the essentials of multi-block selection. Which brings me to the second thing, IMO the biggest blocker right now: this clashes with @iseulde 's #2988, also ongoing.
Both PRs should inform the other so that we can converge on a single pattern that suits mouse- and keyboard-led multi-block selections. Patterns (in DOM probing, state management, isolation, and component flow) that can suit both should emerge. Then, one of the branches should be rebased on top of the other.
(I realize this is easier said than done!)
editor/utils/dom.js
Outdated
const range = liveRange.cloneRange(); | ||
if ( collapseRanges ) { | ||
range.collapse( start ); | ||
} |
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.
To avoid unnecessary clones:
const liveRange = …;
const range = collapseRanges
? liveRange.cloneRange().collapse( start )
: liveRange;
which also better honors the const
nature of the first assignment. That said, I'm not a fan of the name liveRange
(sometimes I use raw
as a prefix, i.e. rawRange
). We could also just use let range
and reassign it if collapseRanges
; I wish JS had LISP-style let
expressions for this.
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.
More importantly: after reviewing here and in onKeyDown
, it is not obvious why we should care about collapsing the range. Also, collapsing defaults to the closing boundary of a range, not the opening. Is this asymmetry relevant depending on whether we're looking at the upper or lower edge?
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.
Yep, I can make sure it only clones when required. The reason for the collapse is when doing a shift selection inside a block, you start to get a non-collapsed selection. The dom
isEdge
method has an early exit for non collapsed ranges, so it immediately fails. I didn't want to actually change the range, but I wanted to test the edges of the range that are relevant (the right edge for down and the left edge for up). Make sense?
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.
As a side note, I would normally call it rawRange
, but I thought that was one of my own particular naming conventions, and not something that another project would prefer :)
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.
Just tried this. The collapse
function returns nothing, so you can't chain it like this. Will reassign.
editor/utils/dom.js
Outdated
* @param {Boolean} selector The selector to match | ||
* @return {Element} A node if one is found matching the selector, otherwise null | ||
*/ | ||
export function closest( node, selector ) { |
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.
Indeed: "element-closest": "2.0.2",
It is used in Editable
. Because it's implemented as a polyfill* on Element, it has to be used the following way:
import 'element-closest';
doFoo( myElement.closest( mySelector ) );
*: not a huge fan of that, but still: we can get rid of this local implementation and its tests.
editor/writing-flow/index.js
Outdated
blocks: getBlockUids( state ), | ||
selectionStart: getMultiSelectedBlocksStartUid( state ), | ||
selectionEnd: getMultiSelectedBlocksEndUid( state ), | ||
multiSelectedBlocks: getMultiSelectedBlocks( state ), |
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.
Since this is only ever used here:
const hasMultiSelection = multiSelectedBlocks.length > 1;
we can be more parsimonious in mapState
:
( state ) => ( {
…,
hasMultiSelection: getMultiSelectedBlocks( state ).length > 1,
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.
Yep, that makes sense.
Done.
So I've looked through #2988 to see what the differences are. This is my current assessment. Let me know if I'm missing what you are talking about
Which particular things do you think are clashing? The vertical edge checks aren't in any way incompatible with this change, and creating a multi-block selection never needs to know about the resultant horizontal position? I don't think the different approaches that both PRs are taken are applicable to the other PR. #2988 adds a vertical position check (which would be required for this PR also) and doesn't have any other relationship with blocks, and this one needs to know about blocks so that it can expand selection based on block ordering. You understand this codebase a lot better than I do, so you're aware of what patterns you are after. But at this stage, I can't see how these two PRs can really learn from each other aside from trying to do it all at once, and I'm not sure what benefit that would give you, given that both PRs can be merged independently (incrementally). Am I making any sense? P.S. Other requests completed. |
Thanks for getting back so quickly, @ephox-mogran. Your assessment looks right.
The phrase may have been a poor choice on my part. I meant two main things: 1) I was afraid work on one side might be happening without knowledge of the other, and vice-versa; 2) a goal to aim for is for the concerned actions (jump between blocks, select text, start multi-block selection, continue MB selection) to be generated in the same place — whether in WritingFlow or nearby if fitting.
That's the sort of case in which both PRs inform the other. :) Sounds like the vertical position check could be added via #2988, then built upon here in order to become block-aware. However, that sort of iteration requires rebasing this branch accordingly. Or have I misinterpreted you?
Examples: avoiding leakage of implementation details into Gutenberg components that shouldn't have to care about selection (or, at least, about DOM selections or events); preferring compositional patterns like
Fair enough. If both PRs are aware of one another and of how one merge could affect the other, that's most of my battle won. :) Consolidating all the logic in a smaller and unsurprising surface is the remainder. |
editor/writing-flow/index.js
Outdated
@@ -20,12 +35,40 @@ class WritingFlow extends Component { | |||
|
|||
this.onKeyDown = this.onKeyDown.bind( this ); | |||
this.bindContainer = this.bindContainer.bind( this ); | |||
this.isLastEditable = this.isLastEditable.bind( this ); | |||
this.isFirstEditable = this.isFirstEditable.bind( this ); |
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.
Binding here is unnecessary, as these methods are always explicitly called as instance methods (this.isLastEditable( args )
) and are never found as standalone function references (e.g. { onSomeEvent: this.isLastEditable }
). In any case, I just pushed a small refactor to eliminate them altogether.
@ephox-mogran: The other PR I wanted to bring attention to is #1965, merged last week. It too deals with multi-block selection, but some of its logic lives in |
Yep. that's what I was mentioning at the top. If we put all the block selection stuff in |
So I guess what I'm trying to say is that if one place does this all, then it has to know about the block DOM nodes. The only place that currently has that information is VisualEditorBlockList. Therefore, it would make sense to move all the navigation code from WritingFlow into it as well (which means that you'll lose it from PostTitle ... but that's a special case anyway). Therefore, WritingFlow really just becomes a glorified div. Is my understanding correct? Is that what you would prefer? |
I believe WritingFlow retains its raison d'être: from a user's perspective, there is no reason to exclude a post's title(*) from this set of benefits we group under the name of flow. Now, that doesn't make the technical hurdle you mention less real:
It doesn't have to stop us, though. Speaking in very broad terms, WritingFlow could be the central hub (dealing with logic and with dispatching actions), and VisualEditorBlockList could be enhanced(**). React is flexible to the point where we could juggle things a little (think It's possible I'm being too optimistic with regards to how much of the conceptual can be transposed into the real, but I believe there would be long-term benefits to this architecture. (*): Why stop at post title? If we can implement this in a way that scales to an hypothetical Gutenberg v2 that can do page layouts, even better. (**): More commonly, with the |
To clarify my own words:
This doesn't mean that keyboard navigation and multi-block selection should work exactly the same if PostTitle is involved, and it doesn't mean that PostTitle has to be supported right now. But, generally speaking, we want a user to be able to jump from title to content, and to select their content, regardless of shape. |
As discussed in a DM with @mcsf , I'll experiment with trying to push all of the arrow key logic from VisualEditorBlockList and WritingFlow into one place (by probably passing down props from WritingFlow). We can then see the difference in a diff. |
So I've created the branch
So in summary, I'm not convinced it's much better at this stage, but I probably didn't also do what you wanted. I think part of the problem is that a decision needs to be made about whether WritingFlow is supposed to stay a generic component with Imagine that everything that is a WritingFlow direct child can be focused via the redux API due to some identifier PostTitle -> Focuses the post title A good way to see what generalising should be done, and can be done is to add new things to writing flow and see how they work. Specifically, add something that isn't just a container with inputs, contenteditables etc. inside. |
From just testing this, I love love love love how performant it feels. Aside from the technical changes which I see are being discussed (as well as some design changes I'd like to do separately), if we can keep the performance where it is in this PR, we'll have something great. Oh by the way, as you select down the page on a long page, if you go below the fold, the page doesn't scroll with the selection. We should probably do that. |
Thanks for the exploration, @ephox-mogran. I haven't been able to spend enough time today to provide a thorough reply, but only a partial one:
Yes, I'm still undecided there.
With regards to PostTitle, it's possible we just promote it to a block (or a specialization thereof).
Even before promoting PostTitle, a simple method would be: // in PostTitle
PostTitle.defaultProps = { uid: PostTitle.UID };
// in some handler in WritingHelper
if ( uid === PostTitle.UID ) {
// treat it differently
This is something that we may want to expose if a block has specific, more advanced needs (which?). Regardless, it's important we devise a method that works by default in most blocks (e.g. with blocks with no conventional fields to write in, a user navigating with the keyboard would enter the block with a single arrow press, and switch to the next block with a second arrow press in the same direction; blocks with a single area would work like Paragraph; what about blocks with multiple text regions?).
Ah, yes, on the same wavelength there. I'll try to have a go at this very soon, I'll keep you posted. |
@mcsf any progress :) ? |
530703d
to
f442373
Compare
Would we want to do that manually in this case or make it happen after a multi-select is detected during rendering? The disadvantage of the latter approach is that it might mean that if people choose to scroll away, if something forces a redraw, then it will jump their scroll back. |
@jasmussen I've implemented scrolling to the end of the multi-selection as you go. We've also noticed that the only reason this feels more performant than the mouse is because the mouse movement actions are throttled by 250ms. We might want to consider lowering that value if the slugglishness of the mouse interactions are discouraging. |
That sounds good! The idea is to emulate native text selection behavior, but in a block editor.
I take personal blame for this — I was the one who floated the idea that as you're just selecting text with the mouse and dragging outside the block, multi select shouldn't start until you'd dragged n pixels into the next block. This seemed to make sense for me at the time, but now that we're looking at optimizing these interactions, it seems like it may be worth revisiting this one. @iseulde I do apologize for making this wrong call. Do you think you have the energy to look at a branch where we try to zero out that 250ms and drop the "you have to be over the next block" idea I had? |
@jasmussen Sure! |
@jasmussen Actually that still seems ready to go here: #2935 |
I just realised that the edge detection code actually needs to know the selection direction, so there's a minor bug where if you have a backwards selection going up, but before hitting the top of your block, press down while still holding shift, it will expand your selection down. We need to look at the selection direction. Investigating. |
Maybe the selection direction bug should be logged as a separate issue. Thoughts? |
Not sure quite how to reprocuce, selection model seems pretty good to me when I just tested. So yeah maybe worth fixing separately. Nice work on the scrolling, btw — we may want to separately look at improving this further, right now it scrolls only to the edge of the block, we might want to add a little visual padding to that, kind of look at how text does it, where it scrolls almost a full page extra, for visibility, when you reach the end of a selection: But that doesn't have to be part of this PR. |
2711eb6
to
21c98cd
Compare
Pushed a fix for the selection direction issue. Rebased from master. |
21c98cd
to
986b6f2
Compare
- collapseRanges - jsdoc and linting - cloning range - removing custom closest implementation - simplifying state knowledge - removing old import - refactor isEditableEdge, trim code - collapseRanges for vertical selections, and storing the caret horizontal pos - fixing linting - scrolling the bottom into view as the selection grows - scroll to the last processed node - use selection direction, not isReverse to identify the selection end to check against
986b6f2
to
80a7e02
Compare
I've re-rebased and squashed the commits in preparation of the merge. |
Nice work, @ephox-mogran. Let's 🚢 🚢 🚢 . |
Works nicely! :) |
Description
Added Shift + arrow keys for multi-block selection
How Has This Been Tested?
Screenshots (jpeg or gifs if applicable):
Types of changes
Introduces a feature to allow shift + up and down to create or expand a multi-selection in WritingFlow. This makes WritingFlow a connected component (to redux), which might not be desirable.
Checklist: