Problem/Motivation

Olivero currently has an expectation that the primary and secondary navigation items will always exist. It will throw JS errors if they do not. 

Uncaught TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.
    at isDesktopNav (js_I3-LxdtjvE_IegIbuj2Um5FE1PkkICsN37a3UrDLN8M.js:463)
    at toggleStickyHeaderState (js_I3-LxdtjvE_IegIbuj2Um5FE1PkkICsN37a3UrDLN8M.js:484)
    at js_I3-LxdtjvE_IegIbuj2Um5FE1PkkICsN37a3UrDLN8M.js:570
    at js_I3-LxdtjvE_IegIbuj2Um5FE1PkkICsN37a3UrDLN8M.js:572

and

Uncaught TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.
    at isDesktopNav (js_I3-LxdtjvE_IegIbuj2Um5FE1PkkICsN37a3UrDLN8M.js:463)
    at IntersectionObserver.toggleDesktopNavVisibility (js_I3-LxdtjvE_IegIbuj2Um5FE1PkkICsN37a3UrDLN8M.js:514)

Steps to reproduce

Remove primary menu block and then look at the JavaScript Console.

To do

Remove primary and secondary nav assets from global styles and only load them when necessary. Add condition within JS so it doesn't throw errors when determining if Olivero is in its desktop nav configuration.

CommentFileSizeAuthor
#18 After Patch 3231416.png567.67 KBchetanbharambe
#18 Before patch 3231416.png609.66 KBchetanbharambe
#17 3231416-16-test-only.patch4.24 KBmherchel
#17 3231416-16.patch6.69 KBmherchel
#14 reroll_diff_3231416_10-14.txt2.39 KBankithashetty
#14 3231416-14.patch6.69 KBankithashetty
#11 after-patch.png87.6 KBgauravvvv
#11 before-patch.png176.48 KBgauravvvv
#10 3231416-10-test-only-FAIL.patch4.24 KBmherchel
#10 3231416-10.patch7.95 KBmherchel
#9 3231416-9-test-only-FAIL.patch4.24 KBmherchel
#9 3231416-9.patch7.95 KBmherchel
#8 3231416-8-test-only-FAIL.patch3.62 KBmherchel
#8 3231416-8.patch7.34 KBmherchel
#7 3231416-7-test-only-FAIL.patch3.51 KBmherchel
#7 3231416-7.patch7.23 KBmherchel
#5 3231416-5.patch3.7 KBmherchel

Comments

davidwbarratt created an issue. See original summary.

davidwbarratt’s picture

davidwbarratt commented
Issue summary: View changes
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

This is a great bug, thanks @davidwbarratt!

Olivero is expecting the primary menu to exist, but it doesn't on your site. I'm going to massage the summary a bit, and then post a patch.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Title: Uncaught TypeError: Window.getComputedStyle: Argument 1 is not an object. » Olivero should not expect its default menu blocks to always exist
Issue summary: View changes
Issue tags: +JavaScript
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 3231416-5.patch3.7 KB

This should resolve it.

davidwbarratt’s picture

davidwbarratt commented
Status: Needs review » Reviewed & tested by the community

Deployed to my site:
https://github.com/davidbarratt/davidwbarratt/blob/f70cdfb9abcc09f061d0b...
and that fixed the problem!
https://davidwbarratt.com/
Thanks!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs tests
StatusFileSize
new 3231416-7.patch7.23 KB
new 3231416-7-test-only-FAIL.patch3.51 KB

Sweet :)

I'm setting this back to needs review. I've created a Nightwatch test for this, but I'm not sure this is the best way to go about testing it.

Ideally, there could be a noConsoleErrors assertion or something, but I couldn't find an easy way to do that (and pass the errors back), plus its out of scope of this issue.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3231416-8.patch7.34 KB
new 3231416-8-test-only-FAIL.patch3.62 KB

Forgot to run prettier. New patches attached.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3231416-9.patch7.95 KB
new 3231416-9-test-only-FAIL.patch4.24 KB

Lets try this again.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3231416-10.patch7.95 KB
new 3231416-10-test-only-FAIL.patch4.24 KB

Third times a charm

🤞🤞🤞🤞🤞🤞🤞

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new before-patch.png176.48 KB
new after-patch.png87.6 KB

Patch #10, fixes the issue.
Testing steps:
1. Remove the primary menu block and then look at the JavaScript Console.
Got two js error messages in console Uncaught TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.

Testing results:
Patch applied #10, No js error messages in the console.
Added before and after patch screenshots for reference.

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
Status: Needs review » Reviewed & tested by the community
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3231416-14.patch6.69 KB
new reroll_diff_3231416_10-14.txt2.39 KB

Rerolled the patch in #10, thanks!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue tags: +Needs reroll

It looks like the re-roll in #14 didn't include the changes in navigation-utils.es6.js.

Also, the diffs between two patch files aren't especially helpful. We need the diff between the code itself when attaching an interdiff.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Looks like the changes in navigation-utils.es6.js are already committed to core via #3194560: Style maintenance page for Olivero.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue tags: -Needs reroll
StatusFileSize
new 3231416-16.patch6.69 KB
new 3231416-16-test-only.patch4.24 KB

Re-uploading patch and also the test only. I'm not sure the test will fail since the JS fix committed as part of #3194560: Style maintenance page for Olivero

chetanbharambe’s picture

chetanbharambe commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Before patch 3231416.png609.66 KB
new After Patch 3231416.png567.67 KB

Verified and tested patch #17.
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: Appearance -> Apply Olivero theme
# Goto: /admin/structure/block
# Remove the primary menu block

Expected Results:
# User should not see JS errors in Console

Note: Currently there are no errors coming in the console before applying the patch.

Please refer attached screenshots for the same.
Looks good to me.
Can be a move to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Nice test and even if the bug is fixed by the other issue I think this approach makes lots of sense.

Committed 7578516 and pushed to 9.3.x. Thanks!

  • alexpott committed 7578516 on 9.3.x 
    Issue #3231416 by mherchel, ankithashetty, Gauravmahlawat,...

Status: Fixed » Closed (fixed)

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