Problem/Motivation

This is something I initially brought up in #3180086: It should not be possible to have two dropdown menus appear at the same time within Olivero, but moving this to a separate issue as it does not need to obstruct the bug fix that is currently there.

This is what I reported:

With this solution the non-mouse user needs to explicitly close an expanded navigation item (or open a different navigation item) before advancing further if they don't want content to be obscured by the menu. Mouse users are not subjected to this step.

I'd recommend an approach similar to Drupal's dropbutton: add a blur event listener on all elements in that section of the menu. The handler will close the menu after a this.timerId = setTimeout(etc...) but that timeout will be cancelled if the next item that receives focs is also with that menu section. The focus listener would begin with something like window.clearTimeout(this.timerID); The focus/blur handlers can also be the mouseover/mouseout handlers, and the timeout delay provides a user-friendly "grace period" for pointers that inadvertently moved outside the mouseover area for a moment. An existing use of this pattern can be referenced in core/misc/dropbutton/dropbutton.es6.js

In a response, it was argued that this isn't an accessibility concern: #3180086: It should not be possible to have two dropdown menus appear at the same time within Olivero, and it's hard to objectively prove otherwise so this won't get tagged "accessibility". The impacts to UX and Drupal-sentiment still justify this being worked on. Closing secondary menus on blur is a popular enough pattern (I couldn't find a site without this pattern) that users come to expect it. While it's not particularly difficult for a user to adapt, every new micro-confusion unique to Drupal contributes to people souring on it.

Currently setting as stable blocker, but would be fine removing if this is brought to the UX meeting and they feel it is fine as-is.

Testing instructions

  1. Use the tab key to tab over to a menu parent item
  2. Open the menu parent by hitting enter on the button next to the menu item.
  3. Use tab to tab through the menu until you get to the next parent menu item.
  4. The previous menu should now close (this is the new behavior).
  5. Do the same thing again, but after tabbing down into the menu, start hitting SHIFT+TAB to tab backwards.
  6. Tab backwards to the previous menu parent. The menu that was open should now close.
CommentFileSizeAuthor
#17 interdiff-15-17.txt1.25 KBmherchel
#17 3191692-17-test-only-FAIL.patch1.1 KBmherchel
#17 3191692-17.patch6.12 KBmherchel
#15 3191692-15-test-only-FAIL.patch1.14 KBmherchel
#15 3191692-15.patch6.17 KBmherchel
#15 interdiff-14-15.txt0 bytesmherchel
#14 interdiff-12-14.txt1.28 KBmherchel
#14 3191692-14.patch6.17 KBmherchel
#14 3191692-14-test-only-FAIL.patch1.14 KBmherchel
#12 interdiff-11-12.txt1.16 KBmherchel
#12 3191692-12.patch5.98 KBmherchel
#12 3191692-12-test-only-FAIL.patch1012 bytesmherchel
#11 interidff-5-11.txt983 bytesmherchel
#11 3191692-11.patch5.98 KBmherchel
#11 3191692-test-only-FAIL.patch983 bytesmherchel
#8 Screen Recording 2021-04-13 at 18.42.20.mov5.14 MBGauravvvv
#8 Screen Recording 2021-04-13 at 18.41.10.mov3.9 MBGauravvvv
#6 Screenshot 2021-04-13 at 3.34.16 PM.png54.15 KBBhumikaVarshney
#6 after-patch.mp41.45 MBBhumikaVarshney
#5 interdiff-3-5.txt1.66 KBmherchel
#5 3191692-5.patch5 KBmherchel
#3 3191692.patch4.53 KBmherchel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bnjmnm created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented

Brought this to the Drupal UX meeting on Apr 2, 2021. They agreed that this should remain a stable blocker

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Active » Needs review
FileSize
3191692.patch4.53 KB

This is hopefully good to go!

The way that I implemented this:

  1. Attached a blur event to the submenu (with useCapture set to true so it can see the events)
  2. event triggers a function that creates a 200ms timer
  3. when timer completes it checks if focused element is under the same menu parent.
  4. if not, it'll close that submenu

Note that I also had to create a core polyfill for element.closest()

Tugboat URL: https://3191692-close-menu-blur-zvrn1odblbhsbnvlmbx2e6mh2mveyg2m.tugboat...

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Needs review » Needs work

