Problem/Motivation

The main-menu.js file included in the Umami theme tries to add an event listener to the main menu block without checking if it actually exists. If the block isn't present on a page, the JS will throw an error:

Cannot read property 'addEventListener' of null

Proposed resolution

Check to make sure `toggler` is not null before adding the event listener.

CommentFileSizeAuthor
#4 interdiff-2--3.txt414 bytesbalsama
#4 3090190-4.patch1.2 KBbalsama
#2 3090190-2.patch1.21 KBbalsama

Comments

balsama created an issue. See original summary.

balsama’s picture

balsama
he/him
Primary language English
Location boston
commented
Status: Active » Needs review
StatusFileSize
new 3090190-2.patch1.21 KB

Patch checks to make sure toggler is not null before adding event listener.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
commented
Status: Needs review » Needs work

This looks good, I confirmed this patch fixes the error logged in the console.
Document.querySelector returns null if no match is found, so checking for null makes sense here.

+++ b/core/profiles/demo_umami/themes/umami/js/components/navigation/menu-main/menu-main.es6.js
@@ -15,5 +15,7 @@
+  })();

tiny nit- this patch introduces 2 extra spaces before the closing bracket on line 21.

balsama’s picture

balsama
he/him
Primary language English
Location boston
commented
Status: Needs work » Needs review
StatusFileSize
new 3090190-4.patch1.2 KB
new interdiff-2--3.txt414 bytes

Thanks. Patch updated.

zrpnr’s picture

zrpnr
he/him
Primary language English
Location UTC-7
commented
Status: Needs review » Reviewed & tested by the community

The patch in #4 fixes the formatting issue, thanks!

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented 
+++ b/core/profiles/demo_umami/themes/umami/js/components/navigation/menu-main/menu-main.es6.js
@@ -15,5 +15,7 @@
-  toggler.addEventListener('click', toggleMenu);
+  if (toggler !== null) {
+    toggler.addEventListener('click', toggleMenu);
+  }

👍 looks good to me. querySelector returns null if the element does not exist.

Follow up nit:

const menu = document.querySelector('[data-drupal-selector="menu-main"]');

Should we also make sure menu is null? Technically it should be... but both variables are required in `toggleMenu`

shaal’s picture

shaal
he/him
Primary language English
Location Boca Raton, FL
commented
Issue tags: +Out of the Box Initiative
Related issues: +#2940023: [PP-1] Improve accessibility of Umami's responsive main menu, +#3042417: Accessible dropdown for Umami's language-switcher and mobile main-menu

Thank you for working on this!
I added some issue tags and related issues.

The mobile menu in Umami has numerous issues #2940023: [PP-1] Improve accessibility of Umami's responsive main menu
And we've been working on it through #3042417: Accessible dropdown for Umami's language-switcher and mobile main-menu that was reviewed today by @smaz, (only few minor changes left for it to be RTBC)
Both language-switcher AND mobile menu will use the same new component.

I am not sure if it would make sense to commit this patch if the other patch can be ready in the next day or two (and would replace all that JS code completely)

In the meantime... more eyes to review #3042417: Accessible dropdown for Umami's language-switcher and mobile main-menu would be awesome and appreciated!

balsama’s picture

balsama
he/him
Primary language English
Location boston
commented

Ah - Ok. I was unaware of #3042417: Accessible dropdown for Umami's language-switcher and mobile main-menu. I would be happy to wait a few days and close this issue as outdated if/when that one lands. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 3090190-4.patch, failed testing. View results

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

acbramley’s picture

acbramley commented
Status: Needs work » Closed (outdated)
Issue tags: +Bug Smash Initiative

This was triaged as part of bug smash.

This javascript no longer exists in core, it was refactored (after being converted from es6) in #3404302: Create header component for Umami, with JS.

The new header JS looks safe, also confirmed by installing Umami and disabling the main nav block and didn't get a javascript error

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.