Problem/Motivation

As mentioned in the parent issue #3238306: [META] Where possible, refactor existing jQuery uses to vanillaJS to reduce jQuery footprint, we are working towards reducing our jQuery footprint. One of the ways to accomplish this is to reduce the number of jQuery features used in Drupal core. We have added eslint rules that identify specific features and fail tests when those features are in use.

There are (or will be) individual issues for each jQuery-use eslint rule. This one is specific to jquery/no-css, which targets the jQuery css function.

Steps to reproduce

Proposed resolution

Remaining tasks

  • In core/.eslintrc.jquery.json Change "jquery/no-css": 0, to "jquery/no-css": 2, to enable eslint checks for uses of jQuery css(). With this change, you'll be able to see uses of the undesirable jQuery feature by running yarn lint:core-js-passing from the core directory
  • Add the following lines to core/scripts/dev/commit-code-check.sh so the DrupalCI testing script can catch this jQuery usage on all files, not just those which have changed
    # @todo Remove the next chunk of lines before committing. This script only lints
    #  JavaScript files that have changed, so we add this to check all files for
    #  jQuery-specific lint errors.
    cd "$TOP_LEVEL/core"
    node ./node_modules/eslint/bin/eslint.js --quiet --config=.eslintrc.passing.json .
    
    CORRECTJQS=$?
    if [ "$CORRECTJQS" -ne "0" ]; then
      # No need to write any output the node command will do this for us.
      printf "${red}FAILURE ${reset}: unsupported jQuery usage. See errors above."
      STATUS=1
      FINAL_STATUS=1
    fi
    cd $TOP_LEVEL
    # @todo end lines to remove

    Add the block about 10 lines before the end of the file, just before if [[ "$FINAL_STATUS" == "1" ]] && [[ "$DRUPALCI" == "1" ]]; then, then remove it once all the jQuery uses have been refactored.

  • If it's determined to be feasible, refactor those uses of jQuery css() to use Vanilla (native) JavaScript instead.

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3238308

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:

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

Issue summary: View changes

bnjmnm’s picture

Status: Active » Needs review
bnjmnm’s picture

Issue summary: View changes
bnjmnm’s picture

Status: Needs review » Needs work

The Custom Commands Failed in the most recent commit is intentional. This means the DrupalCI testing script is successfully catching the uses of jQuery css that should be refactored to use Vanilla JS. Once those are all addressed, there should no longer be a Custom Commands Failed AND the extra lines in core/scripts/dev/commit-code-check.sh checking for this (as mentioned in issue summary) can be removed.

TLDR: tests will now fail in this MR until jQuery().css() uses are vanillified. Happy refactoring!

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

mstrelan’s picture

Status: Needs work » Needs review
rikki_iki’s picture

Status: Needs review » Needs work
mstrelan’s picture

I'm at a loss here. All tests are passing in https://www.drupal.org/pift-ci-job/2247858 but with one instance of $.css() remaining. There are 14 fails in https://www.drupal.org/pift-ci-job/2248118 but these are passing locally for me.

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.

mstrelan’s picture

Note to self since I don't know when I'll come back to this:

Can reproduce the test fails but adding a 100ms wait before the first fail.

mstrelan’s picture

Status: Needs work » Needs review

yogeshmpawar’s picture

Status: Needs review » Needs work

CS errors.

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.

mstrelan’s picture

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

mstrelan’s picture

Status: Needs work » Needs review
StatusFileSize
new21.62 KB

Gitlab won't let me create a new branch from 10.1.x so uploading a patch. It's a lot simpler now that we removed ckeditor, color, quickedit and bartik, and we don't have to transpile es6.

nod_’s picture

Status: Needs review » Needs work

When there is an object in the css method we can use object assign to have a similar DX:

$something.css({left:0, right:0, top:0});

Object.assign(something.style, {left:0, right:0, top:0});
mstrelan’s picture

Status: Needs work » Needs review
StatusFileSize
new20.81 KB
new4.73 KB

Fixed linting and addressed feedback in #20, assuming this is only necessary where styles are applied to multiple properties. There were some instances where an object was used with a single property, but I have left those as the were in #19.

mstrelan’s picture

StatusFileSize
new20.98 KB
new1.51 KB

I don't know why the nightwatch test failed, so reverting that one to see if it comes back green.

mstrelan’s picture

StatusFileSize
new20.81 KB
new2.53 KB
new4.73 KB

Oh, I accidentally started writing Object.apply instead of Object.assign. Let's try this again.

mstrelan’s picture

StatusFileSize
new20.81 KB
new539 bytes

One more instance I forgot to update. It must be Friday afternoon.

smustgrave’s picture

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

So wasn't super clear how to best test this one.

But I applied patch #24 locally and for a few days I've been using it locally.
Created views
Created content types
Tested on mobile

And nothing seems to break and didn't notice any errors in the console.

If better way please let me know.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 3238308-10-1-24.patch, failed testing. View results

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.

mstrelan’s picture

Status: Needs work » Reviewed & tested by the community

