Try improving the side UI for nested blocks

[jasmussen]

This PR leverages the recent block margin refactor to try and improve how nested blocks UI works.

This is a first step, with many possible steps to take in the future, but from a high level it seeks to make immediate improvements to:

• Reducing the visual weight of nested blocks UI, including the dimensions & paddings
• Making the editor appearance look as much the same as the frontend, as possible

It's still very much a work in progress, but even now it has a lot of benefits over master:

• Because the block UI is stacked, the block dimensions are now no longer marred by the excessive margins we used to make the side UI work
• The side UI has a background (experimental) to make it visible even when it covers content

There are still many more todos (I won't list them, some of them will be obvious), but before I go too much farther I would love some thoughts on this implementation as it stands.

If the approach has merit, then there is one additional change we should make in this PR before it's ship ready, and that is to ensure that the parent block is easily selectable. One way to do this could be to always show the gray borders around the parent block, when interacting with a child block. An alternative would be to always show a side-UI type-icon in the vicinity of the parent block, which when clicked selects the parent. This could also serve as a drag handle, now that the side UI is stacking.

Additional ideas for future enhancements could be to dim adjecent blocks when a child block is selected. It would also be nice to look at improving how hovering the parent block works. It can be hard to unhover as it stands.

---

The key point of this PR is to address the side UI and how it affects nested blocks. The approach outlined in this PR was chosen because it was the simplest of them all, and requires the least UI rejiggering to work.

If for whatever reason this approach isn't feasible after all, an alternative approach we've discussed involves surfacing the mobile UI for nested contexts, which would look something like this:

This UI will still definitely be surfaced on phone breakpoints, but the downside it has is that it increases the height of the selected blocks, which presumably can feel jumpy.

---

Here is some demo markup you can use to test the columns block:

<!-- wp:paragraph -->
<p>Loem ipsum dolor sit amet, ferri vidisse nam eu, ad nec copiosae mnesarchum vituperatoribus.</p>
<!-- /wp:paragraph -->

<!-- wp:columns -->
<div class="wp-block-columns has-2-columns">
    <!-- wp:paragraph {"layout":"column-1"} -->
    <p class="layout-column-1">Lorem ipsum dolor sit amet, ferri vidisse nam eu, ad nec copiosae mnesarchum vituperatoribus.</p>
    <!-- /wp:paragraph -->

    <!-- wp:paragraph {"layout":"column-1"} -->
    <p class="layout-column-1">Te brute dicunt sea, ut vis omnium menandri, ut sumo aliquam has. Eum aperiam interpretaris at, sea et recusabo expetenda, omnis tibique mea no.</p>
    <!-- /wp:paragraph -->

    <!-- wp:paragraph {"layout":"column-1"} -->
    <p class="layout-column-1">Pri suas partem ea, ius sonet numquam offendit cu, ad simul admodum pri. Eum cu unum choro albucius.</p>
    <!-- /wp:paragraph -->

    <!-- wp:paragraph {"layout":"column-2"} -->
    <p class="layout-column-2">Lorem ipsum dolor sit amet, ferri vidisse nam eu, ad nec copiosae mnesarchum vituperatoribus.</p>
    <!-- /wp:paragraph -->

    <!-- wp:paragraph {"layout":"column-2"} -->
    <p class="layout-column-2">Eum cu unum choro albucius.</p>
    <!-- /wp:paragraph -->

    <!-- wp:paragraph {"layout":"column-2"} -->
    <p class="layout-column-2">Te brute dicunt sea, ut vis omnium menandri, ut sumo aliquam has. Eum aperiam interpretaris at, sea et recusabo expetenda, omnis tibique mea no. Pri suas partem ea, ius sonet numquam offendit cu, ad simul admodum pri.</p>
    <!-- /wp:paragraph -->
</div>
<!-- /wp:columns -->

<!-- wp:paragraph -->
<p>Sea id autem nominavi deseruisse. Te brute dicunt sea, ut vis omnium menandri, ut sumo aliquam has.</p>
<!-- /wp:paragraph -->

[jasmussen]

As a sidenote, enhancements made to the side UI in this PR overlaps with, and will also benefit, #5400 and #4223.

[jasmussen]

Okay, I've pushed a lot of fixes to this, and and the nested UI (tested on columns block) is very much improved in this branch compared to master. Here's a recent video:

The UI is basically solid, except for one thing. In a943c23, I disabled the ability to hover or even select the parent block (in this case the columns block). Because there is only a tiny sliver of pixels above and below the columns block, the margin essentially, where can hover or select the block. This area will be even smaller, virtually non existant, when we enable margin collapsing. It wasn't working very well at all, so I disabled it pending an explicit action to select the parent block.

Here are mockups for how to enable that to happen. When you've a nested block selected, you see this:

You can then click the "Quote" icon in the editor bar to move up the breadcrumb trail and select the parent block:

Note that in the mockups above, there is no switcher in child blocks, only the parent can be "switched". This can definitely be revisited in the future, but by limiting transformations to the parent block for now we don't have to deal with what happens when you transform a paragraph inside a quote block into a quote (nested quote).

Also, the side UI, while bigger and easier to hit in this branch, has additional improvements coming in #5400.

---

Because this PR is in a fairly good place, and because the code is relatively simple, I would recommend the next step is that someone with a little time on their hands (@mtias? @aduth? @youknowriad ) can help implement a way to select the parent block, perhaps the breadcrumb trail mocked up above. Then we can merge this in, and look at making additional enhancements in future PRs.

My biggest worry is that this PR balloons and becomes too complex.

[aduth]

I'll spend some time iterating at a parent selector.

[jasmussen]

Note that I think @mtias had some ideas he wanted to try too, possibly related to this.

[aduth]

Rebased to resolve conflicts (squashed in the process). Pushed some (rather incomplete) iterations on a hover toolbar to select block or parent block. Will continue tomorrow, but sharing what I have in case you were itching to start hacking away on (much-needed 😄) design improvements.

[jasmussen]

Thank you Andrew, really appreciate that. Pushed some fixes:

This is based on a different mockup for the hover element:

It may be worth pushing this soon, so we can refine separately. Thoughts?

[aduth]

I may revise (simplify) of the implementation if the button is really only meant to navigate one level up to the immediate parent. As-is, if there was double nesting, it would display two buttons, both with "Select parent block" tooltips. Probably not expected, and instead the user, if wanting to navigate upward through to the grandparent block, would select the single "Select parent block" one at a time.

[aduth]

Rebased again to resolve conflicts, and updated implementation. On that end, it feels complete.

I noticed an issue though, which is that with new styling, it's now impossible to add content to the second column in a newly-created columns block.

[aduth]

In fact, if you add a Columns block and immediate deselect it, it's impossible to interact with it at all.

[jasmussen]

Thank you for working on this. I'm having to wrangle a baby right now but can look later, I'm guessing that it's the pointer events handling acting up on empty columns.

[aduth]

Yes, the pointer-events: none has many consequences. One also cannot use block movers or ellipsis menu on a selected columns block, or use the sibling inserter shown in the space between the Columns block and its prior block.

[jasmussen]

Yes, the pointer-events: none has many consequences. One also cannot use block movers or ellipsis menu on a selected columns block, or use the sibling inserter shown in the space between the Columns block and its prior block.

The thing I was trying to accomplish by adding that rule, was to prevent hovering of the _columns_block. You can try commenting it out in https://github.com/WordPress/gutenberg/pull/5658/files#diff-80732b9fe9aee13610c0e0b391943f2aR13

the reason being, the only hoverable area for the parent block is a few pixels above and below the entire thing. Which means if you move the mouse from up before the columns block and onto a child block, is-hovered is applied to the columns block and will only be removed if you carefully hover upwards again and hit those few pixels.

Perhaps a different approach, simply, is to allow this to happen, and then just remove is-hovered from the parent block, whenever a child block is hovered.

This might also allow you to select the parent block by clicking an empty column?

[jasmussen]

In this video, I gave the columns block a red background, and the child blocks green backgrounds. Note how hovering the red area invokes is-hovered on the columns block, and similarly when hovering the green area for child blocks.

The problem is, if you first hover the red area, and then move over the green area, is-hovered is not unset on the red block. Meaning if you move from red, to green, to white, the red highlit block remains hovered, even though the mouse is nowhere near it.

This is what I tried to fix using the pointer-events hack, but I'm going to undo that as it's clearly not ideal. On the other hand, it is desirable to be able to click the red area to select the parent block, when for example the right column is not filled out.

@aduth do you think it would be feasible, and in scope of this PR, to unset is-hovered on the parent block when hovering a child block?

[aduth]

What's the reasoning for the white background in the first place?

[jasmussen]

I actually can't recall. Hmm. @youknowriad?

[jasmussen]

Looks like it was introduced here: 775428e#diff-c2685bc3733aece65a4ed211668982e0R2

So it's probably related to the ability to colorize paragraph backgrounds. But removing it doesn't seem to make a difference. @jorgefilipecosta any thoughts?

[aduth]

Seems we shouldn't want to assume a white background, especially if the plan is to support themes to define their own editor styles.

[jorgefilipecosta]

The reason why we set a white background is related to the contrast verification. If the background was never set getComputedStyles returns back background (if I remember correctly). So adding a background fixes the problem. If themes change the background it also works well because as long as a background is set we are able to retrieve it. The problem happens when no background is set. As we get back background color when background is not set we cannot differentiate and assume is white background because in fact it may be set to black. I will try to research this in more detail maybe there is a way to detect when the background is not set and in that case assume white.
Replicate from comment #5273 (comment).

[aduth]

But this only works if the style is explicitly set, i.e. a theme would need to target and override the .editor-block-list__block:not( .is-multi-selected ) .wp-block-paragraph selector (or selector of equivalent specificity). And the contrast checker would otherwise falsely report contrast issues on the basis of a white background? i.e. if theme actually had black background where a white text would be very contrasting, it would be reported by the editor as an issue.

I'm wondering if it's really worth the trouble to assume a default value (vs. disabling until a background color is known / set).

Alternatively, could we recurse up through parent elements until we find one where background color is not the default value?

[jorgefilipecosta]

I think we can remove the background color, for now, as it is causing us troubles so we can advance.
After, I really like the idea, of recursively iterating up to the parent elements. And I will try to explore it. If we find it not a good solution than we disable contrast checker for colors not explicitly set.

[aduth]

@aduth do you think it would be feasible, and in scope of this PR, to unset is-hovered on the parent block when hovering a child block?

I'll take a look at this. It may be tricky because of the way events are handled / ignored between parent blocks and their children.

[aduth]

This may have been easier with the original hover state which was changed in #5078, since previously it was the case that at most one block could be hovered at a time.

[aduth]

@aduth do you think it would be feasible, and in scope of this PR, to unset is-hovered on the parent block when hovering a child block?

I'll take a look at this. It may be tricky because of the way events are handled / ignored between parent blocks and their children.

This should be working now in 0d0c8e0.

There's an end-to-end test failure that looks legitimate. Investigating now.

[aduth]

Rebased once more. The E2E test failure was due to the fact that we assumed the sibling inserter was positioned after a block, but this was moved in 6203d2c and the test needed to be updated to accommodate (the rebased commit does so).

[aduth]

Hmm, noticing some weirdnesses with the hover behavior for shared nested blocks (notably, it is difficult to access the "More Options" menu for the shared block of a columns block).

[aduth]

Last observed issue is noticeable here and not in master, but not entirely relevant to the changes being proposed. Created a separate fix at #5748.

[aduth]

Rebased to resolve conflicts.

Pushed f67884c to trick the E2E tests into passing by not clicking at the problematic area of the inserter, noting that this is a temporary resolution.

Will do a quick review of the code, but otherwise agree we should get this in soon and iterate.

[aduth]

Was this planned to be addressed here?

[jasmussen]

Ack yes that was intended. All it needs is a proper z index with the mixin. I'm out right now but can look later, alternately feel free to push a fix if you have bandwidth.

[aduth]

There's a '.editor-block-mover': 1 z-index as well. Is that one made redundant with this? Can the controls always have a high z-index, even without the is-selected / is-hovered qualifiers? Are they even present when one of those conditions doesn't apply?

In meantime, I've pushed e1336d3 with reference to z-index list here.

[jasmussen]

I believe that as I worked on this, I decided that yes, the side UI can always be above other things, except in the mobile configuration. So I'd think the old one is redundant now.

[jasmussen]

@aduth was there anything left to fix before merging?

[jasmussen]

🔥🎉❤️❤️

[aduth]

We should remove the redundant z-index.

[aduth]

Noticing an issue: When multi-selecting within columns, it's impossible to click on the "More Options" button:

(Keyboard behaviors appear unaffected)

[jasmussen]

Going to take a look, thanks.