Block List: Use default Inserter for sibling insertion

[aduth]

Partially addresses: #10519

This pull request seeks to change the behavior of the sibling inserter to display the same Inserter menu which appears in the header toolbar. This replaces the current behavior which inserts a new empty paragraph block.

Implementation notes:

As implemented, it's not yet a faithful recreation of the cases where the previous sibling inserter would have been shown. This is partly to do with the need to reposition the inserter from the top of the block to the bottom, effectively changing from an "insert before" to an "insert after" action, since the Inserter is currently architected to only fully support the latter.

Notably, the following divergences exist:

• An inserter is not shown before the first block
• An inserter is wrongly shown in block contexts where no blocks can be inserted, e.g. columns (related: Add allowedBlocksMiddleware #7301).
  • This will require some enhancements to the Inserter component to accept not just rootClientId, but index at which to insert.
• An inserter is shown adjacent the selected block
  • I don't even know why this was disabled previously
• An inserter can be shown while isTyping is active
  • I don't even know why this was disabled previously. And, if I understand correctly, it would prevent tabbing to the sibling inserter while typing. Is that meant to be the intended behavior? How would someone reach the sibling inserter while typing if they wanted to reach it?

I'm not sure if it's worth tackling some of these here or separately.

[aduth]

I think this was wrong before; isFirst means first of an entire rootClientId context, not first in a multi-selection.

gutenberg/packages/editor/src/components/block-list/index.js

Line 218 in b022ad2

isFirst={ blockIndex === 0 }

[aduth]

The clientId behavior was totally broken anyways, and unused. Became obvious as I was trying to implement it for the "before first block" by rendering as first child of the BlockList.

[chrisvanpatten]

Off the bat, this is incredible. This feels so intuitive and "right" that I find myself wishing this was how it worked months ago!

I suspect it's just because it's in progress but there are some issues (Safari 12 on Mojave) where multiple inserters get shown at once:

An inserter is shown adjacent the selected block

I don't even know why this was disabled previously

An inserter can be shown while isTyping is active

I don't even know why this was disabled previously. And, if I understand correctly, it would prevent tabbing to the sibling inserter while typing. Is that meant to be the intended behavior? How would someone reach the sibling inserter while typing if they wanted to reach it?

I suspect these are due to the fact that it could cause an inserter to be partially hidden underneath the block's toolbar.

[ZebulanStanphill]

This is definitely an improvement, in my opinion. The tab order feels a lot more natural. Inserting blocks is a lot easier for mouse users, and keyboard users can still use Enter to just insert a Paragraph block and then use the slash inserter.

Also, finally, I can do this:

And this:

Notably, there is one issue:

I see a few solutions:

• Don't show the sibling inserter for the block before the current one.
• Don't show the sibling inserter unless your cursor first moves into the block it is attached to. This means that the sibling inserter does not appear when simply hovering at the top of a block; it only appears after moving into the block above. (I've also suggested that this behavior be implemented for the movement controls as well.)

Also, in addition to the above, and to have as little overlap of the current block's content as possible, I would prefer if the sibling inserter was shifted to appear below the border of the current block, rather than slightly overlapping the block's content like it does now. But that's outside of the scope of this PR.

[chrisvanpatten]

(I don't want to threadsit but just want to say I have been using this branch for a few hours now and I honestly can't imagine life without it. Such a dramatic improvement to the experience. Major, major kudos.)

[aduth]

An issue we'd encounter if we avoid showing the Inserter button between the block and its prior (to avoid toolbar clashing) is to also accommodate Unified Toolbar mode, where such clashing wouldn't occur and the user would expect to be able to use the sibling inserter.

[chrisvanpatten]

@aduth that was previously handled with CSS: https://github.com/WordPress/gutenberg/pull/11018/files#diff-895927eb8330bace9c900c6f7ac881e9L666

Obviously CSS doesn't provide a way to target previous siblings (of course not, that would be too easy) so it's an order of magnitude more complex.

I'm fairly sure @jasmussen wrote that CSS and definitely thought a lot about those interactions, so he might have an idea 💡

[jasmussen]

This is very nice.

But yep, we need a solid way to address the issue where you have a block selected, and the plus button is obscured by the toolbar, as Zeb and Chris mention:

