Try position sticky for undocking toolbars

[jasmussen]

This should be seen as a progressive enhancement.

Position sticky allows us to have relatively positioned toolbars right until the content starts scrolling out of view, at which point it starts behaving as position: fixed. This is exactly what we want for very long editor blocks. There's no reason we can't try this and get a feel for it.

Note: because position: sticky; is still experimental, there are some quirks. Like position: fixed;, it doesn't work if a parent container has a transform applied. In addition, it doesn't work if a parent has overflow: hidden;, which incidentally means if we want this we'll need a different approach to full-width images (which I volunteer to fix in that case).

This is not yet fully polished, but please have a look and a test and a feel, and let's discuss.

Video:

https://cloudup.com/che4XEHEKk9

[paulwilde]

One consideration is that browser support isn't that great, so may need a polyfill, or JS fallback solution for Edge. The general idea looks great though.

[ellatrix]

It doesn't need a polyfill I think. If it works, great, if it doesn't, it works as before.

[ellatrix]

What's 56px? Probably good to comment which selectors this belongs to?

[jasmussen]

Yep, was looking for a way to replace that with variables, but couldn't immediately put it together and I had to relocate. Definitely will, though.

[jasmussen]

Noting for myself that I need to adjust some margins for when the toolbar is inside a heading block, as the heading block recently moved to using paddings.

[mtias]

In addition, it doesn't work if a parent has overflow: hidden;, which incidentally means if we want this we'll need a different approach to full-width images (which I volunteer to fix in that case).

Can we consider applying position: sticky only for text blocks?

[jasmussen]

Can we consider applying position: sticky only for text blocks?

We can, but that won't solve the overflow: hidden; problem. That ruleset has to be applied to the editor containing all the blocks, in order to hide the horizontal full-width scrollbar, which means it'll still be a parent container to text blocks.

However I'm confident I can find another full-width solution that works with this.

[ellatrix]

Can we consider applying position: sticky only for text blocks?

It looks also very useful for other blocks like big images and galleries.

[jasmussen]

I pushed a bunch of changes to this branch, which fixes every issue I could find.

This includes moving to using this :not based method for full-width, as opposed to the one that involves vw. This wasn't so much due to my dislike for said unit, but rather because it fixes all the issues and drastically simplifies the CSS at the same time.

It's working really well. Here's a video:

https://cloudup.com/cw53qxfqjLa

That being said, I would imagine that there could be a bunch of side-effects to moving the max-width from the containing element and down into each block. And so aside from a whole bunch of 👀 on this, I will do some testing in various browsers. But right at this chrono-juncture, this feels like the right thing for us to do.

[ellatrix]

This is working great! I wonder if it might be better to add some margin under the toolbar, so that it will unstick sooner and not stick when the block is smaller than that margin.

[ellatrix]

Here's what I mean:

.editor-visual-editor__block-controls {
	position: sticky;
	margin-bottom: $item-spacing + 100px;
	/* ... */
}

.editor-visual-editor__block-controls + div {
	margin-top: -100px;
}

This trick creates a 100px "buffer".

• The toolbar will unstick 100px before reaching the end.
• If the block is less than 100px high, it will never stick. This is great because we probably don't want this for smaller blocks.

100px is just an example, it can probably be a bit more.

[jasmussen]

I wonder if it might be better to add some margin under the toolbar, so that it will unstick sooner and not stick when the block is smaller than that margin.

Ooh I love that! Super clever. Going with 100 for now, we can always tune.

[jasmussen]

Okay, rebased, added your fix, @iseulde, and included a collapsing margins fix. This is solid for me in Chrome. I think we should get it in. Final 👀 ? Thanks.

Actually there's an issue with the collapsing margin fix, I want to find a better one. Give me a sec.

[jasmussen]

Okay I'm actually ready for a final pair of 👀 now. The last commit uses a clearfix to prevent collapsing margins.

To clarify a bit: position sticky requires us to use the top value to position the toolbar relative to the viewport, not relative to the block, and so where previously we'd use abs positioning to take the toolbar out of the flow of the block, we can't do that anymore. So we have to use negative margins to position the toolbars on the edge of the block boundary instead (and no, alas we can't use translate either, as that doesn't work with fixed positioning).

Using margins for positioning the toolbar is fine, though — the math is pretty simple and using a negative margin to push the toolbar up, and a positive margin to compensate for the void means it works. The only problem is when the block itself uses a top margin to position itself inside the block boundary. Embed and Lists do this at the moment (they shouldn't, but we can't prevent 3rd party blocks from doing it), we get _collapsing margins. Here's an ascii drawing:

̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲
|           toolbar          |
‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾
. . toolbar bottom margin . .

. . . block top margin  . . .
̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲̲
|           block            |
‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾

In the above illustration, the blocks top margin collapses, causing a jump when you select the block. The clearfix prevents that.

[aduth]

This is a really cool effect when it works. I'm having a few issues though:

https://cloudup.com/c68mv7eOVnU

• The sticky effect only goes about 2/3 down the block
• The sticky effect only works for the first text block, but not adjacent ones

(These might be the same issue surfaced in two different ways)

It doesn't need a polyfill I think. If it works, great, if it doesn't, it works as before.

Have we verified that this doesn't fail spectacularly in browsers where sticky isn't supported?

[aduth]

Should we be removing this?

[jasmussen]

Removed the layout stylesheet as it's now empty.

The sticky effect only goes about 2/3 down the block
The sticky effect only works for the first text block, but not adjacent ones

Those were both the same issue, and resulting from Ella's tip here: #660 (comment) — basically we are adding a bottom margin to the block, so when you only see n pixels of the block, it stops being fixed position. I reduced this to 50px, and open to removing it entirely, though I do like it.

I also rebased. Good for a final look?

Thanks for the review!

[jasmussen]

Forgot to answer this one:

Have we verified that this doesn't fail spectacularly in browsers where sticky isn't supported?

I've tested this codepen: https://codepen.io/joen/pen/wdqYLO — the behavior in unsupported browsers is quite nice, it simply behaves as though the entire position: sticky; rule isn't there. So it's basically what we have in master now. As such it's a nice progressive enhancement.

[aduth]

I must not have been paying attention when I overlooked the past few comments about having a buffer zone for the toolbar 😄 Personally I'm not sure why we need it. And if my uninformed commentary is any indication, it felt like a bug to me.

[ellatrix]

I don't have a strong opinion on this. I just don't see why it should stick until the end until it can no longer be used, and why it should be enabled on tiny blocks.

[aduth]

Did the toolbar previously end up going past the bottom of the block? I think it'd make sense to have it stop when the toolbar's bottom reaches the block's bottom.

[jasmussen]

I adjusted this bottom buffer zone to be about the same as a single line of text. I think this works well:

I think it's good to have a buffer zone — if we don't have any, it'll stop when the bottom of the toolbar touches the very edge of the block boundary, which also looks buggy. Though I totally see Andrews point that it felt like a bug when it was 100px (even though I also did like that).

Since it's now at about 1 lineheights worth of buffer zone, I think it might be worth merging in and getting a feel for it. If we decide to go lower, we it's easy to edit here: d59848c

Final 👍 👍?

[notnownikki]

I like this 👍