Problem/Motivation

The navigation toolbar is very efficiently cached, but when it's not it gets rendered by big pipe. This creates jank (content layout shift) as the main page content gets pushed sideways after the navigation bar is rendered.

#3505610: Add a placeholder preview for the navigation toolbar would use a render placeholder to reserve the space, but this is apparently hard to make work - two of us failed on there.

A quick fix is to use the placeholder strategy deny list - this could mean slightly slower rendering of the rest of the page, but much less content layout shift as it renders. Overall to me at least, it feels less noticeable than the jank.

Steps to reproduce

Install Umami, as user one clear the cache on admin/config/development/performance - notice that the side bar loads with the rest of the page content and doesn't push it sideways (compared to HEAD). Refresh the page or visit a different page and it should be just as snappy.

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#26 judder.mov44.93 KBgodotislate
#9 3585957-nr-bot_r3i5fflf.txt91 bytesneeds-review-queue-bot
#7 3585957-nr-bot_z7bahug_.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3585957

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

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

Put up an MR. This might affect cold cache performance slightly because order of things will be different, we'll find out..

catch opened merge request !15513

catch’s picture

catch
he/him
Primary language English
commented

Results in a very tiny improvement in performance tests too, navigation css/js is loaded with the assets from the main request instead of separately (as it would be on a render cache hit), and slightly less processing which is probably also the asset system not having to calculate anything.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented
Related issues: +#3505610: Add a placeholder preview for the navigation toolbar
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3585957-nr-bot_z7bahug_.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3585957-nr-bot_r3i5fflf.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

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

bluegeek9 changed the visibility of the branch main to hidden.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
Issue tags: +Performance

Rebased.

smustgrave’s picture

smustgrave commented

#3618256: Adjust tests using olivero_test test module is in review should we try and push that one forward first?

catch’s picture

catch
he/him
Primary language English
commented

@smustgrave I don't think that has anything to do with this issue does it?

smustgrave’s picture

smustgrave commented

That’s the link in the todo

catch’s picture

catch
he/him
Primary language English
commented

The link in the todo is #3505610: Add a placeholder preview for the navigation toolbar.

smustgrave’s picture

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

Disregard my last comment some reason my clipboard wasn't clearing on my macbook. Sorry for the noise, this looks fine.

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

godotislate’s picture

godotislate
he/him
commented

I'm actually seeing an issue on HEAD where on Umami, after clearing cache from admin/config/development/performance per IS, the navigation does not appear at all, and there's a browser console error:

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'replace')
    at #Ie (htmx.min.js?v=4.0.0:1:16679)
    at Object.swap (htmx.min.js?v=4.0.0:1:19974)
    at big_pipe.commands.js?v=12.0-dev:79:14
    at Array.forEach (<anonymous>)
    at Object.insert (big_pipe.commands.js?v=12.0-dev:74:15)
    at big_pipe.commands.js?v=12.0-dev:31:41

This might be another symptom of the race condition @berdir alluded to in #3566843-27: Navigation module results in unintended caching

MR changes fix this.

  • godotislate committed 9afec8fa on main 
    task: #3585957 Prevent the navigation sidebar being rendered via big...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 9afec8f and pushed to main. Thanks!

Need an 11.x MR for conflicts in the performance tests. Should this go to 11.4.x as well?

godotislate’s picture

godotislate
he/him
commented

Did the same cache-clear test on Umami 11.4.x. I'm not seeing the JS error, but preventing big pipe rendering of navigation does fix the sideways layout shift seen on HEAD. There's a vertical layout shift that I haven't looked into and assume is from something else.

Race condition could still be an issue on 11.4.x, but it hasn't presented itself in my local tests as obviously.

catch’s picture

catch
he/him
Primary language English
commented

I can't actually see the vertical layout shift on 11.x, there's a bit of flicker when the main content loads but the header and navigation sidebar both look static.

Cherry-picked to a new 11.x branch and updated performance tests.

If this was only fixing the layout shift I wouldn't have any interest in an 11.4 backport but we might want to for the js errors / race condition that are so-far unexplained. Not sure if it will need a separate backport MR or not yet.

catch opened merge request !17054

  • godotislate committed 37e2cf31 on 11.x 
    task: #3585957 Prevent the navigation sidebar being rendered via big...
godotislate’s picture

godotislate
he/him
commented
Version: 11.x-dev » 11.4.x-dev
StatusFileSize
new judder.mov44.93 KB

I can't actually see the vertical layout shift on 11.x, there's a bit of flicker when the main content loads

Tried again today on 11.4.x with fix applied, and it was reproducing far less frequently than yesterday, like 1 in 20, if that. The "flicker" you described might be what I'm seeing. Attached screen recording: https://www.drupal.org/files/issues/2026-09-09/judder.mov.

Committed 37e2cf3 and pushed to 11.x. Thanks!

Doesn't apply to 11.4.x because the performance test conversion using YAML files wasn't backported there.

godotislate opened merge request !17261

godotislate’s picture

godotislate
he/him
commented
Status: Patch (to be ported) » Needs review

11.4 MR: https://git.drupalcode.org/project/drupal/-/merge_requests/17261

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

MR looks ok.

  • catch committed 0077935e on 11.4.x 
    task: #3585957 Prevent the navigation sidebar being rendered via big...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.4.x, thanks!

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

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

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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