-
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
Block Editor: Interpret adjacent block selection from insertion point #16818
Conversation
Ideally done by selector, but for commitment to backwards compatibility, this is not possible
We discussed recently with some folks that we might want to revisit how the "in-between" inserter works. A possibility could be:
Do you think that if we implement such behavior, it will have an impact on this PR? Would it simplify or complexify things? |
@youknowriad I see that as an alternative take on the problem. I don't see them as strictly incompatible, but personally I'd argue the sort of improvement explored in this pull request largely negates the need for something like what you describe. That alternative take seems to embrace the idea of margins between blocks (dead space), as opposed to what's aimed here to create a seamless editable field, while still retaining the ability to insert between blocks. I guess it depends how that alternative approach would be implemented. For a "hover duration" to be detected, I assume you'd still need some overlay target like what exists today with |
Yes, that's the idea. I didn't give much touch about how to compute that hover duration or whether you need a DOM element to compute it (For example the hover area component doesn't require an extra DOM element to compute the left/right areas).
The main reasons for that "in-between" behavior change is not related to the issues this PR is fixing but more to the fact that the "in-between" inserter have some lacunas: Sometimes it triggers when not needed (it's very clunky in nested contexts), it can overlap with the block toolbar, you can't resize the spacer because of it... |
Anyway what I'm describing has probably drawbacks but the idea being there's still room for improvements about how the in-between inserter works. (It shouldn't prevent us from working on improvements like this PR) |
*/ | ||
export function* selectPreviousBlock( clientId ) { | ||
export function* selectPreviousBlock( clientId, initialPosition = -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.
I'm not super excited to see more use of initialPosition
... Ideally, we should try to get rid of it and directly set the correct selection either in the selection store or the DOM. initialPosition
is already reduced to only be needed for block deletion. I was hoping that soon we'd be able to get rid of it entirely.
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 not super excited to see more use of
initialPosition
... Ideally, we should try to get rid of it and directly set the correct selection either in the selection store or the DOM.initialPosition
is already reduced to only be needed for block deletion. I was hoping that soon we'd be able to get rid of it entirely.
In this model, how would you foresee doing something like we're doing here (and also doing in WritingFlow) where we want to map a specific DOMRect
offset to a selection?
In my opinion, this seems a bit complex for little benefit. Why do we need the in between zone to set the caret in the closest block, if it's easy enough to click there directly? It would be nice if the zone did something, I agree. Since it's always so difficult to find and click on the sibling inserter, would it not make more sense to expand the click target of the inserter? |
</div> | ||
const caretRect = new window.DOMRect( event.clientX, targetRect.top, 0, 28 ); | ||
if ( yPercent > 0.5 ) { | ||
selectBlock( clientId, { caretRect } ); |
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.
Why not call placeCaretAtVerticalEdge
directly and avoid the strange initialPosition
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.
Why not call
placeCaretAtVerticalEdge
directly and avoid the strangeinitialPosition
state?
It could be done, but the difficulty is in finding the input, which likely would require copying one or the other of WritingFlow or BlockListBlock's respective behaviors for finding a tabbable at the extent of the block [1] [2].
Maybe there's another option where we "wait" for the focus to take place from the block selection, and then call placeCaretAtVerticalEdge
from the document.activeElement
.
I see it as subtle, but impactful. Similar to how early (often small) bugs in RichText shattered one's "flow" in the editor, I feel that some of the perceived heaviness / clunkiness of the editor can be attributed to these divisions between blocks being so very obvious. When I'm writing, I want to feel as though I'm in a single continuous text field. The changes in this pull request seek to try to mimic that as much as possible, while still acknowledging and embracing the layout interactions of sibling insertion. Evidenced in what @youknowriad shared as possible explorations in #16818 (comment), I think we've overoptimized for these inserters. It's not that we don't need them, but in the majority of cases we're either just scanning over content or editing the content itself. I think the suggestion of expanding the click target would further move us away from this as the primary workflow. The changes don't make the sibling inserter any easier or harder to use, since the hover area is the same as it was previously (larger, in fact). |
To be clear, I offer this as largely experimental, aiming to demonstrate the behavior moreso than to propose a sound technical approach. As mentioned in the original comment, I think an ideal approach would simply leverage padding on the block fields themselves, something made difficult to achieve in our current implementation of the block list. |
I would still very much like to see this usability improvement come to fruition. However, I don't think the approach here is the best option. I've opened an issue at #18733 to continue discussion on this topic, but will close this in the meantime. |
This pull request seeks to experiment with an approach to reduce "dead zones" between blocks. Currently, it can be difficult to select paragraphs using the mouse due to margins between the blocks. The margins are a result of default spacing between blocks and to provide a space for the sibling inserter to be displayed.
Implementation Notes:
Ideally, this would be handled taking advantage of the browser's default treatment of padding for an input field, where the input field should be considered the paragraph (as if it were extended to the bounds of the block). Unfortunately, this is made prohibitively difficult by the complexities of the block styling in handling margins (and margin collapses), inner blocks, and the need to support activation of the sibling inserter.
Instead, this takes the next best approach: It interprets clicks on the sibling inserter as intent to select the following or preceding block.
This required expanding support for the
selectBlock
initialPosition
argument to support an X-offset of the selection (similar as to what's done in theWritingFlow
component to preserve the horizontal offset).This works fairly well, but is not perfect; notably, if the block is already selected, clicking within the sibling inserter will not reposition the caret. There may be an alternative approach to explore here.
Testing Instructions:
Verify that there is no dead zone between blocks.
Verify there are no regressions of the use of the sibling inserter.