Postponed on #2998451: Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays

Problem/Motivation

#2998451: Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays 👈 this issue included the addition of inline JavaScript to <head> so it could load before anything on the page was rendered. This resulted in a less visually jarring toolbar initialization.

The JS is currently added via toolbar_page_attachments(), adding a script tag to $page['#attached']['html_head']. Unlike most Drupal JavaScript, the JS code is not in a dedicated file, but created as a HEREDOC variable. It's not the most elegant DX, but it got the job done. We'd like to find a better approach.

Some things that were tried:

  • 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
  • Having the JS be in a dedicated file, but not part of a library. PHP would load the file in toolbar_page_attachments(), write the contents to a string, which would ultimately become the content of the newly added script tag. This was decided against as it would result in loading a file from the filesystem on every request. This could potentially be addressed by using FileCacheFactory, but best explored in a dedicated issue such as this instead of blocking the anti-flicker enhancements provided by #2998451

Steps to reproduce

Proposed resolution

Move toolbar.anti-flicker.js back to a regular library definition with header: true

While this is an additionally http request, it is only an http request for users with access to the toolbar, unlike the current implementation which loads inline JavaScript for users without access to the toolbar #3379245: Toolbar anti-flicker js shows for anonymous users.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

JavaScript which was added inline in Drupal 10.1.0 to prevent toolbar flickering has been moved to a file in a new toolbar.anti_flicker library. This resolves reported bugs and CSP issues due to the JavaScript originally being inline.

CommentFileSizeAuthor
#31 3355381-31.patch8.35 KBcatch
#31 interdiff.txt681 bytescatch
#17 3355381.patch7.69 KBcatch
#15 3355381.patch7.29 KBcatch
#13 toolbar-antiflicker-clean-comments.patch1.05 KBdrewcking
#3 Toiolbar.module-ln69.png147.83 KBstewest

Comments

bnjmnm created an issue. See original summary.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

stewest’s picture

stewest
Location Wellington
commented
Version: 11.x-dev » 10.1.x-dev
StatusFileSize
new Toiolbar.module-ln69.png147.83 KB

We're noticing that the comments from Line 68 web/core/modules/toolbar/toolbar.module are showing when logged out.

an image showing comments from toolbar module line 68

Had to add this to themes/THEMENAME/includes/page.inc


function THEME_page_attachments_alter(array &$attachments) {
  if(\Drupal::currentUser()->isAnonymous()) {
    $attachments['#attached']['html_head'] = array_filter($attachments['#attached']['html_head'], function($item) {
      return $item[1] !== 'anti_flicker_js';
    });
  }
}
kevinquillen’s picture

kevinquillen commented

Same as #3. This should only load for appropriate users.

drewcking’s picture

drewcking commented

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.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

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.

fngatia’s picture

fngatia commented

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?

<script type="text/javascript" data-toolbar-anti-flicker-loading>(function() {
  const toolbarState = sessionStorage.getItem('Drupal.toolbar.toolbarState')
    ? JSON.parse(sessionStorage.getItem('Drupal.toolbar.toolbarState'))
    : false;
  // These are classes that toolbar typically adds to <body><script id="__bs_script__">//<![CDATA[       <-----HERE
  (function() {
    try {
      var script = document.createElement('script');
      if ('async') {
        script.async = true;
      }
      script.src = '/browser-sync/browser-sync-client.js?v=2.29.3'.replace("HOST", location.hostname);
      if (document.body) {
        document.body.appendChild(script);
      } else if (document.head) {
        document.head.appendChild(script);
      }
    } catch (e) {
      console.error("Browsersync: could not append script tag", e);
    }
  })()
//]]></script>
, but this code
  // executes before the first paint, when <body> is not yet present. The
  // classes are added to <html> 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);
})();</script>
ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

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 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?

lucasvm’s picture

lucasvm commented

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';
    });
  }

Still present on some admin pages.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Title: [PP-1] Investigate better ways to add anti-flicker JS » Investigate better ways to add anti-flicker JS

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 #2998451: Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays. 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.

danny englander’s picture

danny englander
(he/him)
commented

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. (Logged in and logged out.)

catch’s picture

catch
he/him
Primary language English
commented

I just found #3379245: Toolbar anti-flicker js shows for anonymous users 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 #3379245: Toolbar anti-flicker js shows for anonymous users 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: #3375181: Deprecate the touchevents JavaScript library and update CSS. But until that's done it won't be an extra http request in most cases.

drewcking’s picture

drewcking commented
StatusFileSize
new toolbar-antiflicker-clean-comments.patch1.05 KB

This patch tweaks the first JS comment block to replace the HTML tags with words, e.g., <body> becomes "the body tag".

manikandank03’s picture

manikandank03 commented

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.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
Priority: Normal » Major
Status: Active » Needs review
StatusFileSize
new 3355381.patch7.29 KB

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 #3379245: Toolbar anti-flicker js shows for anonymous users, 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.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Needs work 
+++ b/core/modules/toolbar/toolbar.libraries.yml
@@ -50,3 +50,9 @@ toolbar.escapeAdmin:
+toolbar.anti-flicker:

Shouldn't we load this library somewhere? 🤔

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 3355381.patch7.69 KB

Yes that would help - adding the dependency and fixing some cs issues.

drewcking’s picture

drewcking commented

Thanks catch, patch in #17 works for me on a fresh install of 10.1.2 when loading the site through browser-sync.

gapple’s picture

gapple
he/they
Primary language English
commented

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', rather than the more complex and less obvious method of removing the script via a hook.

lmoeni’s picture

lmoeni
she/her
commented

I tested patch #17 on Drupal 10.1.2 and can confirm that it works.
Thanks for the patch!

lmoeni’s picture

lmoeni
she/her
commented
Status: Needs review » Reviewed & tested by the community
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Category: Task » Bug report
Issue tags: +Needs change record

I don't think we were able to anticipate all of the downsides of the chosen solution in #3379245: Toolbar anti-flicker js shows for anonymous users. 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).

catch’s picture

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

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.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

I thought we could file a CR to mention that the inline JS has been removed because some modules, like #3358392: Disable toolbar anti-flicker inline JS 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 😅

catch’s picture

catch
he/him
Primary language English
commented

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?

  • lauriii committed 56e2560a on 11.x 
    Issue #3355381 by catch, drewcking, stewest, bnjmnm, lmoeni, fngatia,...

  • lauriii committed e36422dd on 10.1.x 
    Issue #3355381 by catch, drewcking, stewest, bnjmnm, lmoeni, fngatia,...
lauriii’s picture

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

Committed 56e2560 and pushed to 11.x. Cherry picked to 10.1.x as a major bug fix to address the regression. Thanks!

  • lauriii committed b8ad5f57 on 11.x 
    Revert "Issue #3355381 by catch, drewcking, stewest, bnjmnm, lmoeni,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Fixed » Needs work

Looks like this broke HEAD because #17 was only ran against 10.1.x and \Drupal\FunctionalJavascriptTests\PerformanceTestBase based tests are only in 11.x. 🤦‍♂️

catch’s picture

catch
he/him
Primary language English
commented
Version: 10.1.x-dev » 11.x-dev
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new interdiff.txt681 bytes
new 3355381-31.patch8.35 KB

Since this is a one digit change, moving back to RTBC - will still need #17 for 10.1.x

  • lauriii committed 3b8df679 on 11.x 
    Issue #3355381 by catch, drewcking, stewest, lauriii, bnjmnm, lmoeni,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 3b8df67 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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