Olivero: Focused level-2 nav items should always be in viewport during keyboard navigation.

Good find. I think we may have noticed this before, but it wasn't captured in an issue.

I replicated it with the demo content at https://lb.cm/olivero. Their are 10 links under "webforms", and you can experience the problem if your viewport is less than approx 750px tall.

An 11-inch Macbook Air from 2017 has 768 CSS pixels height when using the browser full screen, iirc? My old laptop has a 1366x768 screen, so I'll check it on that.

10 links in the submenu is a very reasonable scenario for sites, so this certainly isn't an edge case bug.

Promoting to major/blocker. This is certainly a failure of WCAG SC 2.4.7 Focus Visible at level AA, and I think it can easily be classed as a failure of SC 2.1.1 Keyboard at level A.

Good catch. I've played around in my browser and we can fix this by setting the submenu to something like max-height: calc(100vh - <header height>; overflow: auto;.

Patch coming shortly.

Patch attached.

Hi @mherchel,

patch is working fine and resolving issue but it's removing the menu level arrow.

Please have a look.

Thanks!

This is because of overflow: auto; Can you have look into this and provide better preview>

Thanks!

About the approach in comments 4 and 5:

I think the approach in #4 might work okay for pointer users and keyboard users, but it could still be awkward for other interaction methods. I'm not sure if overflow containers are easy or hard to use with speech control, so I'll ask in some a11y chat rooms.

It doesn't follow the proposed resolution from @bnjmnm. It's still worth trying other approaches. I think it would be preferable to get the page to scroll, rather than create a scrolling container on the sub-nav list.

Expanding on the steps to reproduce:

Note that in some instances, the first out-of-viewport item did get scrolled into focus, but the ones following it did not.

Yes, I replicated this too! The sticky header feature is, er, implicated. It took a few attempts with different browser window sizes, but I managed to make it happen. I've recorded a demo video, and here are the things to watch for...

1. Use the demo content at lb.me/olivero, which has a sub-nav with 9 items.
2. Using Firefox, I arranged the browser window size so it was only tall enough to show 5 sub-nav items.
3. Open a sub-nav, and 5 items are visible. Tab to item 5. Focus is visible near the bottom of the viewport. The viewport hasn't scrolled yet.
4. Press tab. Now viewport shifts just enough to bring item 6 into view, and focus is visible.
5. Press tab. The viewport shifts again, just enough to bring item 7 into view, and focus is visible. Also, the sticky header feature has engaged, and the X-button appears in the top left corner.
6. From here on, pressing tab doesn't cause the viewport to scroll, and focus isn't visible. Focus has moved below the viewport.

The experience may vary in other browsers, because their scrolling behaviour differs when focus crosses the viewport edge. Firefox scrolls the viewport just enough to bring an item into the viewport; other browsers scroll by different amounts.

I think it's worth postponing this until we've implemented the new plan in #3186349-9: Major accessibility problems with Olivero header show/hide feature. The sticky header behaviour is partly to blame for the problem here; specifically, the way the sticky behaviour kicks in without the user asking for it.

Feel free to to keep experimenting here though! We'll still need to make this robust enough for the situation when the user has opted-in to a sticky header.

But let's not commit anything here until the other issue has been addressed. Noting this in the issue summary.

#3186349: Major accessibility problems with Olivero header show/hide feature is now committed. We should be able to actively work on this.

New patch attached. I really like my solution here. Movie included.

Tugboat link: coming soon

Hmmm... this solution actually needs some work. We're missing the little carrot.

Fix to carrot is added (I moved it outside of the container).

Video (coming soon)

Tugboat: https://3190120-menu-focus-syqxciq9xqqmexouh3gwu9elwn8tzxat.tugboat.qa/

Demo video

I just navigated my way through the Tugboat demo -> and confirmed I was able to get through the dropdown menu: Webform using key board only. This is really cool.

- scrolling ✅
- carrot ✅
- tab/enter and arrow keys ✅

