Fix/sticky warning reflow

[tellthemachines]

Description

Removed min-width and set width of sticky warning component to match viewport width. Set warning to display at top of screen on mobile, as changes in #16250 will un-anchor it from the top bar.

How has this been tested?

Tested manually at 100% to 400% zoom levels and different screen widths, on Chrome, Firefox, Safari and IE11.

Screenshots

IE11 at 400%, after changes:

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[kjellr]

This looks good! I have a concern about the width rule for components-notice-list, but I've suggested an update for that. Aside from that, I think this is a solid improvement. Nice work, and thanks for taking this on.

Here are some screenshots taken at 375px wide, at 200% zoom:

Before:

After:

(The "Draft" in there is not a problem from this PR, it's text that bleeds down from the toolbar.)

[kjellr]

This ends up breaking things when the sidebar is active:

I think this can actually be max-width instead of width — that had the same effect in my testing, and prevented that breakage.

That said, I know we typically avoid 100vw for the reasons @jasmussen noted in #16250 (comment). I haven't found this PR to cause any issues, and I haven't found any other alternatives, so it's likely okay.

[tellthemachines]

Done! The use of vw is necessary for now because of minimum widths set further up the tree, but I've opened a ticket on trac to address those, so perhaps we can revisit once that is fixed: https://core.trac.wordpress.org/ticket/47603#ticket

[jasmussen]

Help me understand exactly why the vw unit is necessary, and why we can't use % instead, or even left: 0; right: 0; for absolutely, fixed, or sticky items. As noted in a separate review, the vw unit really only works on mobile, where we never have to worry about scrollbars, or sidebars left and right. Kjell tested in fullscreen mode, but I would imagine things would potentially be even gnarlier if not in fullscreen.

As also noted there, it's very important to test such changes both across all breakpoints and browsers, but also without fullscreen, and most importantly with a bunch of block content. The demo content is good to test, because it includes full-wide blocks, and these are also likely affected by the vw unit.

[tellthemachines]

As I mentioned above, and in this earlier comment, fixed min-widths in pixels are set on body (240px) and on #wpadminbar (300px). At a sufficiently high zoom level, these should cause a horizontal scroll, but because body also has overflow-x: hidden set, the close button on the warning just disappears into the overflow. Using 100%, or left: 0 and right: 0 won't have any effect here as the parent element width is constrained by the fixed widths set above, so the only way of making sure the close button is visible is by setting the width of the warning to be relative to the viewport.
I have created an issue in trac to try and remove those minimum widths, but I suspect it might take a while to get it completely fixed as the widths are tied in with wider issues in the responsiveness of wp-admin layouts.
In the meantime, it's quite safe to use vw as a max-width. The only problem with vw is that it takes scrollbar width into account, so a fixed width: 100vw can cause a horizontal scrollbar to appear. But even that won't happen until we get rid of the overflow-x: hidden on the body 😁

[jasmussen]

Thanks so much for your patience with me! Yes, you're absolutely right now that I look at it:

The only problem with vw is that it takes scrollbar width into account, so a fixed width: 100vw can cause a horizontal scrollbar to appear. But even that won't happen until we get rid of the overflow-x: hidden on the body 😁

That's a big problem, though — because overflow-x: hidden; only hides the scrollbar. It's technically still there — especially on a physical device you can swipe left and right and see the right side of the content cut off.

I really appreciate you creating that trac ticket. The long term solution is to remove those, or refactor them to be able to be removed. Completely correct.

However in the mean time, in order to keep moving, we are looking at writing CSS that's specific for the editor, that overrides those upstream styles. This is no matter how you look at it.

So instead of this:

.components-notice-list {
	max-width: 100vw;
	z-index: z-index(".components-notice-list");
}

you could do this:

.components-notice-list {
	z-index: z-index(".components-notice-list");
}

and then the following:

// Unset upstream min-width styles.
// Pending https://core.trac.wordpress.org/ticket/47603 we have to unset these styles
// in order to allow the sticky warnings to reflow properly in the editor at high zoom levels.
body {
	min-width: 0;
}

@media screen and ( max-width: 782px ) {
	body {
		min-width: 0;
		overflow-x: inherit;
	}
}

A good place to put this would be in this file: https://github.com/WordPress/gutenberg/tree/bd76217166e0d75793ca20b63ad5d4c89119b321/packages/edit-post/src

You can even rewrite it to match the existing mixin we have for resetting wp-admin styles that affect the editor,

gutenberg/packages/edit-post/src/style.scss

Line 60 in bd76217

body.block-editor-page {

. You can see that mixin here:

gutenberg/assets/stylesheets/_mixins.scss

Line 547 in bd76217

@mixin wp-admin-reset( $content-container ) {

By unsetting those styles, you are potentially fixing the scrolling issue not just for the notice list, but for any other similarly behaving elements. I would imagine block toolbars (select a block when mobile and zoomed) also use an absolute positioning that might be affected by the presence of horizontal scrollbars.

[tellthemachines]

Apologies, I'm not sure I explained this very clearly above. What I meant to say is that there is no problem with using vw when setting a max-width. Viewport units are widely supported and implementation across browsers is consistent, so their behaviour is stable and predictable. Thus, we should not be reluctant to use them where necessary.
Setting a max-width of 100vw will not cause a scrollbar to appear. As I was trying to explain above, the only situation where unwanted scrollbars can occur is when setting 100vw as width. width and max-width have quite distinct behaviours, and when using max-width with 100vw, the computed width of the element will never be higher than the width of its parent elements, so no chance of scrollbars.

In this particular situation, setting a max-width of 100vw for the sticky warning will ensure that, no matter what styles are applied to its parent elements, this element will never overflow the viewport. Because this is a sticky component, it is crucial that we ensure users are able to close it, so we need to be extra careful that the close button never disappears.

The problem with avoiding vw and instead unsetting the min-width and overflow-x on the body element is that as soon as someone sets another min-width further up the tree, this component will break again. This is a codebase with a lot of contributors, and even the seasoned ones don't know the reasoning behind every line of code by heart. And we can't hope to be aware of every single line that's added every day. So even though we're all doing our absolute best here, with this sort of issue it pays to take a preventive stance.

The other problem with unsetting the overflow on the body is that as soon as we do that we will have to quickly fix a bunch of components causing horizontal scrolls, not only on the wp-admin side but on the block editor as well. For instance, this is the recent implementation of the columns block placeholder, on a 700px viewport at 250% zoom:

While I agree these problems definitely need to be addressed, they are out of scope for this PR.

[kjellr]

Both of these approaches have some good things going for them:

Using 100vw:

• Gets the job done, and doesn't appear to break anything. I ran this through a ton of use cases, and was unable to find any negative effects — I tried a wide variety of breakpoints across Safari, Chrome, Firefox, and IE.
• Does exactly what it says. This is great from a code readability perspective.
• Still leaves open the ability for us to revise this, pending the outcomes of that Trac ticket.

Meanwhile, @jasmussen's approach:

• Eliminates this problem more globally (though we haven't yet specified any other areas that encounter this issue)
• Provides a single, consolidated place to update if the trac ticket ends up handling this for us.

My recommendation:

Seeing as it doesn't break anything and we don't have any other examples of things that'd benefit from the more global approach, I think we can just move forward with 100vw here. We should add a code comment explaining why it's necessary, pointing back to the trac ticket, so that we can revisit after that is closed.

If however, we do find another concrete example of an area that'd benefit from @jasmussen's more global suggestion, I think we should take that route (but that could also be tackled in a separate PR).

Does that sound reasonable?

[tellthemachines]

Yup, absolutely! The approaches are not mutually exclusive 😄. I'm all for implementing @jasmussen 's suggestion but because it'll involve fixing a bunch of horizontal scrolls that will pop up after we remove the overflow, would rather do it as a separate PR.

[kjellr]

Looks great, thanks for following through on this, @tellthemachines!