After merging #3504386: Use a placeholder for the navigation toolbar, it was noticed that the Navigation's expand/collapse button is not working as expected anymore when Big Pipe module is enabled.
To avoid multiple js initializations, the code has the following instance:
if (context === document)
Once the Navigation component is served using a lazy builder, this logic could not work because the navigation bar could be served when the context is not the full document.
Test on pages with Olivero theme
Find an alternative logic to determine whether the component should be initialized or not.
Review and test.
None.
None.
None.
None.
Bug fix. N/A
Show commandsStart 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:
changes
Comments
Comment #3
plopescCreated MR where the mentioned if clause is replaced by a check for the element in the context.
Also updated the specific Nightwatch test for this functionality to use big_pipe.
Tested manually and at first glance it works well, but a review from more experienced JS developer would be appreciated.
Comment #4
kushagra.goyal commentedHi @plopesc,
I followed the same steps you mentioned and performed a fresh setup with the following modules. After installing the modules, the collapse and expand functionality worked as expected for me.
Does this issue occur consistently for you, or is it intermittent? Please let me know if there are any specific steps or configurations you have applied so I can look more effectively.
Comment #5
finnsky commentedComment #6
finnsky commentedI suddenly discovered that we don't need context and behavior.
Comment #7
plopescTested manually and it behaves as expected, fixing the reported bug.
However, the JS here is out of my technical knowledge. So I prefer to have the RTBC signoff from a more experienced FE developer.
Thank you!
Comment #8
smustgrave commentedSo ran the test-only feature and appears to be passing. Possible it could be something else?
Comment #9
finnsky commented@smustgrave i'm not sure. what you mean? tests green. we need review here
Comment #10
smustgrave commentedUnless nightwatch doesn’t work in the test only feature I’d expect the test to fail without the fix
Comment #11
finnsky commentedThen I would solve the testing problem in the next task. Here we have a critical bug.
I have no idea how the BigPipe works and how well it is handled by Nightwatch. But now the Navigation module does not work at all in Olivero and partly in Claro.
Comment #12
finnsky commentedhttps://www.drupal.org/project/drupal/issues/3506880
Comment #13
smustgrave commentedI’ll leave for someone else then. But typically see that when tests are pushed to a follow up they usually don’t happen
Comment #14
plopescMade some new changes in the Nightwatch test to ensure that cache is cleared before reloading the navigation after installing bigpipe and it failed locally without the JS changes.
However, the test-only job stays green. I don't see whether the specific nightwatch test was executed either. Could it be something wrong with the CI job?
Comment #15
catchI'm not sure the test only job supports nightwatch at all.
It might be worth a second, test only MR, to show the nightwatch test failing without the fix just to make sure it's not a bigger CI issue.
Comment #17
plopescCreated MR 11227 that demonstrates that test-only changes break the Nightwatch test, while the original MR 11124 is still green.
Comment #18
m4oliveiThe code looks great, some nice simplifications while addressing the bug.
I've tested with and without big_pipe. The expand/collapse logic works in both scenarios. I've also tested all of the triggers for expand/collapse, which include the hamburger, close icon, and overlay on mobile as well as the bottom left arrow on desktop. All of those scenarios work as designed.
RTBC for me. Nice work all!
Comment #20
nod_Committed d0e11e1 and pushed to 11.x. Thanks!
Could be good to have for 11.1.x, doesn't cherry pick cleanly so need a new MR
Comment #22
plopesc@nod_ if I'm not mistaken, this bug was a regression introduced by #3504386: Use a placeholder for the navigation toolbar to adapt Navigation to #3493911: Add a CachedPlaceholderStrategy to optimize render cache hits and reduce layout shift from big pipe and looks like that none of them were backported to 11.1.x.
Not sure if we need to backport this one to 11.1.x then.
Comment #23
nod_My bad thanks for checking this!