1. visit the tugboat link (https://3190120-menu-focus-syqxciq9xqqmexouh3gwu9elwn8tzxat.tugboat.qa/)
2. Make the your browser viewport wide, but short (shorter than the natural height of the level 2 navigation)
3. Use keyboard navigation, and tab down into the level 2 navigation.
4. Ensure that all of the focused navigation items are visible within the viewport

Works for me in Chrome 89 and FF 87:
Chrome: https://www.loom.com/share/711bd4896e6444a1a5219d14cb7ba1bb
FF: https://www.loom.com/share/0827807debf44b3e96d59bbd3ac928fa

I did notice that in FF I could select the whole submenu container (see about 20s into the video) which lets me scroll the submenu with the arrow keys, while Chrome just jumps to the first element (but you can still control the submenu scrolling with arrow keys).

Works for me on the tugboat link above. Tested on Firefox 86. Setting to RTBC

The git conflict was in one of the generated files (core/themes/olivero/js/second-level-navigation.js), so setting this back to RTBC.

Patch updated. The JS was failing because of a variable name change in #3208286: Olivero: "topLevelMenuITem" variable should be "topLevelMenuItem"

Previous test failure appears to be unrelated. Queuing up another test.

@lauriii wants some UX people to give a 👍 on this solution before committing.

For the UX folks who are reviewing this, you can demo a tugboat preview of this at https://3190120-menu-focus-syqxciq9xqqmexouh3gwu9elwn8tzxat.tugboat.qa/

and a quick explanation movie is attached.

I can speak as a member of the Usability team. (This does not mean that I am a UX expert.)

I did a little testing with FireFox/Linux and the patch in #29.

I noticed the same effect that @AndyF (hi!) pointed out in #20: after opening the level-2 menu, the first tab selects the container. This is a little confusing: what is the point of selecting the container?

With the container selected, I can scroll the sub-menu with the cursor keys. Regardless of scrolling, the first tab selects the first menu item and makes it visible.

With the container selected, or with one of the sub-menu items selected, if I hit <Esc>, then the container closes and nothing has focus. (Or the focused element is not visible?) From here, <Tab> moves focus to the search icon and <S-Tab> shifts focus to the down-caret. When nothing has focus, the up/down cursors have no effect.

With the container selected, or with one of the sub-menu items selected, the up/down cursor keys scroll the sub-menu instead of the whole page. I am not sure how serious this problem is. I can tab out of the sub-menu, or <Esc> followed by <Tab> or <S-Tab>, and then scroll the window. But I cannot scroll the window while the sub-menu is open. (Why would I want to? <shrug />)

Once I select an item on the sub-menu, I can scroll the container with the cursor keys, and if I scroll far enough, then the focused item is no longer in view. The same thing happens (scrolling the whole window) with the Bootstrap page mentioned in the issue summary.

Conclusion:

1. The patch in #29 is an improvement over the current behavior.
2. The behavior in FireFox where the container is selected is a minor WTF.
3. The behavior with <Esc> looks like a bug. It happens with Chromium, too.
4. I think it is better if the cursor keys scroll the whole page, not the container.
5. I agree with #8, that it would be better to scroll the whole page instead of the container.

On the plus side, <Esc> has mostly the expected effect. Compare that to the Bootstrap page, where <Esc> does not do anything.

If (5) is too hard, then I will be satisfied if you can fix (3). I will be even happier if you can fix (2) and (4) along with (3).

I did some more testing with Chromium, and I noticed a problem.

In Firefox, when the container or a sub-menu item has the focus, I can use the up/down keys to scroll the container. When I get to the top or bottom, the key has no effect.

With Chromium, once I scroll to the bottom, the down key makes the page scroll. And then the header menu goes away. I can continue to scroll down, but the up key has no effect. I can get the submenu back with <Tab> or <S-Tab> or by shifting focus away from Chromium or back again.

I guess this is the reason you scroll just the container, not the window.

This problem is serious enough that I am setting the issue back to NW.

With Chromium, once I scroll to the bottom, the down key makes the page scroll. And then the header menu goes away. I can continue to scroll down, but the up key has no effect. I can get the submenu back with or or by shifting focus away from Chromium or back again.

The behaviors you are describing are the default overflow behaviors of the browser. We can change these, but we one million times should not for both accessibility and performance reasons.

I'll attend the meeting on Friday and can give a little more context.

Applied patch #29 and it worked fine.Added screen recording.

RTBC +1

We ran this by the UX meetup at #3208186: Drupal Usability Meeting 2021-04-23. The consensus was that this solution is good from a UX perspective.

Opened followup issue #3210434: Olivero: Header menu should not close if menu item has focus

Followup opened #3210443: Olivero: Focus after submenu close via ESC key.

Note that the body element gets focus, so it's not a violation as I understand it. We can discuss further in the issue.

I checked the Tugboat site at https://3190120-menu-focus-syqxciq9xqqmexouh3gwu9elwn8tzxat.tugboat.qa and it worked fine. I recorded the following video screencasts:

• Google Chrome - Olivero Theme Demonstration Issue 3190120
• Firefox - Olivero Theme Demonstration Issue 3190120
• Safari - Olivero Theme Demonstration 3190120

RTBC +2

Now focused items are in the viewport. Adding after-patch screen recording for reference.

Moving to RTBC.

Still need to get sign off from @bnjmnm (or another core accessibility maintainer)

I am adding an issue tag for #45.

This issue already has a tag for a follow-up issue. That was added in #38, and two issues were cited in #40 and #42. I think we can remove the tag now, but I hope someone else will confirm.

I'm confused. Do you want an accessibility review from another accessibility maintainer?

Do we need to open up any additional followup issues?

@mherchel:

The "Needs accessibility review" issue tag is just a way of indicating what you said in #45:

Still need to get sign off from @bnjmnm (or another core accessibility maintainer)

At least, that is how we use the "Needs usability review" tag.

One practical reason for adding the issue tag: now that the issue is RTBC, it is on the queue for the core committers. They will quickly see the tag and decide whether to start reviewing the issue. If we just mention it in Comment #45, then they will not know that until they get to that comment.

I did not add the "Needs followup" tag, I just brought attention to it. If you are confident that we have all the follow-ups we need, then you can remove the tag. If you are not confident, then maybe @bnjmnm can check, since he is the one who added it.

Makes sense. Thanks for the followup :D

test failures are unrelated. Setting back to RTBC.

Spelled "carrot" like the vegetable as opposed to the character.

Patch works. Caret is present and items scroll into view as they're tabbed to.

Screenshot/cast attached, moving to RTBC.

If I load the interdiff from #55 in my web browser, I see

-        html:not(.js) body:not(.is-always-mobile-nav) .primary-nav__menu-item--level-1:hover .primary-nav__menu-carrot {
+        html:not(.js) body:not(.is-always-mobile-nav) .primary-nav__menu-item--level-1:hover .primary-nav__menu-🥕 {

Looking at the patch in a text editor, I see

-        html:not(.js) body:not(.is-always-mobile-nav) .primary-nav__menu-item--level-1:hover .primary-nav__menu--level-2 {
+        html:not(.js) body:not(.is-always-mobile-nav) .primary-nav__menu-item--level-1:hover .primary-nav__menu--level-2,
+        html:not(.js) body:not(.is-always-mobile-nav) .primary-nav__menu-item--level-1:hover .primary-nav__menu-🥕 {

From the comment, I thought it was going to replace "carrot" with "caret", but it looks like the patch replaces "carrot" with an emoji.

yeah, the emoji reduces the need to know how to spell carrot vs caret (I didn't personally know this).

Emoji's in selectors are 100% valid (including IE11).

Its honestly just a little levity being added to the theme :)

Tugboat URL for the #55 patch: https://3190120-menu-focus-2-mlnvidzs0qvqjkg5gsceuuuxu14f2h0v.tugboat.qa/

Fixed few code style nit picks. Interdiff attached here.

Committed 11af82a and pushed to 9.3.x and 9.2.x. Thanks!