Notices: Make non-dismissible notices push content down

[noisysocks]

Attempts to fix #12243.

Prevents non-dismissible notices from obscuring the post content by separating them from dismissible notices and placing them outside of edit-post-layout__content.

Testing

1. Create a new post
2. Create some non-dismissible notices by running this in your console: [ ...Array( 3 ) ].forEach( () => wp.data.dispatch( 'core/notices' ).createInfoNotice( 'Not dismissible', { isDismissible: false } ) )
3. See that they are push the editor's content down
4. For good measure, create some dismissible notices by running this in your console: [ ...Array( 3 ) ].forEach( () => wp.data.dispatch( 'core/notices' ).createInfoNotice( 'Dismissible' ) )

[noisysocks]

This nearly works. There's one issue which I haven't addressed yet. If you create a few non-dismissible notices and then scroll while your cursor is positioned over the WP Admin sidebar, the notices don't appear fixed to the top like they should.

I could use some CSS help 🙂 cc. @jasmussen

[jasmussen]

Nice work here. After thinking long and hard about the specific approach, though, I decided to take it in a slightly different direction. It still pushes content downwards, but it is no longer separate from the post-content container like it was before — that affected the scrollbars as you note, and we should be extremely careful and intentional when changing those because their change effects compound and multiply across iphones, ipads, and desktops.

Here's the new approach. Desktop:

Mobile:

As you can see, the notices literally push the contents down, but they are part of the same content-container. And the regular, dismissible, notices, are now sticky instead of fixed, so they more or less behave as before but with the added benefit that they can stack below non dismissible ones. The good news is that this approach seems to be working, and working pretty well

But, we need to test carefully:

• We need to test this across browsers, and in Edge specifically, which has an iffy technical implmentation of sticky.
• We need to test it on a physical iPhone, or in the Xcode simulator.
• We need to verify that the z-index change I made to the notices does not have any ill effects. I assume the previous z index had a reason for being that high, but in my testing it seemed unnecessary for notices to cover the editor bar.
• How do we like the is-pinned class I added to the non-dismissible notices? And how about the way I added that class? Feel free to refactor.

A couple of nice side-effects from this is that the notices no longer cover content. The fixed position the notices had before actually did cover content, even though it was content at the top of the screen. This was noticable on mobile, specifically. Additionally, the notices now no longer cover scrollbars, when visible — this was a frustration I've been meaning to address forever.

Let me know your thoughts.

[aduth]

We shouldn't be assigning components-notice-list here; and it seems like we shouldn't need to be either. If components-notice-list has its own class it wants to assign while allowing others to be included, the two should be merged from NoticeList itself. I'll see about making the change to your branch.

[aduth]

It existed before, but I don't think it's the NoticeList component itself which should need to receive a z-index, but rather its specific usage in the editor as appearing above other content. We could reflect this by applying (merging) a class name specific to the editor notice's list .editor-editor-notices__notice-list

[jasmussen]

I tried removing the z-index because I agree. But then this happens:

[aduth]

I think it needs to exist, just not targeting NoticeList broadly, because a NoticeList could be rendered anywhere, not necessarily as we have it above content. So the z-index ought to be targeted to the more specific component which needs it (EditorNotices).

Not something which ought to be done here I think.

[aduth]

• We need to verify that the z-index change I made to the notices does not have any ill effects. I assume the previous z index had a reason for being that high, but in my testing it seemed unnecessary for notices to cover the editor bar.

Based on relative numbers, it likely occurs already in master, but there can occasionally be awkward overlap of "higher" content within the editor.

To be clear, I don't think this is any different in this branch than it is in master, so I wouldn't consider it a blocker, but worth considering for potential future justification of increasing the z-index.

[aduth]

• We need to test this across browsers, and in Edge specifically, which has an iffy technical implmentation of sticky.

I'm running a slightly older version of Edge (38 vs. 41; trying to update), but in my testing the dismissible notices just don't stick. Unsure if this could be considered an acceptable degradation for a browser which doesn't properly support position: sticky (also unsure if this is a claim which can be made).

• We need to test it on a physical iPhone, or in the Xcode simulator.

In my testing (XCode iPhone XR) the dismissible notices also don't stick, and there's a gap above them (at least when only dismissible notices exist).

[jasmussen]

Based on relative numbers, it likely occurs already in master, but there can occasionally be awkward overlap of "higher" content within the editor.

This is a good call. But I thin it might be worth looking at those z-indexes in a holistic way, separately, because the issue is also here:

I would wager that as the UI has changed notably around the block interface, some of the rules have piled up a little bit and could use a dedicated cleanup.

[jasmussen]

Thanks so much for testing Andrew, really appreciate it. I'm going to do some additional testing now, as well as hopefully some bugfixing.

I personally see it as fine for the stickiness to degrade in Edge, as it's not critical for it to work well there. We've also already seen fixes happen in Edge during the course of this Gutenberg development, where an issue with the sticky block toolbars was broken on my version of Edge, but when I upgraded, the issue was gone, see #10059. For that reason I'd be worried about optimizing too much for the current version of Edge.

