Problem/Motivation

Calling this a regression because I think it surfaced with #3568566: Olivero's secondary menus inoperable when authenticated & BigPipe present.

It appears that the second-level-navigation.js file gets loaded as a dependency of the olivero/search-wide library, even when there are no menu items.

This causes a JavaScript error when the ESC key is pressed, which prevents the mobile menu from closing:

second-level-navigation.js?v=12.0-dev:108 Uncaught TypeError: Cannot read properties of undefined (reading 'forEach')
    at Object.areAnySubNavsOpen (second-level-navigation.js?v=12.0-dev:108:25)
    at HTMLDocument.<anonymous> (navigation.js?v=12.0-dev:53:27)
areAnySubNavsOpen @ second-level-navigation.js?v=12.0-dev:108
(anonymous) @ navigation.js?v=12.0-dev:53

Most sites won't have an empty main menu, but I found that the problem also occurs with the nightwatch_a11y_testing profile and causes tests for #3338664: Migrate Nightwatch Axe tests to PHPUnit to fail by default because of the console error.

Steps to reproduce

  1. Install with the Standard profile.
  2. Log in as admin.
  3. Go to /admin/structure/menu/manage/main.
  4. Uncheck the box in the "Enabled" column for all menu items.
  5. Click "Save"
  6. Enable the Search module.
  7. Go to the front (Home) page.
  8. Reduce viewport width until the "mobile" menu toggle button appears (aka, the hamburger).
  9. Click the toggle button to expand the menu.
  10. Press the ESC key.
Expected behavior
  • The menu closes.
  • JavaScript errors do not appear in the browser console.
Actual behavior
  • The menu does not close.
  • JavaScript errors do appear in the browser console.

Note: The console error appears even when the menu isn't open, but AFAICT that case doesn't break functionality.

Proposed resolution

Copy the following change from #3515403: Update to selenium/standalone-chrome:133 to an MR for this issue:

-  let secondLevelNavMenus;
+  let secondLevelNavMenus = [];

Remaining tasks

  1. Create MR.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#7 patch-After.png57.38 KBrupeshghar
#7 patch-before.png47.64 KBrupeshghar
#7 olivero-mobile_menu_not_closing_on_esc-3584314.patch476 bytesrupeshghar

Issue fork drupal-3584314

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

kentr created an issue. See original summary.

kentr’s picture

kentr commented
Issue tags: +Regression
kentr’s picture

kentr commented
Issue summary: View changes
kentr’s picture

kentr commented
Related issues: +#3568566: Olivero's secondary menus inoperable when authenticated & BigPipe present

longwave made their first commit to this issue’s fork.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Rediscovered this over in #3515403: Update to selenium/standalone-chrome:133 which triggers this same bug from a test (older Chrome did not). The fix is a one liner:

-  let secondLevelNavMenus;
+  let secondLevelNavMenus = [];
rupeshghar’s picture

rupeshghar commented
Status: Active » Needs review
StatusFileSize
new olivero-mobile_menu_not_closing_on_esc-3584314.patch476 bytes
new patch-before.png47.64 KB
new patch-After.png57.38 KB

Root cause: with an empty main menu, secondLevelNavMenus stays undefined since init() never runs. When Search is enabled, navigation.js's Escape-key handler still calls areAnySubNavsOpen() → secondLevelNavMenus.forEach(...), throwing Cannot read properties of undefined (reading 'forEach') and aborting before the mobile menu actually closes.

Fix per #6: initialize secondLevelNavMenus = []. Verified with a headless-browser check — before: Escape throws and menu stays open; after: no errors, menu closes. Patch attached.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Fixes need to be in MRs for review please

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

This was already fixed in main and 11.x as part of #3515403: Update to selenium/standalone-chrome:133 but I think we could backport the fix itself to 11.4.x and 10.6.x here.

kentr’s picture

kentr commented
Status: Needs work » Postponed (maintainer needs more info)

@longwave:

I think we could backport the fix itself to 11.4.x and 10.6.x here.

For that, does it need an MR here with this change?

-  let secondLevelNavMenus;
+  let secondLevelNavMenus = [];
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Postponed (maintainer needs more info) » Needs work

Yes, that's all, not even sure we need test coverage here really.

kentr’s picture

kentr commented
Issue summary: View changes

rupeshghar opened merge request !17040

quietone’s picture

quietone commented
Project: Drupal core » Olivero
Version: main » 2.x-dev
Component: Olivero theme » Code

Olivero is now a contributed project