Navigate regions landmarks style and position improvements

[afercia]

Description

This PR seeks to improve the focus style of the navigate regions landmarks and the position of the "Open publish panel" region.

• uses outline instead of a CSS pseudo element: this way, when scrolling the page or the sidebar, the focus style is always displayed correctly
• IE 11 doesn't support outline-offset so the pseudo-element method is kept just for this browser using @supports to serve the outline method to modern browsers
• changes the animation editor_region_focus to animate just the outline-width: seems Safari doesn't like the animation on the outline shorthand
• changes position of the "Open publish panel" region to use a fixed position, so it doesn't cover part of the content and it's always visible also in Firefox; also adds some right and bottom space
• avoids to use :focus-within, not supported by IE11 and Edge

Notes:

• since the focus style uses now a CSS animation that is set directly on the section container, there was a conflict with the sliding animation of the publish panel; I've found a workaround but I'd rather keep it simple and just remove the focus style animation on the outline /Cc @jasmussen
• IE 11 will still see the focus style scroll together with the section... I'm not sure there's a way to avoid this but honestly I'm not too worried about IE11
• in the responsive view, or when the sidebar has some long content, the "Open publish panel" region will appear on top of other content: this is a pre-existing issue, wondering if we should add a white background:

Example with scrolled content before:

And after (same happens in the sidebar);

Fixes #8553

[jasmussen]

Very nice work, this fixes a ton of issues. This seems to fix a bunch of issues and make the whole thing work a bit better. I think we'll get this in without too much trouble.

A few thoughts though:

• I appreciate that you managed to copy over the animation that "expands" the focus rectangle upon focus, a la https://codepen.io/joen/pen/Nvjror. However it seems to expand outwards now, as opposed to inwards. Previously it would feel like an "inner border" on the region focused. This effect is slightly lost now. Can we make it expand inwards?
• It feels a bit weird to me that the primary keyboard shortcut for navigating regions goes counterclockwise, and you have to add "shift" to the combo to make it go clockwise. I defer to you on whether this is intended, but if it's just an arbitrary solution I'd reverse this.
• Is the skip link to the publish box new? Is it strictly necessary given that it does the same as clicking the "Publish..." button? Would a keyboard shortcut to the publish button be better?
• If the skip link to the publish box is new, it feels slightly roundabout that I first have to navigate to the region, then press tab, then press space or enter to invoke. It seems like as soon as I focus the publish skiplink region, I should be able to press space or enter to go there.

Some thoughts to digest, hope it makes sense.

[afercia]

@jasmussen thanks for your feedback.

Animation expand inwards: eagle eye! Yep I think it can be done animating also the outline-offset property, see latest commit. However, I'd rather consider to remove the animation. To make it work, I had to hack a bit the other animation slide_in_right applied on the same element. This introduces the usage of the selector .is-focusing-regions in the layout component which is extraneous to the logic in navigate-regions and shouldn't be there. Although the animation is nice to have, maybe having clean, isolated, component it's preferable. I'd defer to you and the team.

Counterclockwise: it works as it always worked, it just focuses the regions in the DOM order. Visually, it's a bit different but that depends on the slightly different visual order and DOM order. What is maybe a bit confusing now is that the new "Publish panel" region is the third one and maybe it should be the last one. Not sure it can be moved though /Cc @jorgefilipecosta

Skip link to the publish box: this is part of the work done in #7552 and discussed during WCEU. As you know, everything must be within a landmark region. The Publish panel wasn't. Using landmark regions is like establishing a contract with users and inform them there are n landmarks they can use. The problem with the publish panel is that it's not always rendered so there was the risk to have an "empty" landmark region. That would basically mean breaking the contract with users. So we've opted to put a "jump button" within the landmark region. When pressed, it's replaced by the real publish panel. This was, there's always something inside the landmark. It may seem complicated but basically:

• we need to have the landmark region rendered initially, so users are informed it exists (screen readers automatically lists landmarks)
• we need the landmark region to be non-empty

GIven these two conditions are respected, I'm open to suggestions 🙂