Note that I saw this comment: #12301 (comment) — and I was sure to carefully check out the rebased and force pushed branch to have the most up to date version before I push any fixes. But I'm still seeing two separate notice components, one that's is-pinned. Not sure if that's how it should be or if I read the comment wrong, but in any case I'd appreciate help fixing that.

[jasmussen]

I can confirm that in my version of Edge, the sticky notice scrolls as it should:

I'm on this version:

[jasmussen]

I pushed a fix to address the stickiness on iPhones, and indeed all phones. GIF first:

The thing is that position sticky doesn't work if an ancestor element has an overflow rule. It's a little messy and complex, but this Stack Overflow comment explains it relatively well: https://stackoverflow.com/posts/44929597/revisions

So I pushed this code: b285699#diff-ec5f5c91d0b3295f448977820605e4e4R88. As you can see, we used to have this overflow rule directly for mobile, when ironically that rule should not be for mobile. I suppose it was written by yours truly in perhaps a tired state. But I'm still curious why it worked.

The thing we want to make sure is that on mobile, the html element is the scrolling container, whereas on desktop, the .edit-post-layout__content element is the scrolling container. The former because otherwise if you put your thumb on the editor bar and scrolled, you'd scroll the html container but not the content below, which invokes an interrupting overscroll bounce effect that blocks actual scrolling of the content below until the animation stops.

On desktop we do want the .edit-post-layout__content to scroll, so as to avoid scroll bleed in the inserter and sidebar.

GIF, notice how the scrollbar should span the height on the mobile breakpoint but not the desktop:

The weird thing is that even with the overflow-y rule present, scrolling worked as intended on the phone, so I wonder if something else made this work even without the rule being scoped to being beyond mobile.

[jasmussen]

I think this is ready for a final sanity check and code review.

[aduth]

But I'm still seeing two separate notice components, one that's is-pinned. Not sure if that's how it should be or if I read the comment wrong, but in any case I'd appreciate help fixing that.

Yes, it's still expected there be two notice lists. The link appears to be broken so I'm not sure which comment specifically you're referring to, but if it's #12301 (comment) , the issue wasn't so much that there were two notice lists, or even the classes assigned to them, but more specifically where those classes were being assigned (i.e. only NoticeList should be applying the .components-notice-list class).

[aduth]

Not entirely relevant, but the separation of dismissible vs. non-dismissible got me thinking a bit about how we reverse the order of notices such that newest notices are shown at the top. I'm not totally sold on it in general, and moreso for the non-dismissible notices, as they don't feel as much a "feed" of content where there'd be as much an expectation of new notices to be shown over time.

Not something I think needs to be addressed here.

[aduth]

I'm still seeing the aforementioned issue in iPhone where a gap is shown above notices. I think it's limited to cases where only dismissible notices are present. In iPhone, navigate to a new post, add a title, and immediately publish. Close the publish panel and I see this:

[jasmussen]

I can't reproduce that issue. Even when there's only a dismissible notice, both on short posts with or without metaboxes, and on long posts with and without non dismissible notices.

[karmatosed]

I also can't replicate the gap. However, I recall this being a problem a while ago and it was a plugin conflict. Just wondering if there are more pieces at play here?

[jasmussen]

I wanted to note that the behavior of notices native to Gutenberg, with this PR, I feel is very solid.

One aspect that I missed is that we recently upgraded legacy notices to show up in the editor. In my personal opinion, we should not be doing that.

Upgrading those legacy notices to work in the Gutenberg editor assumes they were written in full knowledge that the Gutenberg editor exists in the first place, which for 99% of instances will not be the case. When we upgrade every legacy PHP notice under the sun, we make it very likely for users to encounter a myriad of notices that are unaware of their new context. They might assume specific layout that the classic editor has. They might assume features that the classic editor has. In the end, a user might end up seeing a confusing and completely inactionable notice, simply because we upgraded it.

Not only that, but given the modernized layout and design of Gutenbeg, notices both look differently and behave differently to every other notice in the admin. Seeing the same Welcome to My Sweet Acme Plugin. Click here to activate. notice on every page (except the customizer), including perhaps a garbled version on the editing screen because we strip out graphics and formatting, will be a jarring and horrible experience.

In my opinion it would be fine to upgrade such a notice if that notice function call explicitly opts into the block editor. This is no different to how metabox registration works.

With Gutenberg we have an opportunity to lay the groundwork for a more holistic approach to notices. Start here and expand the patterns to the rest of the admin. The Customizer does not show notices, because why would it? In the editing context, you should see editing contextual notices. It seems like we're throwing out a massive opportunity for finally improving notices in WordPress, all in the name of supporting something that arguably should only be supported in the editing context, when the notice is actually contextual to the editing context.

Here's the Dashboard:

Here's the editor:

Here's the customizer:

This is not a good experience.

[aduth]

I'm fine to not upgrade notices. I've made such steps in #12423.

To address a few points in particular though:

With Gutenberg we have an opportunity to lay the groundwork for a more holistic approach to notices.

It's an option. Conversely, maybe it shouldn't be on this first phase of an editor to solve all of the WordPress' deficiencies. The opportunity here would be a conscious choice to bring within the scope of Gutenberg's effort to change how notices appear in the admin.

The Customizer does not show notices, because why would it?

A notable difference is that the Customizer doesn't show notices at all. Gutenberg already established a pattern of notices with Publish confirmation.

Upgrading those legacy notices to work in the Gutenberg editor assumes they were written in full knowledge that the Gutenberg editor exists in the first place, which for 99% of instances will not be the case. [...] arguably should only be supported in the editing context, when the notice is actually contextual to the editing context.

Maybe not quite 99%. Ideally the notices were applied in a filter with at least some awareness of them being specific to the editor screen (example). Given the limitations of notices not being a proper API, I'll grant that it certainly is not limited to those instances, since it's far easier to show a notice on every admin screen than in specific instances.

[DrewAPicture]

@youknowriad Please justify why this was punted to 4.7. This has to be fixed for 5.0, and the 4.7 milestone appears to be targeted to 5.0.x.

[youknowriad]

This will be fixed in 5.0 one way or another. There's an alternative here #12423

[mtias]

The introduction of admin notices in the editor is flawed, as @jasmussen mentions, from the perspective they are created with entirely different expectations.

This notice, for example, was designed for a different context and would be a worse experience to just transition it:

Closing in favor of #12440

[aduth]

For what it's worth, there's still value in this pull request irrespective of whether the admin notices compatibility exists, since isDismissible is an option which has and continues to exist for Gutenberg notices.

I don't think it needs the same level of prioritization as addressing the compatible notices, but it could remain open all the same.

[mtias]

Good point.

[desrosj]

I just wanted to express my opinion, with reasoning, of why I feel not upgrading traditional admin notices at this point is the wrong decision.

My first reason against #12440 is that an RC has already been shipped. An RC should be a code-complete version of what we think is the final product, pending only bugs. This is a change in how something is functioning. While it fixes a bug, I don't think it is a change that should be made during the RC phase of the project.

Traditional admin notices have been around for many, many releases and are the well established best practice way for plugins and themes to display notices to their users in the admin. If the decision is made to not display notices in the new editor, it should be accompanied by a detailed dev note far in advance detailing what is changing, why, what the path forward is, and how to use the new method correctly. Developers should be given time to update the notices in their plugins and themes, which, in some cases, they have probably invested a lot of time and effort into.

The admin notices part of WordPress is not a good user experience. But, it is a much larger issue (being explored in Trac-43484) that needs to be tackled holistically.

I am not against not displaying traditional admin notices in the new editor. But, this adds a requirement for plugins and themes to register notices twice if they need to be displayed in the editor and other parts of the admin. Ideally, there should be a function such as wp_register_admin_notice() that would handle both scenarios. But again, this is a larger issue to tackle.

IMO I feel that changes to a long-standing part of Core should be better communicated and accompanied with a clear path forward for the developers utilizing them currently.

[DrewAPicture]

I agree with @desrosj on this one.

While I don't love effectively dropping support for traditional admin notices on a block editor-enabled screen, I do think a revert of #11604 is the best solution at this point in the core release process. Better to not work in the block-editor enabled screens than to kind-of-work and mess with the editing experience.

Kudos to @mtias for making a tough call.

The idea of grandfathering in "legacy" admin notice support – I use the term "legacy" loosely considering these notices will still be used in every other place they used to pre-5.0 – might have been a bit hasty.

This is a great example of a situation where feature project priorities kind of bowled over core ones – intentionally or not – in the sense that WordPress core's primary priorities (imho) have always been to create a great, consistent, even sometimes innovative user experience from release to release.

Existing features worked before, they should continue to work in the places they worked as best we can make them. And if they aren't going to work or don't fit, we need to communicate with users and developers much earlier in the process about what's changing, why, and how to move forward.

The pull to "fix" perceived deficiencies of WordPress by driving innovation in a feature project is strong, though great care should be taken to give historical and future priorities equal weight.

• Why was this feature built in the first place?
• What problems did it solve or introduce?
• What are the upsides or downsides of making it easier or harder to use and extend?
• Who is already leveraging this functionality and how will they be affected?

All good questions we should be asking about any of the core experiences Gutenberg is changing. All good questions we should have answers for before making decisions.

Don't get me wrong, that doesn't mean features can't innovate, it means the innovation needs to be a conversation. It's where transparent decision-making shows its real value, and why it's proven time and again to be key to WordPress' success as an open source project. It's not a democracy, but it is a conversation.

[youknowriad]

Closing this as we went with the revert option.