The dropbutton component shows its menu when JavaScript is disabled, but then quickly hides it when JS loads.

This results in a layout shift of the page, which provides a suboptimal user experience. IMHO, it makes it seem fairly janky.

This is easily viewable on the /admin/content page:

Google also highly prioritizes elimination of layout shifts, and includes a "Cumulative Layout Shift" metric within its Core Web Vitals. See

What can be done

We want to still support non-JS admin users (at least for the time being).

We fix this issue by adding a <noscript> tag to the document head, and then adding a <link> tag pointing to non-JS dropbutton styles that we migrate to a separate CSS file.

CommentFileSizeAuthor
#9 interdiff-5-9.txt2.04 KBmherchel
#9 3361315-9.patch9.47 KBmherchel
#5 3361315-5.patch10.02 KBmherchel
#3 3361315.patch10.17 KBmherchel
layout-shift.gif196.34 KBmherchel

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Title: Dropbutton quickly shows/hides its menu on pageload causing content shift » Dropbutton quickly shows/hides its menu on pageload causing layout shift
Issue summary: View changes
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Component: other » Claro theme
StatusFileSize
new 3361315.patch10.17 KB

We fixed a similar issue with the mobile-friendly menus within Olivero by adding a noscript_styles variable to the html.html.twig.

This patch follows the same pattern.

That being said, it would be nice if libraries or themes/modules had an easier way to add noscript styles to a page. Maybe something like this in a *.libraries.yml file could be a followup:

drupal.dropbutton:
  css:
    component:
      misc/dropbutton/dropbutton-noscript.css: { noscript: true }
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Active » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3361315-5.patch10.02 KB
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Was able to confirm the shift as seen in the gif.

Applied the patch and no longer seeing it.

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.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work

I've been wanting this addressed for a long time!

Only feedback is pretty minor.

  1. +++ b/core/themes/claro/claro.theme
@@ -39,6 +39,28 @@ function claro_theme_suggestions_details_alter(&$suggestions, $variables) {
+function claro_preprocess_html(&$variables) {
+  $theme_path = \Drupal::request()->getBasePath() . '/' . \Drupal::service('extension.list.theme')->getPath('claro');
+  $query_string = \Drupal::state()->get('system.css_js_query_string') ?: '0';

    If this goes in a page_attachments_alter there isn't the need to change html.html.twig, and its leveraging an existing API to add this 

     function claro_page_attachments_alter(array &$attachments) {
  $theme_path = \Drupal::request()->getBasePath() . '/' . \Drupal::service('extension.list.theme')->getPath('claro');
  $query_string = \Drupal::state()->get('system.css_js_query_string') ?: '0';

  $attachments['#attached']['html_head'][] = [
    [
      '#tag' => 'link',
      '#noscript' => TRUE,
      '#attributes' => [
        'rel' => 'stylesheet',
        'href' => $theme_path . '/css/components/dropbutton-noscript.css?' . $query_string,
      ],
    ],
    'dropbutton_noscript',
  ];
}

    It's possible this was tried and ruled out in Olivero but couldn't quickly find the relevant issue as it was implemented when Olivero was contrib. If the reasoning is already in the issue and there's a good reason to use the existing approach, link to the issue with the explanation, address item 2 (which is only needed if you keep preprocess_html) and fine to re-rtbc. 

  2. +++ b/core/themes/claro/templates/classy/layout/html.html.twig
@@ -38,6 +38,7 @@
+    {{ noscript_styles }}

    If my attachments_alter suggestion isn't implemented,and this change still happens, then this template file should be moved to a new directory. Any theme that used to have (the now removed) Classy as a subtheme had the classy templates it used copied to a /classy subdirectory to help distinguish formerly-classy from templates specific to the theme. Once we change it, it's effectively overridden and belongs to Claro, etc.

    This isn't particularly apparent in Drupal 10 as the test coverage that would let you know the move was needed got removed with Classy

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3361315-9.patch9.47 KB
new interdiff-5-9.txt2.04 KB

To be honest, I'm not sure why we implemented it the way we did in Olivero. I think it was a situation where the code morphed a bunch and then it ended up there (which works well). That being said, I like this solution better because it doesn't need to modify the HTML template.

New patch and interdiff attached. Thank you :)

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Reviewed & tested by the community

I just tested this and it work perfect. Also, since @bnjmnm's feedback has been addressed, I'm moving this back to RTBC.

  • lauriii committed c5604c9d on 11.x 
    Issue #3361315 by mherchel, smustgrave, bnjmnm, ckrina: Dropbutton...

  • lauriii committed fa2193ff on 10.1.x 
    Issue #3361315 by mherchel, smustgrave, bnjmnm, ckrina: Dropbutton...
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

Woah! I've wanted to work on this so many times since we first implemented the Claro dropbuttons. Thank you @mherchel & all! 💯

Committed c5604c9 and pushed to 11.x. Also cherry-picked to 10.1.x. Thanks! 🎉

Status: Fixed » Closed (fixed)

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