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.

Comments

Manuel Garcia’s picture

Issue tags: +frontend performance
FileSize
13.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

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

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Active » Needs work
FileSize
20.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

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

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

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

joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +Needs reroll
Prashant.c’s picture

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

Prashant.c’s picture

Status: Needs work » Needs review

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

Jeff Burnz’s picture

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

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

FileSize
10.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

#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

FileSize
13.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

Issue tags: +SprintWeekend2016
joelpittet’s picture

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

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.