Sub-issue of #2412945: Determine which additional asset libraries should be in the critical path/loaded i/t header (core/drupal, core/dropbutton).

The .js class:

- is not used very much - only a couple of stylesheets in core reference it and most of that usage looks vestigial
- is easily added by contrib
- triggers a layout invalidation on every page view

It's also the only reason to keep drupal.js in the header, if we decided it was necessary and were actually using it, which itself would be a performance hit.

CommentFileSizeAuthor
#47 Screenshot 2024-06-12 at 10.05.05 AM.png229.72 KBsmustgrave
#39 interdiff-38_39.txt366 bytesgauravvvv
#39 2413847-39.patch39.07 KBgauravvvv
#38 2413847-38.patch39.43 KBgauravvvv
#16 Views-Drupal81-after-2413847-9-patch.png13.83 KBxxAlHixx
#14 remove-js-action.png10.13 KBkrishnan.n
#4 remove-js-css-class-2413847-4.patch20.2 KBmanuel garcia
#1 js-class-usage.txt13.25 KBmanuel garcia

Issue fork drupal-2413847

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

manuel garcia’s picture

manuel garcia commented
Issue tags: +frontend performance
StatusFileSize
new js-class-usage.txt13.25 KB

This looks to be a bit more work than we had initially had thought, but we should be able to work through it.

Attached the result of $ grep -r --include="*.css" ".js " . (inside core directory).

Most of the results are for dropbutton, or dropbutton related.

My suggestion is for dropbutton js to add its own dropbutton-active class to all elements it affects, and then change the CSS selectors based on that.

The rest of the files are:

  • modules/views_ui/css/views_ui.admin.css
  • modules/views_ui/css/views_ui.admin.theme.css
  • modules/system/css/system.module.css
  • modules/system/css/system.maintenance.css
  • modules/locale/css/locale.admin.css
  • modules/book/css/book.admin.css
  • modules/color/css/color.admin.css
  • themes/bartik/color/preview.cs
manuel garcia’s picture

manuel garcia commented

Let me correct myself... dropbutton already adds an activation class, dropbutton-processed.

manuel garcia’s picture

manuel garcia commented
Assigned: Unassigned » manuel garcia
manuel garcia’s picture

manuel garcia commented
Assigned: manuel garcia » Unassigned
Status: Active » Needs work
StatusFileSize
new remove-js-css-class-2413847-4.patch20.2 KB

Modernizr will still add a js class to the <html> element.

Here is some progress, I have worked on the changes for dropbutton only, tested this on the views admin page and on node/add/article.

Still to do:

  • Review what's done, and test dropbutton elsewhere for visual regressions.
  • The rest of the files listed on #1.
damien tournoud’s picture

damien tournoud commented

This class is there for a reason, to avoid flashing on the rendering of JS-based components. The only thing that needs to be in the header is the setting of this class. Setting this in the head will trigger layout invalidation, but given that there has been nothing to layout at that point, I don't see why it would hurt.

fabianx’s picture

fabianx commented

And I think we could seriously do this as inline-JS so that we don't need to load any file just for that part in the header.

damien tournoud’s picture

damien tournoud commented

@Fabianx: absolutely, setting the class with a simple inline JS would be perfectly fine.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +Needs reroll
prashant.c’s picture

prashant.c
he/him
Primary language English
Location Dharamshala
commented
StatusFileSize
new remove-js-css-class-2413847-9.patch30.82 KB

Last submitted path failed applying.
Submitting new patch after replacing all the instances of .js class.

prashant.c’s picture

prashant.c
he/him
Primary language English
Location Dharamshala
commented
Status: Needs work » Needs review

The last submitted patch, 4: remove-js-css-class-2413847-4.patch, failed testing.

Jeff Burnz’s picture

Jeff Burnz commented
Status: Needs review » Needs work

AFAICT inline JS is out if we want to allow Drupal to be CSP compliant, that said I don't see the point to removing this, I still use js/no-js classes for various things, although it is true I tend to add/remove more specific classes with JS for my fallback styles. I would agree with catch's characterisation of this being "vestigial", just not sure we're ready to let go just yet, so agree with #5.

krishnan.n’s picture

krishnan.n commented

Applied patch:
- Checked a few of the buttons, drop-down, blocks, and other places where .js was removed, looks fine.
- appears to be present in a couple of more files

