Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays

Confirmed that the flicker in #60 is introduced by the changes in the MR. It seems to be related to the #toolbar-item-anti-flicker element, but I didn't research why it was causing the flickering.

Linking another related issue which approaches the problem differently.

Here's one more animation showing other kind of flicker that's happening when the shortcut tray is open:

Updated the MR and provided a slow-motion example on Youtube. With this, the remaining flickers are the SVGs loading which is being tackled in 📌 Add progress spinner/message functionality to submit buttons Closed: outdated . That said, there's a little bit of strategic fade in/out which is less ideal than the toolbar just appearing static, but it's better than the flickers. here's why:

The flickering that is sorta introduced by #toolbar-item-anti-flicker as mentioned in the prior comment is something I'm not sure is addressable on the PHP end without messing with caches (happy to find out I'm wrong). Anywhere in PHP that I could think to put logic that would determine the active tray was something that would ultimately get cached and not consistently aware of which tray was active. Relaxing the cache (unsurprisingly) increased page load times significantly so that's certainly not a good route to go.

As a workaround, I fade the horizontal menu in so the default admin menu is not briefly rendered before the active menu appears. Hoping there's a way to use the cookie approach instead of this workaround. It seems like the approach in #1863824: Use the temp store to remember the last state of the toolbar per user and reload it → has the same obstacles as what are being faced here, but if there's something about getting the data from Drupal state vs cookie that circumvents the cache challenges then I'll look into that.

If this seems like a fairly reasonable appraoch I can put some additional time in documenting things.

I'm not sure I see why we are adding the fade, or how it would help with addressing the problem. With the current MR it seems like the toolbar changing on a page load is even more visible than before. 🤔

Added the 2998451-toolbar-flicker-v2 Merge Request, which is similar to 2998451-toolbar-rendering-creates but reduces the LOC. I want to keep the earlier MR for a bit as I may need to repurpose additional parts of it depending on review feedback.
Comparison: https://youtu.be/lp35EQNvuac
Comparison in slow motion: https://youtu.be/egIUljKPRHE

Addressing the top/left movement when a tray pops in could be seen as its own flicker problem when the tray is lazy loaded (trays such as user or shortcut). With the current flickering fix, you'll see an empty tray during loading which is eventually replaced by the lazy loaded active tray. This means the tray content appears suddenly which is a bit jarring, but unlike HEAD, it does not cause a reflow by adding top or side padding to the content.

If theres dislike of the tray content popping-in, it could be eased by having it fade in, or there being a progress graphic in the tray placeholder. There's logic to allow that in the other MR that isn't currently in 2998451-toolbar-flicker-v2

Another benefit of this fix is if a tray is closed, it does not reopen when a new page is visited.

Nice work! Tested the MR and it definitely reduces the layout shift happening on a page load.

Posted feedback on the code in the MR.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

So when I try to apply the diff from MR 3464 I get

test.patch:665: trailing whitespace.
.toolbar-loading.toolbar-horizontal :not(#{$active_tray}) {$gt} .toolbar-lining {opacity: 0}
warning: 1 line adds whitespace errors.

Not sure if that's going to be an error.

Apply the change and am no longer seeing the flicker.
Manager link still open/closes as expected
Toolbar stays sticky to the top.
Tried on claro, olivero, and a custom uswds theme.

I’ve been using this patch on every site I build for quite some time. Haven’t tested the very latest, nor reviewed recent changes, but very happy to see this at RTBC. Ready to help get it over the line if it needs anything. Thanks!

Left a couple of comments, only one is blocking to me the function setToolbarClassesEarly that leaks to the global scope when it doesn't appear to be necessary.

The other issue are also from the original toolbar so I'd be ok not to mess with it too much since it's been working for a while in contrib, but if that's possible a few code improvements would be welcome.

Couple of nits, otherwise good to go for me.

Works as expected, less jumping around overall, improvement compared to current situation on narrow screen. Code is good,

RTBC for me :)

Thanks for the CR! I posted some feedback on the MR. Tagging with needs security review for the username cookie.

