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
| Comment | File | Size | Author |
|---|---|---|---|
| #35 | 3192903-33-reroll.patch | 5.18 KB | mherchel |
| #21 | 3192903-20-2.patch | 5.09 KB | mherchel |
| #20 | 3192903-20.patch | 0 bytes | mherchel |
| #20 | interdiff-18-20.txt | 1.36 KB | mherchel |
| #17 | 3192903-17.patch | 5.15 KB | mherchel |
Comments
Comment #2
andrewmacpherson commentedComment #3
mherchelSimple 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).Comment #4
lauriiiWe 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.
Comment #5
nod_Comment #6
mherchelUpdated the JS to not utilize the CSS classes added by Twig.
Comment #7
mherchelDuring Olivero mini-sprint, @lauriii reviewed this. He's believes that the menu should close if there's a
clickevent outside of it (since the menu won't necessarily automatically close on themouseoutevent)Comment #8
mherchelUpdated patch attached. This adds an event listener that tells the browser to close the menus if a click happens outside of the menu element.
Comment #9
andrewmacpherson commentedClarifying 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?
Comment #10
andrewmacpherson commentedRe. #7:
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.
Comment #11
mherchelGood point. I'll open up a followup. In the meantime, attached is a new patch with that specific functionality removed.
This is currently the case.
Comment #12
mherchelComment #13
anacolautti commentedReproduced 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.
Comment #14
mherchelI 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.
Comment #15
andrewmacpherson commented@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.)
Comment #16
mherchelComment #17
mherchelUpdated patch and video attached!
Comment #18
nod_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?Comment #19
mherchelNope. You're completely correct! Good catch :D
Comment #20
mherchelUpdated patch attached!
Comment #21
mherchelUploading the actual patch here :D
Comment #22
droplet commentedIf 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
Comment #23
mherchelI'm not disagreeing with you, but there are accessibility reasons outlined by @andrewmacpherson in the comments above.
I'm not against this, but currently Olivero is the only core theme that has dropdown menus that appear on hover.
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.
Comment #24
gauravvvv commentedI have re-rolled patch #21, I have fixed the dropdown arrow upside down when it's on/off in this patch. Please review.
Comment #25
gauravvvv commentedArrow direction fixed in #24, when aria-expanded is true.
Comment #26
gauravvvv commentedComment #27
gauravvvv commentedComment #28
gauravvvv commentedComment #29
gauravvvv commentedComment #30
bhumikavarshney commentedHi @Gauravmahlawat
Patch works fine for me.
Needs maintainer review.
Thanks!.
Comment #31
mherchelThe patch in #21 is still good.
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).
Comment #32
katannshaw commentedWorks great. Marking RTBC!
Comment #33
lauriiiComment #34
gauravvvv commentedComment #35
mherchelRe-roll of patch in #21
Comment #36
mherchelMoving back to RTBC as @katannshaw has approved it in #32
Comment #38
lauriiiCommitted 4ae7ef9 and pushed to 9.2.x. Thanks!
Leaving open until the followup mentioned in #11 has been opened.
Comment #39
mherchelOpened followup #3206211: Olivero: Close desktop dropdown menus if click event is outside of submenu
Comment #41
quietone commentedFollow up was made, removing tag.