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.

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

Closed
wants to merge 7 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 30, 2019

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.

Before After
before after

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 the WritingFlow 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.

@aduth aduth added [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Package] Block editor /packages/block-editor labels Jul 30, 2019
@youknowriad
Copy link
Contributor

We discussed recently with some folks that we might want to revisit how the "in-between" inserter works. A possibility could be:

  • Hover that area between blocks for 2/3 seconds
  • A big appender is shown (kind of like the one you get when you insert a group block) between these two blocks allowing you to insert a block there.

Do you think that if we implement such behavior, it will have an impact on this PR? Would it simplify or complexify things?

@aduth
Copy link
Member Author

aduth commented Jul 30, 2019

@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 <BlockInsertionPoint />. The main difference would be that it would visually grow after some duration has elapsed?

@youknowriad
Copy link
Contributor

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 . The main difference would be that it would visually grow after some duration has elapsed?

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).

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.

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...

@youknowriad
Copy link
Contributor

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 ) {
Copy link
Member

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.

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'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?

@ellatrix
Copy link
Member

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 } );
Copy link
Member

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?

Copy link
Member Author

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?

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.

@aduth
Copy link
Member Author

aduth commented Jul 31, 2019

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?

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).

@aduth
Copy link
Member Author

aduth commented Jul 31, 2019

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.

Demo: https://codepen.io/aduth/pen/KOmLav

@aduth
Copy link
Member Author

aduth commented Nov 25, 2019

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.

@aduth aduth closed this Nov 25, 2019
@aduth aduth deleted the update/insertion-point-select branch November 25, 2019 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Package] Block editor /packages/block-editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants