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-attr, which targets the jQuery attr function.

Steps to reproduce

Proposed resolution

Remaining tasks

  • In core/.eslintrc.jquery.json Change "jquery/no-attr": 0, to "jquery/no-attr": 2, to enable eslint checks for uses of jQuery .attr(). 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 .attr() to use Vanilla (native) JavaScript instead.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#36 3239161-nr-bot.txt90 bytesneeds-review-queue-bot
#26 3239161-nr-bot.txt9.11 KBneeds-review-queue-bot
#23 3239161-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3239161

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

Theresa.Grannum created an issue. See original summary.

Theresa.Grannum opened merge request !1237

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.

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
Version: 9.5.x-dev » 10.1.x-dev
Assigned: Unassigned » andy-blum

andy-blum opened merge request !2792

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.

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

rahulrasgon opened merge request !5791

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
Assigned: andy-blum » Unassigned
rahulrasgon’s picture

rahulrasgon commented
Assigned: Unassigned » rahulrasgon
rahulrasgon’s picture

rahulrasgon commented

Below replacement needs to be done more.

/core/misc/machine-name.js
  157:11  error  Prefer getAttribute to $.attr  jquery/no-attr

/core/misc/tableresponsive.js
  40:28  error  Prefer Function#bind to $.proxy  jquery/no-proxy

/core/misc/tableselect.js
  72:16  error  Prefer setAttribute to $.attr  jquery/no-attr

/core/modules/block/js/block.admin.js
  26:24  error  Prefer getAttribute to $.attr  jquery/no-attr

/core/modules/comment/js/comment-new-indicator.js
  24:9   error  Prefer getAttribute to $.attr  jquery/no-attr
  28:22  error  Prefer getAttribute to $.attr  jquery/no-attr
  79:11  error  Prefer getAttribute to $.attr  jquery/no-attr
  82:24  error  Prefer getAttribute to $.attr  jquery/no-attr

/core/modules/comment/js/node-new-comments-link.js
   80:9   error  Prefer getAttribute to $.attr  jquery/no-attr
   83:19  error  Prefer getAttribute to $.attr  jquery/no-attr
   84:22  error  Prefer getAttribute to $.attr  jquery/no-attr
  125:11  error  Prefer setAttribute to $.attr  jquery/no-attr
  167:11  error  Prefer getAttribute to $.attr  jquery/no-attr
  170:24  error  Prefer getAttribute to $.attr  jquery/no-attr

/core/modules/contextual/js/views/AuralView.js
  43:9  error  Prefer setAttribute to $.attr  jquery/no-attr

/core/modules/editor/js/editor.admin.js
  1022:31  error  Prefer getAttribute to $.attr  jquery/no-attr

/core/modules/editor/js/editor.js
  17:21  error  Prefer getAttribute to $.attr  jquery/no-attr

/core/modules/file/file.js
  258:30  error  Prefer getAttribute to $.attr  jquery/no-attr
  261:9   error  Prefer setAttribute to $.attr  jquery/no-attr
  268:11  error  Prefer setAttribute to $.attr  jquery/no-attr
  291:7   error  Prefer setAttribute to $.attr  jquery/no-attr

/core/modules/layout_builder/js/layout-builder.js
   65:11  error  Prefer setAttribute to $.attr  jquery/no-attr
   70:11  error  Prefer setAttribute to $.attr  jquery/no-attr
  209:7   error  Prefer setAttribute to $.attr  jquery/no-attr
  238:18  error  Prefer getAttribute to $.attr  jquery/no-attr
  258:41  error  Prefer getAttribute to $.attr  jquery/no-attr
  374:51  error  Prefer getAttribute to $.attr  jquery/no-attr
  434:9   error  Prefer setAttribute to $.attr  jquery/no-attr

/core/modules/menu_ui/menu_ui.admin.js
  63:9  error  Prefer setAttribute to $.attr  jquery/no-attr

/core/modules/settings_tray/js/settings_tray.js
  168:76  error  Prefer getAttribute to $.attr  jquery/no-attr

/core/modules/system/js/system.modules.js
  66:11  error  Prefer setAttribute to $.attr  jquery/no-attr
  72:11  error  Prefer setAttribute to $.attr  jquery/no-attr
  86:11  error  Prefer setAttribute to $.attr  jquery/no-attr