The part of the MR that required security review was de-scoped and moved to 📌 Reduce toolbar user button related browser reflow Fixed

Applied the MR, cleared cached but I'm still seeing the flicker.

I wonder if this is related to a weird issue I'm getting in 9.5 where if I change the active toolbar toggle button ("Manage", etc) to blue to address https://www.drupal.org/project/drupal/issues/3097907 🐛 Active toolbar tray has weak affordance and fails WCAG color criteria Needs work , a few white pixels flicker on and off to the left or right of the button when resizing the window in Firefox.

Issue appears to have been fixed in 10 but not sure how

Re #83 The issue mentioned there was fixed in 🐛 Fix the flicker of the "active" icons when a high-latency user clicks on toolbar items Fixed

Re #82 I updated the issue summary and title to better explain what is being fixed here. While the scope of this issue is specific to the delayed padding changes to make room for trays, I was able to address what was shown in the gif.

This should overall be pretty smooth now, but keep in mind that any lazy loaded trays like shortcuts and user will still have the tray contents popping in shortly after the page loads. Long term it might be nice to have it fade in instead but that would be out of scope here and will probably require modifications to big pipe. The menu buttons in the "manage" tray also come in after the page loads as they're added via JavaScript in a way that can't be done earlier, but I did introduce a fade-in so they show up more gracefully

Here is a video of the results that starts with real time followed by a slow motion version https://youtu.be/zL9IlRLkzTg Even the lazy-loaded trays pop in without it feeling too jerky even though there isn't a fade/transition.

Thank you for clarifying.

Taking that new info in account definitely noticing an improvement clicking to new tabs.

Don't mind marking then.

Like this approach better, and works just as well :)

The change record needs to be updated (title) and information about core adding inline JS and CSS needs to be added for people who have CSP configured. Followups should probably be created so that core adds the content hash by default so make sure it'll work without requiring people to add more lax rules to their CSP config.

Going to the filesystem on each call to hook_page_attachements is a problem. The convenience of having an external file is not worth the performance impact. There are two options that I see:

1. use FileCacheFactory to make sure we don't need to load form filesystem every time.
2. We put it in the PHP and don't bother with an external file.

Some more comments in the MR above :)

I wanted to be sure this did not cause any problems for popular contrib toolbars.

Created 📌 [Drupal 10.1] Flickering sidebar nav width on page load Fixed for Gin Toolbar as this change could introduce new flickering on it. Fortunately, this flickering would only happen in a very specific scenario (a user that switched to Gin Toolbar from not-Gin-Toolbar) and would not appear in any tabs opened after the switch to Gin Toolbar.

I also tested this with Admin Toolbar and it benefits from the improvements here the same way the core toolbar does.

I planned to test the Adminimal Toolbar as well but it only supports up to 9x.

Gave another pass for the MR and confirmed that all of the feedback has been addressed.

Updated credits

There was a leftover toolbarUserName in toolbar.js, fixed on commit.

Committed 34b0bcd and pushed to 10.1.x. Thanks!

I published the change record.

I believe I found a regression from this issue. Documented (and patch posted) in 🐛 Black bar appears (then disappears) at top of viewport when navigating Fixed

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

This somehow broke the Browsersync javascript:

It seems that the browsersync javascript is trying to replace the <body> tags mentioned in the comment:

// These are classes that toolbar typically adds to , but this code
// executes before the first paint, when is not yet present. The...

If I rewrite the comment to be something like:

// These are classes that toolbar typically adds to the body tag, but this code
// executes before the first paint, when the body tag is not yet present. The

browsersync starts working again.

Should I open a new issue?

For the issue #96 please refer the below patch,
https://www.drupal.org/project/drupal/issues/3355381#comment-15196226 🐛 Investigate better ways to add anti-flicker JS Fixed

The patch fixes the issue and working fine.

I think we should have gone with the standard header: true approach here, added a patch on 🐛 Investigate better ways to add anti-flicker JS Fixed . See also 🐛 Toolbar anti-flicker js shows for anonymous users Closed: duplicate