Random fail, setting back to RTBC.

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nod_’s picture

  1. +++ b/core/misc/ajax.js
    @@ -1480,7 +1480,11 @@
    -      $(response.selector).css(response.argument);
    

    maybe this one needs a bit more, jquery handles kebab-case to camelCase transformation of css rules.

    At least in core there are two uses of the CssCommand that uses kebab-case.

    Since this code is aimed at backend folks and all the jQuery niceties are expected, let's exclude this one from the list. We can handle that when we uncouple ajax.js from jquery.

  2. +++ b/core/misc/progress.js
    @@ -74,9 +74,9 @@
    +          [].forEach.call($(this.element).find('div.progress__bar'), (el) => {
    
    +++ b/core/misc/tabledrag.js
    @@ -401,9 +401,13 @@
    +    [].forEach.call($tables.find('.tabledrag-hide'), (el) => {
    ...
    +    [].forEach.call($tables.find('.tabledrag-handle'), (el) => {
    
    @@ -418,9 +422,13 @@
    +    [].forEach.call($tables.find('.tabledrag-hide'), (el) => {
    ...
    +    [].forEach.call($tables.find('.tabledrag-handle'), (el) => {
    
    +++ b/core/modules/views_ui/js/dialog.views.js
    @@ -14,7 +14,12 @@
    +      [].forEach.call($scroll, (el) => {
    
    @@ -24,8 +29,12 @@
    +      [].forEach.call($modal, (el) => {
    

    I mean let's use .each() since we're working with jquery elements, when converting the selector to vanilla we'll be able to directly use .forEach.

  3. +++ b/core/modules/toolbar/js/views/ToolbarVisualView.js
    @@ -79,17 +79,11 @@
    +        const height = toolbarTabOuterHeight + toolbarTrayHorizontalOuterHeight;
    +        this.model.set('height', height);
    

    doesn't seem related to .css() functions

  4. +++ b/core/scripts/dev/commit-code-check.sh
    @@ -495,6 +495,22 @@
    +# @todo Remove the next chunk of lines before committing. This script only lints
    +#  JavaScript files that have changed, so we add this to check all files for
    +#  jQuery-specific lint errors.
    +cd "$TOP_LEVEL/core"
    +node ./node_modules/eslint/bin/eslint.js --quiet --config=.eslintrc.passing.json .
    +
    +CORRECTJQS=$?
    +if [ "$CORRECTJQS" -ne "0" ]; then
    +  # No need to write any output the node command will do this for us.
    +  printf "${red}FAILURE ${reset}: unsupported jQuery usage. See errors above."
    +  STATUS=1
    +  FINAL_STATUS=1
    +fi
    +cd $TOP_LEVEL
    +# @todo end lines to remove
    +
    

    Not needed anymore, it's automatic when changing eslint config now.

nod_’s picture

mstrelan’s picture

StatusFileSize
new18.89 KB
new9.54 KB
mstrelan’s picture

Status: Needs work » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new4.39 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nod_’s picture

We need to add a comment to disable the eslint rule for this one use in ajax.js

mstrelan’s picture

StatusFileSize
new19.23 KB
new289 bytes
mstrelan’s picture

Status: Needs work » Needs review
StatusFileSize
new19.27 KB
new3.11 KB

I forgot how to jquery.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

update looks good. Hopefully can get this in early for 10.2

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/misc/tableheader.js
    @@ -245,12 +242,15 @@
    +        Object.entries(css).forEach(([property, value]) => {
    +          this.$stickyTable[0].style[property] = value;
    +        });
    
    @@ -318,14 +317,17 @@
    +            Object.assign($stickyCell[0].style, {
    +              width: window.getComputedStyle($that[0]).width,
    +              display,
    

    We have some cases where we use Object.entries.forEach and others where we use Object.assign - is there a reason?
    Per #20 I think the .assign feels nicer.

  2. +++ b/core/modules/toolbar/js/views/ToolbarVisualView.js
    @@ -84,12 +84,12 @@
    +        document.querySelector('body').style.paddingTop = `${this.model.get(
    +          'height',
    

    any reason we don't use document.body here?

  3. We're adding quite a few new uses of $.each() here, which feels odd given we're trying to remove usage of jQuery, ah but #30.2 details why

Couple of questions, putting back to NR for the first two

mstrelan’s picture

StatusFileSize
new19.11 KB
new1.22 KB

Good points, I think they were just oversights. Updated patch addresses 39.1 and 39.2.

mstrelan’s picture

StatusFileSize
new19.11 KB
new684 bytes

#39.2 Let's just revert back to the original jquery selectors, changing them is not really in scope

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Do we want a follow up for 39.2?

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll after it .is issue went in

Conflict in

core/modules/views_ui/js/views-admin.js

mstrelan’s picture

Status: Needs work » Needs review
StatusFileSize
new19.11 KB
new941 bytes

#42 I think there is (or should be) a child issue of #3238306: [META] Where possible, refactor existing jQuery uses to vanillaJS to reduce jQuery footprint to replace all use of jquery selectors with native equivalents.

Re-rolled #41 to fix up #43.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

LGTM

larowlan’s picture

  • larowlan committed 6a43b511 on 11.x
    Issue #3238308 by mstrelan, bnjmnm, nod_, smustgrave, larowlan,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6a43b51 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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

iberezovchuk’s picture

After this refactoring tableheader.js stopped adding correct width to sticky-header.

ljwilson’s picture

We just migrated a production site from 10.1.8 to 10.2.6, and noticed for one of our custom blocks the contextual nested links were overlaying on top of each other. I tracked it down to this change:
core/modules/contextual/js/contextual.js

I asked Chat-GPT what the difference might be, and it recommended adding 'px' since the version in 10.1.8 probably interpreted it as pixels but the 10.2.x version does not. Adding the units of 'px' fixed the problem. My change below:

diff --git a/core/modules/contextual/js/contextual.js b/core/modules/contextual/js/contextual.js
index 52fb138e9..324fe7766 100644
--- a/core/modules/contextual/js/contextual.js
+++ b/core/modules/contextual/js/contextual.js
@@ -72,7 +72,7 @@
 
       // Adjust nested contextual link's position.
       $nestedContextual[0].style.top =
-        $nestedContextual.position().top + height;
+        $nestedContextual.position().top + height + 'px';
     }
   }