As Chris references, this was hidden using this CSS:

.edit-post-layout:not(.has-fixed-toolbar) .is-selected>.editor-block-list__insertion-point>.editor-block-list__insertion-point-inserter {
    opacity: 0;
    pointer-events: none;
}

What this does, is hide the sibling inserter on the selected block, if the unified toolbar is not selected. It hides it with the opacity, and makes it unclickable using the pointer events rule. Yet it was still tab accessible — though the tab order in this branch is vastly superior to what was in the old version, so that's not an issue anymore.

But perhaps the hiding using pointer events and opacity is still worth looking at?

[aduth]

I'll plan to restore the styles. I was going to gripe about the conflicting positioning, but I see that's already been discussed ad nauseam at #7168 😅

[aduth]

The styles don't work quite as-is, since it's no longer the case that we hide the selected block's own sibling inserter, but rather the previous block (since it's now end-of-block, not start-of-block). I've played with some rough ideas of a pseudo-element to cover the area where the preceding block's sibling inserter would be, but I'm not terribly happy with it (nor with the duplication just to "nullify" the preceding insertion point).

.editor-block-contextual-toolbar + .editor-block-list__block-edit::after {
	content: "";
	position: absolute;
	z-index: 10;
	top: ( -$block-padding * 2 ) - ( $block-spacing / 2 );
	height: $block-padding * 2;
	left: -$block-padding;
	right: -$block-padding;
}

Suggestions welcome, or I'll continue iterating on it tomorrow. I also think this is something which could potentially be avoided altogether if we separately refactor the Inserter to better support explicit insertion points (so that we could keep the sibling inserter where it is today in the markup order).

[jasmussen]

I took a look at various solutions. But becuase there isn't a "previous sibling selector", like you I couldn't really find a way to hide the sibling inserter that was before the selected block.

However your code example in #11018 (comment) seems to work very well for me. I tried giving it a red background to see when it would cover the thing — it seems solid enough.

Even if you don't feel this is a great implementation, I feel like it is a very important issue to solve, and pending separate refactors, we might want to add a comment that this is a necessary hack pending such a refactor. This is depending on which version we need to ship this with, of course, 5.0 or 5.1.

[chrisvanpatten]

It's a very dumb idea but there's no way to do a is-before-selected-block class on the edit wrapper of the previous block by targeting currentBlockIndex - 1?

That said, I think it's worth getting this in 5.0 so if the pseudo element works I'd advocate not thinking too hard about how hacky it might feel and shipping 🚢

[aduth]

It's a very dumb idea but there's no way to do a is-before-selected-block class on the edit wrapper of the previous block by targeting currentBlockIndex - 1?

Not a dumb idea 🙂 It is possible, and perhaps equally viable. The only trade-off I see is in a potential small performance cost in reconciling the DOM when the selection changes, vs. a purely CSS solution.

[aduth]

I've pushed a rebased copy of the branch, which reverts placement of the sibling inserter to the first child of the block. In doing so, I've implemented the necessary behaviors around supporting an explicit insertion point via the Inserter, which were fortunately simpler than I'd anticipated. This means we don't need the hacky pseudo-element and can instead rely on existing styles, and all behaviors previously supported should now be supported.

Testing should include variations on selected block, testing the behavior of the "show insertion point on hover" behavior of the inserter menu items.

[aduth]

One thing I've neglected to note is that our Inserter component is not very optimized, and with it now being rendered between every block, I might expect a performance degradation. My approach here is to see about optimizing the Inserter (related: #11028, more to come).

[aduth]

More inserter optimization proposed at #11243

[youknowriad]

@aduth Code change look good, also tested and it works properly as advertised. The e2e test failure seems legit (probably the test need to be tweaked somehow)

[youknowriad]

Feel free to merge when the tests are resolved.

[aduth]

The e2e test failure seems legit (probably the test need to be tweaked somehow)

I could have sworn I'd updated this test once before. Updated in 57d7de0.

There appears to be a service interruption for Docker at the moment, however, so tests are still failing.

[StaggerLeee]

If you have some free time make animated GIF how it works now. For all of us who don't install developer version. It is a bit "unreadable" by screenshots from issue. Thank you.

[jasmussen]

Screenshot: