Problem/Motivation

Mouseout event should not close navigation sub-menu if focus is inside the sub-menu.

Some users don't use a pointer, but the device is nevertheless equipped with one.

  • Your laptop has a touchpad one whether you like it or not. Some desktop OS offer a easy way to disable it (e.g. KDE Plasma), but many others don't (macOS).
  • Many computers are shared by several users.

Even when you don't intentionally use a pointer, accidental pointer movements can easily occur. Here are some common nuisance scenarios...

  • A colleague in your office brings a pile of mail, or a cup of tea. When they put it down, the mouse gets nudged accidentally.
  • Your pet cat comes to curl up on your desk. It twitches it's tail, and the mouse gets nudged accidentally.
  • A keyboard-only user hasn't figured out how to disable the touchpad on their laptop. They sometimes touch it accidentally, causing pointer movements. This one happens a lot.

The problem only affects the sub-menus on the wide menu display. The narrow menu display is unaffected.

Steps to reproduce

  • Use a wide viewport. The problem happens with the wide menu.
  • Using a keyboard, navigate to a sub-menu disclosure button, and open the sub-menu.
  • Press tab to move focus to one of the sub-menu links.
  • Now move the mouse pointer accross the sub-menu. When the pointer moves outside the submenu boundary, the sub-menu closes.
  • Now it's not clear where keyboard focus is. The active element hasn't changed, but is no longer visible, or in the tabbing order. Now, pressing tab will move focus to the next top-level item, not the next sub-menu item. If you want a sub-menu item you'll have to shift-tab backwards and open it again.

Proposed resolution

The mouseout event listener callback should check where focus is before deciding to close the sub-menu. If focus is is currently inside a sub-menu, or focus is on the button which controls the sub-menu, then don't close it.

// Pseudo-code...

onMouseOut() {
    if (subMenu is open && (activeElement is desendent of subMenu  OR activeElement == subMenuDisclosureButton) {
      return // do nothing, leave it open!
   }
   closeSubMenu();

Remaining tasks

User interface changes

No design changes. Kinder to keyboard users.

API changes

Data model changes

Release notes snippet

Comments

andrewmacpherson created an issue. See original summary.

andrewmacpherson’s picture

Issue summary: View changes
mherchel’s picture

Status: Active » Needs review
StatusFileSize
new1.13 KB
new4.42 MB

Simple fix! Patch attached with video.

Note this fixes the problem described in this issue, however it's still possible to close a submenu by mouseover'ing a different parent item (which opens that submenu and triggers the closing of the submenu that has the focused element).

lauriii’s picture

Status: Needs review » Needs work
+++ b/core/themes/olivero/js/second-level-navigation.es6.js
@@ -68,7 +68,12 @@
+          '.primary-nav__menu--level-2.is-active *',

We should not use presentational classes for attaching JS. If we fixed that here, it wouldn't add work into #3153234: [Meta] Olivero JavaScript should be selecting [data-drupal-selector] attributes where possible.

nod_’s picture

Issue tags: +JavaScript
mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new5.05 KB
new5.07 KB

Updated the JS to not utilize the CSS classes added by Twig.

mherchel’s picture

Status: Needs review » Needs work

During Olivero mini-sprint, @lauriii reviewed this. He's believes that the menu should close if there's a click event outside of it (since the menu won't necessarily automatically close on the mouseout event)

mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new1.41 KB
new5.58 KB

Updated patch attached. This adds an event listener that tells the browser to close the menus if a click happens outside of the menu element.

andrewmacpherson’s picture

Issue summary: View changes

Clarifying the proposed resolution. Mouse movements shouldn't close a sub-nav list if focus is on the sub-nav disclosure button either.

That's the situation when a keyboard user has just activated a sub-nav disclosure button, and takes time to read the list before pressing another key. An unintended mouse movement would be annoying here too.

I haven't manually tested patch #8 yet. Maybe it covers that already?

andrewmacpherson’s picture

Status: Needs review » Needs work

Re. #7:

He's believes that the menu should close if there's a click event outside of it (since the menu won't necessarily automatically close on the mouseout event)

That's not the scope of this issue. Please don't add new behaviours unrelated to the bug that needs to be fixed. Mixing bugfixes and feature requests in the same issue makes it harder to keep track of what the designed behaviour is. A lot of accessibility review time gets taken up by trying to hunt down the issue comments that introduced a behaviour. Often, I find there was barely any discussion about it. Adding new behaviours in a hurry is a good way to make things less accessible.

Was there an explanation of who (or how) it is intended to help? More importantly, was there any consideration for who it could hinder?

I'm thinking of a situation where a non-hover pointer user tries to activate a link inside a submenu, but misses. If the menu closes, they'll have to open it to try again, and that's extra labour.

Please file a separate issue to consider this.

mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new5.02 KB
new1.08 KB

Mixing bugfixes and feature requests in the same issue makes it harder to keep track of what the designed behaviour is.

Good point. I'll open up a followup. In the meantime, attached is a new patch with that specific functionality removed.

Clarifying the proposed resolution. Mouse movements shouldn't close a sub-nav list if focus is on the sub-nav disclosure button either.

This is currently the case.

mherchel’s picture

Issue tags: +Needs followup
anacolautti’s picture

Status: Needs review » Reviewed & tested by the community

Reproduced the issue:
https://www.loom.com/share/09b3467c9de946d7b528e2fbcad6b3a3

Applied patch on #11

Got this result:

https://www.loom.com/share/ad8be4488b804e258eebe00b336af91a

As expected, after applying the patch the dropdown remains open even after I move the mouse around (while not hovering over other menu items of the level 0, as discussed above.)

Tested on Drupal 9.2-dev, same version of Olivero, on Chrome.

mherchel’s picture

Status: Reviewed & tested by the community » Needs review

I spoke with @andrewmacpherson about this (and many other things) yesterday, and demo'd it for him. It works as it should, although he still needs to give the sign-off.

andrewmacpherson’s picture

Status: Needs review » Needs work

@anacolautti - thanks for the video evidence in #13. Those are nice, clear demonstrations. However they don't test the case where the mouse movement happens while focus is on the sub-nav open/close button.

I tested patch #11 in Firefox 85 and Chrome 88 (both on Kubuntu Linux, fwiw).

The patch fixes the case where focus is on a link inside a sub-nav list.

It doesn't fix the case where focus is on a sub-nav open/close button. (I don't recall whether @mherchel and I checked that during our video call last week.)

mherchel’s picture

Issue tags: +FLDC2021
mherchel’s picture

Status: Needs work » Needs review
Issue tags: -FLDC2021
StatusFileSize
new3.28 MB
new1.29 KB
new5.15 KB

Updated patch and video attached!

nod_’s picture

+++ b/core/themes/olivero/js/second-level-navigation.es6.js
@@ -68,7 +68,11 @@
+        !document.activeElement.matches('.is-active-menu-parent *') &&

seems like we can simplify a bit no? !document.activeElement.matches('.is-active-menu-parent *, [aria-expanded="true"]') Seems to be working as well or is there some cases I'm not seeing?

mherchel’s picture

Status: Needs review » Needs work

Nope. You're completely correct! Good catch :D

mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new1.36 KB
new0 bytes

Updated patch attached!

mherchel’s picture

StatusFileSize
new5.09 KB

Uploading the actual patch here :D

droplet’s picture

If your cat touched, you can start again. BUT the below case, you may refresh the whole page and lost your inputs.

The story can be I started with KB and can't use KB anymore, then I will find it difficult to close the menu WITH MOUSE after this patch.

Or the story can be I use Mouse and the cat nudged the KB that tabbed inside into menu, then I don't know how to close it with the mouse.

If you're a KB user, I think it's almost ZERO of time your mouse & KB pointed to the menu. (A little movement of the Mouse won't affect it mostly)

If the mouse moves from outside to inside and then moves out. Menu should be closed.

BTW, why not make it a CORE feature or lib that can be extended? (Only make it standard classes will work)

** another problem btw, the dropdown arrow should change upside down when it's on/off

mherchel’s picture

I'm not disagreeing with you, but there are accessibility reasons outlined by @andrewmacpherson in the comments above.

BTW, why not make it a CORE feature or lib that can be extended? (Only make it standard classes will work)

I'm not against this, but currently Olivero is the only core theme that has dropdown menus that appear on hover.

another problem btw, the dropdown arrow should change upside down when it's on/off

I'm not sure I agree with you here, but you can open up a followup issue and we can bring it up with the UX team.

gauravvvv’s picture

StatusFileSize
new6.85 KB

I have re-rolled patch #21, I have fixed the dropdown arrow upside down when it's on/off in this patch. Please review.

gauravvvv’s picture

StatusFileSize
new22.57 KB

Arrow direction fixed in #24, when aria-expanded is true.

gauravvvv’s picture

StatusFileSize
new6.87 KB
gauravvvv’s picture

StatusFileSize
new6.86 KB
gauravvvv’s picture

StatusFileSize
new6.86 KB
gauravvvv’s picture

bhumikavarshney’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new132.87 KB

Hi @Gauravmahlawat
Patch works fine for me.
Needs maintainer review.
Thanks!.

mherchel’s picture

Status: Reviewed & tested by the community » Needs review

The patch in #21 is still good.

I have fixed the dropdown arrow upside down when it's on/off in this patch.

Lets tackle this in a separate issue. This needs to be run through our designers (and maybe UX team).

Also, when you upload new patches, please include the interdiff (the changes between the two patches).

katannshaw’s picture

Status: Needs review » Reviewed & tested by the community

Works great. Marking RTBC!

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
gauravvvv’s picture

mherchel’s picture

Issue tags: -Needs reroll
StatusFileSize
new5.18 KB

Re-roll of patch in #21

mherchel’s picture

Status: Needs work » Reviewed & tested by the community

Moving back to RTBC as @katannshaw has approved it in #32

  • lauriii committed 4ae7ef9 on 9.2.x
    Issue #3192903 by mherchel, Gauravmahlawat, andrewmacpherson: Mouseout...
lauriii’s picture

Committed 4ae7ef9 and pushed to 9.2.x. Thanks!

Leaving open until the followup mentioned in #11 has been opened.

mherchel’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

Issue tags: -Needs followup

Follow up was made, removing tag.