Polyfill is failing ESLint.

Also, I discovered that the blur event is occurring at mobile widths, too. This only needs to be at wide widths.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Needs work » Needs review
FileSize
3191692-5.patch5 KB
interdiff-3-5.txt1.66 KB

Fixed the issues. The polyfill is now excluded from ESLint, and the blur event listener will check to see if it's in desktop nav state.

Tugboat URL stays the same: https://3191692-close-menu-blur-zvrn1odblbhsbnvlmbx2e6mh2mveyg2m.tugboat...

BhumikaVarshney’s picture

BhumikaVarshney CreditAttribution: BhumikaVarshney as a volunteer and at OpenSense Labs commented
FileSize
after-patch.mp41.45 MB
Screenshot 2021-04-13 at 3.34.16 PM.png54.15 KB

Applied patch #5 and its working fine, the blur event is not occurring now.
Thanks.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Issue summary: View changes

@BhumikaVarshney No need to include screenshots of the patch applying. Drupal's automated tests will fail if the patch doesn't apply.

Also, it appears that you tested hover events as opposed to the blur event. Below is instructions for testing (and I'll update the issue description)

  1. Use the tab key to tab over to a menu parent item
  2. Open the menu parent by hitting enter on the button next to the menu item.
  3. Use tab to tab through the menu until you get to the next parent menu item.
  4. The previous menu should now close (this is the new behavior).
  5. Do the same thing again, but after tabbing down into the menu, start hitting SHIFT+TAB to tab backwards.
  6. Tab backwards to the previous menu parent. The menu that was open should now close.
Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commented
FileSize
Screen Recording 2021-04-13 at 18.41.10.mov3.9 MB
Screen Recording 2021-04-13 at 18.42.20.mov5.14 MB

I have added before and after patch behavior screen recordings.

Now menu item gets closed when we move to the next menu item. I have tested this on live preview https://3191692-close-menu-blur-zvrn1odblbhsbnvlmbx2e6mh2mveyg2m.tugboat...
Moving to RTBC.

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commented
Status: Needs review » Reviewed & tested by the community
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Now that we have some JS test coverage for the menus, could we add tests for this too?

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3191692-test-only-FAIL.patch983 bytes
3191692-11.patch5.98 KB
interidff-5-11.txt983 bytes

Tests added!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
FileSize
3191692-12-test-only-FAIL.patch1012 bytes
3191692-12.patch5.98 KB
interdiff-11-12.txt1.16 KB

Some errors from prettier. Fixed, should work now.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Needs review » Needs work

Tests are passing on my local, but not on Drupal CI :(

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Needs work » Needs review
FileSize
3191692-14-test-only-FAIL.patch1.14 KB
3191692-14.patch6.17 KB
interdiff-12-14.txt1.28 KB

Lets see if DrupalCI likes these tests better.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
FileSize
interdiff-14-15.txt0 bytes
3191692-15.patch6.17 KB
3191692-15-test-only-FAIL.patch1.14 KB

Darnit. Forgot to run prettier. New patches attached.

The last submitted patch, 15: 3191692-15.patch, failed testing. View results

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
FileSize
3191692-17.patch6.12 KB
3191692-17-test-only-FAIL.patch1.1 KB
interdiff-15-17.txt1.25 KB

If this doesn't work, I'm gonna flip my desk.

katannshaw’s picture

katannshaw CreditAttribution: katannshaw at Lullabot commented
Status: Needs review » Reviewed & tested by the community

I went through the "Testing instructions" listed in the OP on the Tugboat site and everything worked as expected as seen in this video:

Monosnap video: https://monosnap.com/direct/gGzK35KlSE1LdP30WJCwqrpdFQ13oG

Marking this RTBC.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: +Needs change record

We still need to open change record for the new polyfill similar to https://www.drupal.org/node/3159731 ✌️

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented

Change record located at https://www.drupal.org/node/3211146

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

Committed 67e7662 and pushed to 9.2.x. Thanks!

  • lauriii committed 67e7662 on 9.2.x 
    Issue #3191692 by mherchel, Gauravmahlawat, BhumikaVarshney, bnjmnm,...

Status: Fixed » Closed (fixed)

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