core$ grep -r "^.js " *
modules/color/css/color.admin.css:.js .color-preview {
modules/locale/css/locale.admin.css:.js .locale-translation-update__wrapper {
themes/stable/css/system/system.maintenance.css:.js #edit-submit-connection {
themes/stable/css/system/system.maintenance.css:.js #edit-submit-process {

krishnan.n’s picture

krishnan.n commented
StatusFileSize
new remove-js-action.png10.13 KB

Applying this patch breaks the "save and publish" button on create page: content -> add content -> basic page. Please see attached image. NB: tend to agree with #5, #12

Jeff Burnz’s picture

Jeff Burnz commented

#9 I just noticed my previous comment removed your patch and changed the status, this was not intentional at all. I don't know why that happened. My apologies.

xxAlHixx’s picture

xxAlHixx commented
StatusFileSize
new Views-Drupal81-after-2413847-9-patch.png13.83 KB

I've applied patch remove-js-css-class-2413847-9.patch and can confirm that issue with "save and publish" from #14 exists and in Views UI issue with Operations buttons appear too.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +SprintWeekend2016
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue summary: View changes
Issue tags: +Needs manual testing

Though there is a problem with the last patch busting the RTL

+++ b/core/themes/stable/css/views_ui/views_ui.admin.theme.css
@@ -761,29 +761,29 @@ td.group-title {
-[dir="rtl"].js .dropbutton-wrapper.dropbutton-multiple.open .dropbutton-action:first-child a {
+.dropbutton-wrapper.dropbutton-multiple.open .dropbutton-action:first-child a {

Dropped the language prefix on a lot of these.

andriyun’s picture

andriyun commented
Issue tags: -Needs reroll

Patch #9 is applicable to 8.1.x branch.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

ramya balasubramanian’s picture

ramya balasubramanian commented
Assigned: Unassigned » ramya balasubramanian
ramya balasubramanian’s picture

ramya balasubramanian commented

Hi @andriyun,
According to #13, some files are missing and it is affecting some 'Save&Publish' button and dropdown buttons. Before continuing this ticket, please let me know whether we need to exclude the 'Save and Dropdown' button designs.

ramya balasubramanian’s picture

ramya balasubramanian commented
Assigned: ramya balasubramanian » Unassigned

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 2413847-38.patch39.43 KB

I have attached patch for 11.x, please review
Also the issue reported in #16 & 14 are no longer reproducible with this patch. thanks

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 2413847-39.patch39.07 KB
new interdiff-38_39.txt366 bytes

Fixed the build errors.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Think there's some BC to consider before just fully removing no?

yash.rode made their first commit to this issue’s fork.

yash.rode opened merge request !8370

yash.rode’s picture

yash.rode commented

Converted to MR ^ As patch testing will be stopped form 1st july.

catch’s picture

catch
he/him
Primary language English
commented
Title: Remove .js class from core » Remove usages of .js class from core
Status: Needs work » Needs review

The MR is only removing usages of the class, not the class itself, so there is no bc issue with the current MR. I think we could keep this issue to remove usages and open a second issue to stop adding the class.

I do think we need some kind of bc layer here because CSS written to use the js class will just stop working without it.

The class is set in:

drupal.init.js

document.documentElement.className += ' js';

One possibility for bc would be to add a new module to core, which does exactly this via altering a js file into the core library definition, enable that module by default in a post update, but have it uninstalled for new sites (or sites that uninstall it). We could then deprecate that module in Drupal 11 and make it obsolete in Drupal 12. Just writing that out makes me think two issues would make this easier to get done rather than trying to tackle both here.

smustgrave’s picture

smustgrave commented
Issue summary: View changes
Status: Needs review » Needs work

Sounds like a good plan, updated issue summary

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
Status: Needs work » Needs review

@smustgrave in #44 I was suggesting to do that plan in a new issue, not in this one because I think it'll be a bit complex to attempt both. I've opened #3453951: Deprecate setting the .js class in core in preparation for removal and adjusted the issue summary.

smustgrave’s picture

smustgrave commented
StatusFileSize
new Screenshot 2024-06-12 at 10.05.05 AM.png229.72 KB

Oh I had read it that maybe it could be included here but cool.

So in regards to this ticket of removing, did a search for .js in css files and found more instances

more

smustgrave’s picture

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

mithun s
Primary language English
Location Bangalore
commented
Assigned: Unassigned » mithun s

Currently looking into this.

mithun s’s picture

mithun s
Primary language English
Location Bangalore
commented
Assigned: mithun s » Unassigned
Status: Needs work » Needs review

Removed the pending .js class from the css files and added a commit. Please review.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue summary: View changes

I'm very confused about this change. See my MR comment, js might not be used much directly, but the js-show and js-hide classes have 10+ usages in core alone and the proposed change completely breaks the functionality of that.

smustgrave’s picture

smustgrave commented

How come issue summary was removed?

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3167377: Rewrite jQuery .show() and .hide()
catch’s picture

catch
he/him
Primary language English
commented

.js-show and .js-hide should not be included here.

Those are now implemented without having to execute or load any JavaScript at all:

/**
 * Use the scripting media features for modern browsers to reduce layout shifts.
 */
@media (scripting: enabled) {
  /* Extra specificity to override previous selector. */
  .js-hide.js-hide {
    display: none;
  }
  .js-show {
    display: block;
  }

from <code>core/modules/system/css/components/js.module.css

i.e. we have a CSS-only implementation for .js-show and .js-hide, the .js class is only added so that .js .js-hide works, which is not necessary with the media query.

So this issue should be about removing the JavaScript implementation and the .js class, leaving the CSS-only implementation intact.

smustgrave’s picture

smustgrave commented

Hiding patches for clarity.

I'm not seeing the removal of js-hide or js-show in the MR?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

@smustgrave it's https://git.drupalcode.org/project/drupal/-/merge_requests/8370/diffs#e8... as pointed out by @berdir in the MR

That is the thing that makes .js-hide work on browsers that don't support @media (scripting: enabled), which used to be all of them when it was added, but is not necessary now (except there are a couple of 'unknowns' on https://caniuse.com/?search=media%20scripting%3A)

This file should have been the only ever usage of the class in core or anywhere else, because the whole point of it is to make .js-hide and .js-show work, not for styling things independently of those, but it's clear that usages have crept in over time.

That CSS would need to be handled in #3453951: Deprecate setting the .js class in core in preparation for removal but should be left here.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.