That said, all these points are a bit out of the scope of this PR, happy to discuss them in separate issues if you feel strongly about them.

Note: the build is failing for unrelated reasons (unrelated e2e tests failing?)

[jasmussen]

Animation expand inwards: eagle eye! Yep I think it can be done animating also the outline-offset property, see latest commit.

Without diving deep into the code, I can't fully grok why it's difficult. Was it needlessly complex in the previous implementation also?

I do think the animation is important. It's okay to make it faster, like .1s instead of .2s, but the presence of the animation makes the transition from region to region less jarring, and feels more intentional. It also ambiguates the type of focus, which I feel makes sense given it's landmark navigation as compared to regular tabbing.

Re: counterclockwise, check.

As you know, everything must be within a landmark region. The Publish panel wasn't.

Right. Just wondering if we can use the Publish button itself as a pseudo-skip-link instead of adding an additional publish button that does the same. In other words, could we have the publish button live inside a nested region of its own? I don't know which is more mental overhead, but there's something about this implementation that seems less than ideal. I don't know which is more mental overhead, but if this is the best implementation, no objections.

As for further suggestions and separate PRs, I'm personally always a fan of iterative fixes. I'm sure better solutions will present themselves in the mean time, and I'm okay with intermediate fixes.

About the tests, try pushing again, there were some docker issues recently.

[jasmussen]

Crazy idea, shoot it down if it's too crazy: what if the publish dialog simply popped into view when it was navigated to? I.e. you'd navigate from initial focus to...

1. Header
2. Editing canvas
3. Sidebar
4. ✨ POP! ✨ publish dialog slides in (⊃｡•́‿•̀｡)⊃━☆ﾟ.*･｡ﾟ Publish dialog
5. ✨ POOF! ✨ publish dialog slides out Header again

etc.

[afercia]

The animation should be OK after the latest commit. Haven't touched its speed, it's region_focus( .1s ) as before. It's not difficult but it forced me to use a CSS selector in a file that shouldn't have knowledge of it. If this is OK for @aduth and @youknowriad then I have no objections 🙂

Just wondering if we can use the Publish button itself as a pseudo-skip-link instead of adding an additional publish button that does the same. In other words, could we have the publish button live inside a nested region of its own?

Hm ideally, nested landmarks should be avoided but that's not really the point. Please consider these regions will be used by both sighted keyboard users and screen reader users. Screen readers offers tools to jump through landmarks independently from the keyboard shortcut provided by Gutenberg:

When users land on the page we need to establish a contract with them and inform them there are 4 landmarks (in addition to the WordPress ones):

1. role="region" aria-label="Editor top bar"
2. role="region" aria-label="Editor content"
3. role="region" aria-label="Editor publish"
4. role="region" aria-label="Editor settings"

These regions must always be there and they can't be empty. Aside: the "Editor settings" sidebar, which is dismissible, is an unsolved problem.

So, once we have these 4 regions, what kind of content should be inside of them? Specifically, the publish panel is not rendered by default. In this default state, we could put inside the region the existing Publish button as you suggest. But then, the publish panel should be rendered there, in the same exact spot in the DOM, which means inside the top toolbar. I can easily imagine a lot of layout and CSS problems.

So a better option is to place the "Editor publish" region separately in its own spot. At this point we still have the problem of what content should be there when the publish panel is not rendered.

The POP / POOF option may seem to solve the issue at first sight. As in:

• always render the publish panel within its own region
• by default the publish panel is out of view
• pressing Ctrl+backtick and landing on the region, the publish panel slides in
• pressing Ctrl+backtick and landing on another region, the publish panel slides out

However, there's a problem:

• when the publish panel is rendered but out of view, all the focusable elements within can still be navigated with the keyboard
• so, when the publish panel is out of view we should hide its content to avoid users can tab to multiple elements that are actually not visible
• if we hide the content, then the region is basically "empty" and we are again at the starting point: a landmark region without content

Not saying the current implementation is ideal, I see your points, but I think it's the best we can do so far.