/core/modules/system/tests/modules/js_message_test/js/js_message_test.js
  44:24  error  Prefer getAttribute to $.attr  jquery/no-attr
  46:13  error  Prefer getAttribute to $.attr  jquery/no-attr
  50:26  error  Prefer getAttribute to $.attr  jquery/no-attr

/core/modules/toolbar/js/views/ToolbarVisualView.js
  315:9  error  Prefer setAttribute to $.attr  jquery/no-attr
  319:9  error  Prefer setAttribute to $.attr  jquery/no-attr

Meanwhile can someone please review the MR :- https://git.drupalcode.org/project/drupal/-/merge_requests/5791#note_242353

rahulrasgon’s picture

rahulrasgon commented
Assigned: rahulrasgon » Unassigned
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

The remaining task around the code-committ still seems needed.

nod_’s picture

nod_
Primary language French
Location Lille
commented

There are several places that create a jquery object just to get the original dom element after. We can get rid of the creation of the jQuery object and use the dom directly

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

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Addressed all feedbacks in MR. please review

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Changes caused failures.

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

ahsannazir’s picture

ahsannazir commented

In the above raised MR, the Funtional Javascript test cases are failing and there are console errors as well

ahsannazir’s picture

ahsannazir commented

Resolved all failures. Please review

ahsannazir’s picture

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

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3239161-nr-bot.txt90 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 necessarily 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.

ahsannazir’s picture

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

smustgrave commented
Issue tags: +Needs subsystem maintainer review

Will leave for sub-maintainer to review but personally find adding [0] makes the readability go down.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3239161-nr-bot.txt9.11 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 necessarily 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.

shubh_’s picture

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

smustgrave commented
Status: Needs review » Needs work

Issues in MR.

Sam Phillemon made their first commit to this issue’s fork.

Sam Phillemon’s picture

Sam Phillemon commented
Status: Needs work » Needs review

Fixed the remaining MR-related issues.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

For good practice can the MRs be cleanup/hidden. There are currently 4 branche

shubh_ changed the visibility of the branch 3239161-refactor-if-feasible to hidden.

shubh_ changed the visibility of the branch 3239161-10.1.x to hidden.

shubh_ changed the visibility of the branch 9.3.x to hidden.

shubh_’s picture

shubh_ commented
Status: Needs work » Needs review

Done

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3239161-nr-bot.txt90 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 necessarily 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.

ahsannazir’s picture

ahsannazir commented
Status: Needs work » Needs review
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Needs work
ahsannazir’s picture

ahsannazir commented
Status: Needs work » Needs review
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Needs work
ahsannazir’s picture

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

smustgrave commented
Status: Needs review » Needs work

MR needs a manual rebase

ahsannazir’s picture

ahsannazir commented
Status: Needs work » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Closed (won't fix)
Issue tags: -Needs subsystem maintainer review

First of all thank you all for the hard work on this one, I've had to push a number of patches like this and I know how hard it is to keep up.

I'm going to try and refocus the jQuery removal work and to do that I need to take a few decisions. I'm going to close this issue for a few reasons:

  • This MR is big, it impact a very big number of subsystems and make the code more brittle. jQuery is good at dealing with undefined elements, empty sets and so on, the DOM isn't. We already had regressions from a previous patch with undefined elements
  • The MR is too big to review and make sure there are no regressions (even with the tests we already have), it would create unstability that we don't have to endure
  • Every time I review this MR, I find a new bug
  • Sometimes the jQuery code is simply more readable, I do not think this change is a net positive:
              $details
            .filter('[data-drupal-system-state="forced-open"]')
            .removeAttr('data-drupal-system-state')
            .attr('open', false);
          $details.forEach((detail) => {
            if (
              detail.getAttribute('data-drupal-system-state') === 'forced-open'
            ) {
              detail.removeAttribute('data-drupal-system-state');
              detail.setAttribute('open', 'false');
            }
          });

    There is one line to change and just noticed a bug (jQuery doesn't have a forEach method)

I ported the credits to #3464044: Credit for work on the reduce jQuery issues, which I will mark as fixed as soon as I go through all the others impacted jQuery issues.