This can be viewed at https://tugboat-aqrmztryfqsezpvnghut1cszck2wwasr.tugboat.qa/tabs

If secondary tabs are present, the JS will throw an error

drupal.js?v=9.3.0-dev:16 Uncaught TypeError: Cannot read properties of null (reading 'clientHeight')
    at isTabsMobileLayout (tabs.js?v=9.3.0-dev:15)
    at init (tabs.js?v=9.3.0-dev:28)
    at Array.forEach (<anonymous>)
    at Object.attach (tabs.js?v=9.3.0-dev:40)
    at drupal.js?v=9.3.0-dev:27
    at Array.forEach (<anonymous>)
    at Object.Drupal.attachBehaviors (drupal.js?v=9.3.0-dev:24)
    at drupal.init.js?v=9.3.0-dev:29
    at HTMLDocument.listener (drupal.init.js?v=9.3.0-dev:17)

CommentFileSizeAuthor
#9 3244621-tests-only.patch1.97 KBcatch
#8 3244621-post patch.png839.28 KBrachel_norfolk
#7 interdiff_2_7.txt1.97 KBrikki_iki
#7 3244621-7.patch5.47 KBrikki_iki
#4 3244621_olivero_js_tabs_patched.png44.26 KBcathwaldron
#4 3244621_olivero_js_tabs_unpatched.png69.5 KBcathwaldron
#2 3244621.patch3.5 KBmherchel
Tabs___Olivero_🔊.png46.02 KBmherchel

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Active » Needs review
StatusFileSize
new 3244621.patch3.5 KB

Tugboat preview for this patch is at https://3244621-tabs-js-error-ohijdc1xm9ymzyxcaurbsbkezyrdeuna.tugboat.q...

cathwaldron’s picture

cathwaldron commented

Issue 3244621

I checked to see if MHerchel's 3244621.patch eliminates the JS error.
I confirmed that yes, it does so.

Detail - Here is What I did:
In a browser window on my PC, I compared patched to unpatched Drupal Tugboat instances, to check that the JS error exists with unpatched, and does not exist with patched instance.

OS / Browser / Status:

Ubuntu 18.04 18.04.6 LTS 64-bit / Chrome Version 94.0.4606.61 (Official Build) (64-bit) / patch fixes error
Ubuntu 18.04 18.04.6 LTS 64-bit / Firefox Version 93.0 (64-bit) / patch fixes error
Windows 10 Home Version 20H2 (64-bit) / Edge Version 95.0.1020.30 (Official Build) (64-bit) / patch fixes error
Windows 10 Home Version 20H2 (64-bit) / Chrome Version 95.0.4638.54 (Official Build) (64-bit) / patch fixes error
Windows 10 Home Version 20H2 (64-bit) / Firefox Version 93.0 (64-bit) / patch fixes error

Attached screenshots (Edge browser on Windows 10 PC) show the comparison of unpatched and patched Olivero Tugboat instances.
Per MHerchel I will mark the status of this issue RTBC (reviewed and tested by the community)

cathwaldron’s picture

cathwaldron commented
StatusFileSize
new 3244621_olivero_js_tabs_unpatched.png69.5 KB
new 3244621_olivero_js_tabs_patched.png44.26 KB
cathwaldron’s picture

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

Attached screenshots (Edge browser on Windows 10 PC) show the comparison of unpatched and patched Olivero Tugboat instances, and that the patch fixes the JS error.
Per MHerchel I will mark the status of this issue RTBC (reviewed and tested by the community)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I think we should add some automated test coverage of Olivero's tabs. It looks like we're missing coverage.

rikki_iki’s picture

rikki_iki commented
Status: Needs work » Needs review
StatusFileSize
new 3244621-7.patch5.47 KB
new interdiff_2_7.txt1.97 KB

Here's an untested test.... Having trouble with chromedriver locally so putting it up for ci to test

rachel_norfolk’s picture

rachel_norfolk
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 3244621-post patch.png839.28 KB

Looks fixed in Safari 15.1, too.

screenshot of safari showing the console with no errors

The tests added in #7 look to give reasonable cover and are currently passing so I'm going to mark as RTBC!

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 3244621-tests-only.patch1.97 KB

Uploading a test only patch to ensure the new test covers this bug.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

While I am not a frontend pro, the fix itself looks very straightforward and in fact mostly a docs fix to follow the selector change to focus the behaviour on the primary tabs only. I agree that the fix looks good.

catch’s picture

catch
he/him
Primary language English
commented 
✖ Olivero/oliveroPrimaryTabsTest
15:11:08  – Verify desktop primary tab display (5.415s)
15:11:08    Timed out while waiting for element <[data-drupal-nav-primary-tabs]> to be present for 5000 milliseconds. - expected "visible" but got: "not found" (5105ms)
15:11:08        at Object.Verify desktop primary tab display (/var/www/html/core/tests/Drupal/Nightwatch/Tests/Olivero/oliveroPrimaryTabsTest.js:29:8)
15:11:08        at processTicksAndRejections (internal/process/task_queues.js:97:5)
15:11:08  – Verify mobile tab display and click functionality (5.37s)
15:11:08    Timed out while waiting for element <[data-drupal-nav-primary-tabs]> to be present for 5000 milliseconds. - expected "visible" but got: "not found" (5100ms)
15:11:08        at Object.Verify mobile tab display and click functionality (/var/www/html/core/tests/Drupal/Nightwatch/Tests/Olivero/oliveroPrimaryTabsTest.js:38:8)
15:11:08

Nice!

Patch in #7 remains the one to commit.

  • lauriii committed f8a197f on 9.4.x 
    Issue #3244621 by mherchel, rikki_iki, catch, cathwaldron,...
lauriii’s picture

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

Committed f8a197f and pushed to 9.4.x. Also cherry-picked to 9.3.x. Thanks!

  • lauriii committed 5082060 on 9.3.x 
    Issue #3244621 by mherchel, rikki_iki, catch, cathwaldron,...

Status: Fixed » Closed (fixed)

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