[afercia]

Thinking at a different approach, why the publish panel is separated from the sidebar in the first place? What if the sidebar could be a sort of "dynamic sidebar" with content that changes depending on the flow context?

After all, this is already what happens when plugins add their own sidebars:

Ignore the fact the sidebar gets a different name ("Editor plugins region"). Having just 3 regions would be a nice simplification. Is it possible, from a technical and design perspective, to place the publish panel as fill of the sidebar?

This way, the sidebar region would always have some content (except when it's collapsed by users, but that would be the result of an intentional action). Extending the concept of "dynamic sidebar" to the publish flow could be interesting to explore. At that point, the various buttons would just invoke their own sidebar content (settings, plugin, publish panel, etc.). Thoughts?

[afercia]

Rebased on current master. Let's see if the builds pass.

[jasmussen]

This is working well for me. I'm fine with this shipping.

I think it's important we fix #8616 next, and I still think it would be good to think about how to avoid the duplicate publish button.

[afercia]

Worth also noting the publishing flow is going to see many changes in #7602.

[afercia]

Rebased. /Cc @jasmussen. See #9014.

[afercia]

Rebased. Worth noting the sidebar header sticky positioning and z-index added in #10062 broke the styling on the top part of the sidebar:

[jasmussen]

Sorry about the breakage. How can I help here? I feel the sticky header solves a problem, but if it introduces unintentional side effects,of the it can be revisited.

[afercia]

Not sure. Is the z-index really needed? Removing it could help. But see also #10062 (comment)

[jasmussen]

The z-index is needed, otherwise this happens:

But in any case, I'll take a look.

[jasmussen]

I've tried, and I can't think of a good simple way to fix the focus issue in this situation. If we can't think of anything better for the sidebar, okay to remove it.

[gziolo]

@afercia - what's the status of this one? Do you plan to bring it up to date to include it in the upcoming 5.1 release if it gets reviewed? For the time being, I'm marking it as Stale to make triaging and reviewing easier.

[afercia]

@gziolo I've already rebased 4 times 🙂going for the 5th. Will also try to see how to address the focus style regression introduced in #10062.

[pbiron]

I was more concerned on the negative z-indexes

Honestly speaking, I have no idea if that has any implications. z-index are ticky. We should revisit them at some point separately.

Has the question of the z-index been revisited?

I ask because I'm dealing with a legacy (i.e., geared toward Classic Editor) plugin that uses jQuery UI datepickers in a side metabox and the z-index is causing those datepickers to not be visible.

See this discussion in slack for the details of of why the z-index is causing problems for jQuery UI datepicker.

I'd note that with WP 5.2.1 (with and without GB 5.7,0 active) if I set the z-index on .edit-post-sidebar > .components-panel to auto that I'm not seeing the problem described in #8554 (comment) ...which, if I'm reading the history correctly, is why the z-index was introduced. Perhaps other CSS changes between the time this PR was merged and now mean that the z-index is no longer necessary.

I realize that supporting such legacy plugins that use metaboxes is not high on the GB priority list...but if that z-index is not actually necessary, it is possible that it might be removed?

[afercia]

if I set the z-index on .edit-post-sidebar > .components-panel to auto

Seems to me the negative z-indexes are still necessary. Setting the panel's one to auto causes the panel to overlap the header when scrolling, see screenshot on the right:

Quoting from a previous comment:

I can't think of alternative ways to make the focus style visible and keep the sidebar header sticky other than refactoring the scrolling mechanism of the sidebar. Worth reminding the whole sidebar is scrollable, while its header is sticky. This seems to me not strictly necessary. It could work like the top toolbar:

• making the header static
• making only the panels area scrollable and positioned with a proper top offset

[pbiron]

Seems to me the negative z-indexes are still necessary. Setting the panel's one to auto causes the panel to overlap the header when scrolling, see screenshot on the right:

thanx for pointing that out...I hand't noticed it.

I guess for the time being I'll just live with that overlap so that the plugin is (mostly) usable with GB. thanx for looking into it.