- Issue created by @bnjmnm
- 🇳🇿New Zealand stewest Wellington
We're noticing that the comments from Line 68 web/core/modules/toolbar/toolbar.module are showing when logged out.
- 🇺🇸United States kevinquillen
Same as #3. This should only load for appropriate users.
- 🇺🇸United States drewcking
Is there a need for these comments to be in the javascript? It makes sense to move them above this heredoc so that they are PHP comments instead of JS comments.
- 🇺🇸United States bnjmnm Ann Arbor, MI
Is there a need for these comments to be in the javascript? It makes sense to move them above this heredoc so that they are PHP comments instead of JS comments.
Good call. No need. I think the comments presence in the heredoc was due to that block of code originally being in a JS file and we didn't think to adjust for it going into the heredoc.
- 🇰🇪Kenya fngatia Eldoret
Am also having problems with the comments as indicated above. My custom js are injecting data/code into the tag in the comments instead of the HTML one. Can the tags wrap tag be avoided?
```
(function() { const toolbarState = sessionStorage.getItem('Drupal.toolbar.toolbarState') ? JSON.parse(sessionStorage.getItem('Drupal.toolbar.toolbarState')) : false; // These are classes that toolbar typically adds to //, but this code
// executes before the first paint, when is not yet present. The
// classes are added to so styling immediately reflects the current
// toolbar state. The classes are removed after the toolbar completes
// initialization.
const classesToAdd = ['toolbar-loading', 'toolbar-anti-flicker'];
if (toolbarState) {
const {
orientation,
hasActiveTab,
isFixed,
activeTray,
activeTabId,
isOriented,
userButtonMinWidth
} = toolbarState;classesToAdd.push(
orientation ? `toolbar-` + orientation + `` : 'toolbar-horizontal',
);
if (hasActiveTab !== false) {
classesToAdd.push('toolbar-tray-open');
}
if (isFixed) {
classesToAdd.push('toolbar-fixed');
}
if (isOriented) {
classesToAdd.push('toolbar-oriented');
}if (activeTray) {
// These styles are added so the active tab/tray styles are present
// immediately instead of "flickering" on as the toolbar initializes. In
// instances where a tray is lazy loaded, these styles facilitate the
// lazy loaded tray appearing gracefully and without reflow.
const styleContent = `
.toolbar-loading #` + activeTabId + ` {
background-image: linear-gradient(rgba(255, 255, 255, 0.25) 20%, transparent 200%);
}
.toolbar-loading #` + activeTabId + `-tray {
display: block; box-shadow: -1px 0 5px 2px rgb(0 0 0 / 33%);
border-right: 1px solid #aaa; background-color: #f5f5f5;
z-index: 0;
}
.toolbar-loading.toolbar-vertical.toolbar-tray-open #` + activeTabId + `-tray {
width: 15rem; height: 100vh;
}
.toolbar-loading.toolbar-horizontal :not(#` + activeTray + `) > .toolbar-lining {opacity: 0}`;const style = document.createElement('style');
style.textContent = styleContent;
style.setAttribute('data-toolbar-anti-flicker-loading', true);
document.querySelector('head').appendChild(style);if (userButtonMinWidth) {
const userButtonStyle = document.createElement('style');
userButtonStyle.textContent = `#toolbar-item-user {min-width: ` + userButtonMinWidth +`px;}`
document.querySelector('head').appendChild(userButtonStyle);
}
}
}
document.querySelector('html').classList.add(...classesToAdd);
})();```
- 🇨🇦Canada ambient.impact Toronto
I'm a bit baffled as to why this made it into a stable release because it feels like this is not a good way to handle this. Core intentionally moved away from inline JavaScript years ago, and this is non-usable on sites with Content Security Policy 📌 Disable toolbar anti-flicker inline JS Fixed set up. Just putting this into render-blocking external JavaScript and CSS files linked from the
<head>should accomplish the same thing, in that browsers should prioritize downloading that before starting to render the page, should it not? - 🇦🇷Argentina lucasvm
I can confirm this issue still exists on Drupal 10.1 i got fixed by adding this to the .theme file instead of creating a new page inc
function THEME_page_attachments_alter(&$variables) { if(\Drupal::currentUser()->isAnonymous()) { $variables['#attached']['html_head'] = array_filter($variables['#attached']['html_head'], function($item) { return $item[1] !== 'anti_flicker_js'; }); } - 🇺🇸United States bnjmnm Ann Arbor, MI
I'm a bit baffled as to why this made it into a stable release because it feels like this is not a good way to handle this
The reasons why it was not done this way was discussed in the issue where it was implemented 🐛 Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays Fixed . There's was also an acknowledgement that it might not be the ideal approach, thus the existence of this issue.
I look forward to seeing MRs or patches that improve it, as there seems to be enough interest in this to result in some motivation.
- 🇺🇸United States Danny Englander San Diego
I just got hit with this one. My use case is, I have Laveral Mix and BrowserSync running which injects some inline JS to hot reload the page when the CSS has changed from my Sass build. The JS from Laveral Mix inserted itself right within the anti-flicker script so as to break the page just like the screen capture in #3.
- 🇬🇧United Kingdom catch
I just found 🐛 Toolbar anti-flicker js shows for anonymous users Closed: duplicate and in the process of searching for duplicates found this issue.
I wasn't involved in the previous issue, but I think we should do;
Having this JS be part of a library with header: true, which was decided against as it adds an additional JS request, even when assets are aggregated
Once 🐛 Toolbar anti-flicker js shows for anonymous users Closed: duplicate is fixed we'd only be adding the JavaScript for users with access to the toolbar, these are authenticated with admin or semi-admin permissions who will mostly be browsing the site with the file already browser cached. So for me it is not worth the workaround to save the cold cache http request for those users.
The other reason is that we already have a library with render-blocking js in the header, although I opened an issue to remove that here: 📌 Re-evaluate touchevents detection Needs work . But until that's done it won't be an extra http request in most cases.
- 🇺🇸United States drewcking
This patch tweaks the first JS comment block to replace the HTML tags with words, e.g.,
<body>becomes "the body tag". - 🇮🇳India manikandank03 Tamil Nadu
Hello drewcking,
Thanks for the patch, in my case I faced the same issue and did the same changes as custom patch and fix the issue.
Here I confirm the patch #13 fix the issue and works fine on Drupal 10.1.1.
- Status changed to Needs review
over 1 year ago 9:31am 21 August 2023 - last update
over 1 year ago Custom Commands Failed - 🇬🇧United Kingdom catch
Here's a patch, moves it back to a file, uses the same approach as https://git.drupalcode.org/project/drupal/-/merge_requests/3464/diffs?co...
I'm bumping this to major:
1. The comments in the inline js are breaking sites as seen above.
2. The current inline js is needlessly rendered for anonymous users 🐛 Toolbar anti-flicker js shows for anonymous users Closed: duplicate , this not only adds cruft to all those pages, but because the js relies on session storage if you're logged in then log out, you can still get the toolbar classes applied.
Both of those issues could be individually fixed with the current implementation, but I don't think the special-casing is really necessary.
This will result in an extra render-blocking HTTP request as discussed in the original issue, but that extra HTTP request is only for users with access to the toolbar, who are extremely likely to have the file cached.
- Status changed to Needs work
over 1 year ago 9:43am 21 August 2023 - 🇫🇮Finland lauriii Finland
+++ b/core/modules/toolbar/toolbar.libraries.yml @@ -50,3 +50,9 @@ toolbar.escapeAdmin: +toolbar.anti-flicker:Shouldn't we load this library somewhere? 🤔
- Status changed to Needs review
over 1 year ago 10:25am 21 August 2023 - last update
over 1 year ago 29,469 pass - 🇬🇧United Kingdom catch
Yes that would help - adding the dependency and fixing some cs issues.
- 🇺🇸United States drewcking
Thanks catch, patch in #17 works for me on a fresh install of 10.1.2 when loading the site through browser-sync.
- 🇨🇦Canada gapple
AFAIK browsers' preload scanner will still request the JS file while rendering is blocked for CSS parsing. HTTP 1.1 may hit a limit on concurrent requests that could delay the request for an additional file (KeepAlive should mitigate overhead of an additional connection), but that shouldn't be an issue with HTTP/2.
Since the JS comes right after the CSS, which already blocks rendering, users won't see a FOUC and the execution time of the script seems like more of a concern than the time for an additional HTTP request on the first visit to a page its needed.----
The CSP module currently removes the inline script.
I think it's best to avoid an inline script that would nudge less aware site-builders to add an'unsafe-inline'exception on responses for authenticated users, rather than the more complex and less obvious method of removing the script via a hook. - 🇩🇪Germany lmoeni
I tested patch #17 on Drupal 10.1.2 and can confirm that it works.
Thanks for the patch! - Status changed to RTBC
about 1 year ago 5:32am 24 August 2023 - last update
about 1 year ago 29,469 pass - 🇫🇮Finland lauriii Finland
I don't think we were able to anticipate all of the downsides of the chosen solution in 🐛 Toolbar anti-flicker js shows for anonymous users Closed: duplicate . It seems fine to change that decision and go with the option that is slightly less performant in some use cases due to the extra request, but less likely to run into challenges.
I think we should create a new CR and update https://www.drupal.org/node/3350272 → .
Also wondering if we should backport this or not. It seems like we should backport this, but the fix is adding a new library. I think that should be fine given that it's addressing a bug, and even when the JavaScript is missing, the site remains functional (users just have to suffer from flickering until next cache clear).
- 🇬🇧United Kingdom catch
I'm not sure what to write for a new change record, could we maybe just update https://www.drupal.org/node/3350272 → ?
Something like
From Drupal 10.1.0 to 10.1.3:
[existing text]Since Drupal 10.1.4, this JavaScript has been moved to a new toolbar.anti-flicker library which is a dependency of the main toolbar library and will no longer be rendered inline.
Then a release note. I've added a releases notes snippet.
- 🇫🇮Finland lauriii Finland
I thought we could file a CR to mention that the inline JS has been removed because some modules, like 📌 Disable toolbar anti-flicker inline JS Fixed have made changes to integrate with it (or actually remove it). Not too fussed about it but feels like we file CRs for less impactful changes occasionally 😅
- 🇬🇧United Kingdom catch
Having written #23 I figured out what we could write for a new one, so here it is: https://www.drupal.org/node/3383056 →
Then I think we could do the same text update to the original CR as proposed in #23 just adding a link to the CR?
- Status changed to Fixed
about 1 year ago 8:56am 24 August 2023 - Status changed to Needs work
about 1 year ago 11:18am 24 August 2023 - Status changed to RTBC
about 1 year ago 4:52pm 24 August 2023 51:11 32:41 Running- 🇬🇧United Kingdom catch
Since this is a one digit change, moving back to RTBC - will still need #17 for 10.1.x
- Status changed to Fixed
about 1 year ago 6:21am 25 August 2023 Automatically closed - issue fixed for 2 weeks with no activity.