When a path is initially navigated that utilizes a table filter (like the modules or permissions pages). The page is initially viewable without the table filter. Then the JavaScript kicks in and modifies the DOM so that the page layout shifts as the table filter appears.

This issue is intended to mitigate or remove this jankiness (as much as possible).

Plan

The plan is to use the new at-media scripting feature to reduce the jank as much as possible in browsers that support this. For older browsers, the current behavior will still persist.

Testing

Note that the at-media feature is not supported in the latest version of Chrome (119). It is supported in the next version of Chrome (120), Safari 17, and Firefox. So, if you're testing on Chrome, please verify that you're on 120 or later (you can use Chrome Canary if you'd like).

  1. Navigate to the page in question (in this case /admin/modules), and open developer tools. Go to the network tab and set throttling to "Fast 3G") and ensure that "Disable cache" is checked.
  2. Reload the browser and notice how the "filter" pops in and shifts the layout.
  3. Apply the patch.
  4. Test again and note how the "filter" is there on page load and does not "pop in".
  5. Test the page without JavaScript (you can disable this in dev tools)
  6. Test the page on an older browser that doesn't support this new feature. The behavior should not change.
  7. Test the page on other browsers to ensure it works as expected.
CommentFileSizeAuthor
#2 3404218.patch591 bytesmherchel
table-filter.mp41.83 MBmherchel

Issue fork drupal-3404218

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

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3404218.patch591 bytes
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

@mherchel is there an additional step? I'm on Version 119.0.6045.159 (Official Build) (arm64) and applying the patch I'm not noticing any difference on the page load

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

@mherchel is there an additional step? I'm on Version 119.0.6045.159 (Official Build) (arm64) and applying the patch I'm not noticing any difference on the page load

Ha! You're just like me and skipped over part of the testing steps (I do this all the time). Here's the part that you missed 🤣🤣🤣

Note that the at-media feature is not supported in the latest version of Chrome (119). It is supported in the next version of Chrome (120), Safari 17, and Firefox. So, if you're testing on Chrome, please verify that you're on 120 or later (you can use Chrome Canary if you'd like).

smustgrave’s picture

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

Tested on firefox and see what you mean now.

Wonder if a meta should be opened for other locations this could be used?

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Great minds think alike :D
#3404214: META: Reduce / eliminate “jank” (layout shifts) within Drupal’s admin UI

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/system/css/components/js.module.css
@@ -20,3 +20,17 @@
+  [class] .js-hide {

    Why not .js-hide.js-hide like we do usually when we need to increase specificity? 

  2. +++ b/core/modules/system/css/components/js.module.css
@@ -20,3 +20,17 @@
+  .js-show {

    Shouldn't we add the extra specificity here too?

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

djsagar opened merge request !5587

djsagar’s picture

djsagar commented
Status: Needs work » Needs review

HI lauriii,

I tried to changes as per your comment, kindly review.

Thanks!

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

djsagar’s picture

djsagar commented
Status: Needs work » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Why not .js-hide.js-hide like we do usually when we need to increase specificity?

No specific reason. Attribute has the same specificity as a class, so 🤷‍♂️. Not sure if there's a preference.

Shouldn't we add the extra specificity here too?

Nope. We don't need it. The extra specificity is only for when JS is enabled, and therefore the current (previous) ruleset works file. We only need to take into account for situations where JavaScript is enabled but the .js class is not yet present.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Ah, you're right @mherchel! I fixed it.

smustgrave’s picture

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

Restoring status.

johnpitcairn’s picture

johnpitcairn commented

Safari 17 is quite a high bar given Apple ties it to specific OS updates (it's the new IE). Any interest in a fallback technique with broader browser support?

  • lauriii committed 3ad9ff98 on 11.x 
    Issue #3404218 by djsagar, mherchel, smustgrave: Table filter creates...

  • lauriii committed 66834db5 on 10.2.x 
    Issue #3404218 by djsagar, mherchel, smustgrave: Table filter creates...

lauriii closed merge request !5587

lauriii’s picture

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

Not sure it's worth using too much time trying to invent another solution for this since it's a perceived performance improvement for a problem that has existed for years in Drupal. If you are interested in that and want to propose something, I think it would be fine to do so in the meta issue.

Committed 3ad9ff9 and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

johnpitcairn’s picture

johnpitcairn commented

Fair enough. No on second thought it's a problem that will go away eventually by itself with these fixes in.

Status: Fixed » Closed (fixed)

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

catch’s picture

catch
he/him
Primary language English
commented

Follow-up #3514748: Remove legacy browser support from js.module.css.