Refactor (if feasible) uses of the jQuery text function to use vanillaJS [#3239123]

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

Steps to reproduce

Proposed resolution

Remaining tasks

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

User interface changes

API changes

Data model changes

Release notes snippet

Comment	File	Size	Author
#36	3239123-36-d10.patch	66.09 KB	hooroomoo
#35	3239123-35-d10.patch	34.45 KB	hooroomoo
#23	jquery-text-d10-23.patch	57.41 KB	bnjmnm
#21	jquery-text-d10.patch	66.08 KB	bnjmnm

Issue fork drupal-3239123

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3239123-refactor-if-feasible changes, plain diff MR !1259
Check out this branch for the first time

Check out existing branch, if you already have it locally

1 hidden branch

3239123-jquery-text

changes, plain diff MR !1321

Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

24 September 2021 at 16:47

hooroomoo created an issue. See original summary.

Comment #2

27 September 2021 at 15:56

hooroomoo opened merge request !1259

Comment #3

15 October 2021 at 18:49

hooroomoo opened merge request !1321

Comment #4

28 October 2021 at 17:26

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

Comment #5

hooroomoo

they/them

nyc🗽

commented 4 November 2021 at 14:23

Am aware that I used .includes() in core/modules/views_ui/js/views_ui.listing.js.

This is in the hope that #3239509: Add String.includes polyfill to support IE11 and Opera Mini would get committed to core by the time this was reviewed since it was already committed once but de-committed due to the most recent code freeze.

Comment #6

larowlan

🇦🇺🏝.au GMT+10

commented 4 November 2021 at 19:48

I've asked if #3239509: Add String.includes polyfill to support IE11 and Opera Mini is still 9.3 eligible, thanks for the reminder

Comment #7

hooroomoo

they/them

nyc🗽

commented 4 November 2021 at 20:11

Status:

Active

» Needs review

Comment #8

bnjmnm

he/him

English

Ann Arbor, MI

commented 11 November 2021 at 12:29

Status:

Needs review

» Needs work

Every change has been reviewed. Even those that don't need changes have a (resolved) comment in Gitlab stating that so nobody has to re-review parts already confirmed to be fine.

Comment #9

11 November 2021 at 12:29

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.

Comment #10

hooroomoo

they/them

nyc🗽

commented 6 December 2021 at 18:50

Status:

Needs work

» Needs review

Comment #11

bnjmnm

he/him

English

Ann Arbor, MI

commented 4 January 2022 at 12:57

Status:

Needs review

» Needs work

Setting to "needs work" - multiple test failures, but apparently didn't automatically set the issue to NW

Menu.Drupal\Tests\system\Functional\Menu\LocalTasksTest.Drupal\Tests\system\Functional\Menu\LocalTasksTest
Node.Drupal\Tests\node\Functional\NodeTypeTest.Drupal\Tests\node\Functional\NodeTypeTest
Node.Drupal\Tests\node\Functional\NodeTypeTest.Unknown
User.Drupal\Tests\user\Functional\UserPermissionsTest.Drupal\Tests\user\Functional\UserPermissionsTest
User.Drupal\Tests\user\Functional\UserPermissionsTest.Unknown

Comment #12

hooroomoo

they/them

nyc🗽

commented 5 January 2022 at 20:57

Status:

Needs work

» Needs review

Comment #13

hooroomoo

they/them

nyc🗽

commented 6 January 2022 at 22:02

Checked that nodelist.foreach polyfill dependecy is added where necessary.

Comment #14

nod_

French

Lille

commented 11 January 2022 at 15:54

Status:

Needs review

» Needs work

few things :)

Comment #15

hooroomoo

they/them

nyc🗽

commented 13 January 2022 at 17:05

Status:

Needs work

» Needs review

Comment #16

nod_

French

Lille

commented 14 January 2022 at 10:03

Status:

Needs review

» Needs work

Couple of things, 4-5 bugs, one regression few extra changes that are not necessary for this issue.

Looking good!

Comment #17

hooroomoo

they/them

nyc🗽

commented 14 January 2022 at 20:20

Status:

Needs work

» Needs review

Comment #18

nod_

French

Lille

commented 17 January 2022 at 10:56

Status:

Needs review

» Needs work

Just one little comment and tests fail to fix (need to check if element exists before getting the textContent) and we're good to go! Probably need a D10 patch once it's RTBC too

Great work, thanks you!

Comment #19

hooroomoo

they/them

nyc🗽

commented 18 January 2022 at 22:22

Status:

Needs work

» Needs review

Comment #20

nod_

French

Lille

commented 19 January 2022 at 11:14

Status:

Needs review

» Reviewed & tested by the community

And it is RTBC!

Everything looks good now. Thanks so much for going through that again :) We only need a D10 patch.

(marking rtbc because the issue is against 9.4.x and that code is RTBC)

Comment #21

bnjmnm

he/him

English

Ann Arbor, MI

commented 19 January 2022 at 13:26

Status	File	Size
new	jquery-text-d10.patch	66.08 KB

Drupal 10 patch

Comment #22

nod_

French

Lille

commented 19 January 2022 at 13:36

D10 patch is good except for the changes to the core.libraries.yml file

Comment #23

bnjmnm

he/him

English

Ann Arbor, MI

commented 19 January 2022 at 14:17

Status	File	Size
new	jquery-text-d10-23.patch	57.41 KB

In a patch with that many changes, I'd be shocked if I didn't miss something. Here's the patch without core.libraries

Comment #24

nod_

French

Lille

commented 19 January 2022 at 14:50

Unfortunately the eslint rule isn't bullet-proof and we missed a few instances of .text() usage

core/misc/tableresponsive.es6.js
          this.$link.show().text(this.showText);
          this.$link.hide().text(this.hideText);
          this.$link.text(this.hideText).data('pegged', 1);
          this.$link.text(this.showText).data('pegged', 0);
core/modules/ckeditor/js/views/VisualView.es6.js
          .text(
core/modules/contextual/js/views/AuralView.es6.js
          .text(
core/modules/quickedit/js/views/ContextualLinkView.es6.js
        this.$el.find('a').text(options.strings.quickEdit);

I think it would be fine to have a follow-up though this will avoid having to be careful of the 60+ files that are changed by the current patch.

Leaving RTBC because the patch does fix the issue and eslint rules are passing.

Comment #25

nod_

French

Lille

commented 19 January 2022 at 14:50

in any case MR + D10 patch are RTBC for me.

Comment #26

hooroomoo

they/them

nyc🗽

commented 19 January 2022 at 15:05

Status:

Reviewed & tested by the community

» Active

There are some .text() usages that were not caught by the linter that nod_ pointed out. Will address.

Comment #27

hooroomoo

they/them

nyc🗽

commented 19 January 2022 at 22:47

Status:

Active

» Needs review

Comment #28

nod_

French

Lille

commented 20 January 2022 at 10:28

Status:

Needs review

» Reviewed & tested by the community

Fixes are working as expected and don't see anymore jQuery .text usage!

Thank you :)

Comment #29

lauriii

he/him

Finnish

Finland

commented 21 January 2022 at 10:02

Status:

Reviewed & tested by the community

» Needs review

I don't see any discussion about whether using Node.textContent is appropriate because it isn't exactly same as jQuery.text().

Based on jQuery documentation, jQuery.text() is escaping any nodes. This means that jQuery.text( 'This is a test.' ) would result in This is a test., whereas Node.textContent = 'This is a test.' would result in This is a test..

Comment #30

nod_

French

Lille

commented 21 January 2022 at 10:16

Status:

Needs review

» Reviewed & tested by the community

I believe their documentation is outdated.

In the version of jquery we're using there it no createTextNode involved when using the text function: https://git.drupalcode.org/project/drupal/-/blob/9.4.x/core/assets/vendo...

The code in this patch is doing the same thing as jQuery :)

Comment #31

lauriii

he/him

Finnish

Finland

commented 21 January 2022 at 12:03

Status:

Reviewed & tested by the community

» Needs work

Posted few comments on the MR

Comment #32

hooroomoo

they/them

nyc🗽

commented 21 January 2022 at 22:50

Status:

Needs work

» Needs review

Comment #33

nod_

French

Lille

commented 26 January 2022 at 12:26

Status:

Needs review

» Reviewed & tested by the community

Last problems have been addressed, all good for me

Comment #34

lauriii

he/him

Finnish

Finland

commented 26 January 2022 at 14:05

This needs a new Drupal 10 patch

Comment #35

hooroomoo

they/them

nyc🗽

commented 26 January 2022 at 18:00

Status	File	Size
new	3239123-35-d10.patch	34.45 KB

d10 patch

Comment #36

hooroomoo

they/them

nyc🗽

commented 26 January 2022 at 18:48

Status	File	Size
new	3239123-36-d10.patch	66.09 KB

1 file was hidden/shown/deleted

Status	File	Size
hidden	3239123-35-d10.patch	34.45 KB

Comment #37

28 January 2022 at 09:54

lauriii committed 8c62a32 on 10.0.x

Issue #3239123 by hooroomoo, bnjmnm, nod_: Refactor (if feasible) uses...

Comment #38

28 January 2022 at 09:54

lauriii committed 804a0a1 on 9.4.x

Issue #3239123 by hooroomoo, bnjmnm, nod_: Refactor (if feasible) uses...

Comment #39

28 January 2022 at 09:54

lauriii closed merge request !1321

Comment #40

lauriii

he/him

Finnish

Finland

commented 28 January 2022 at 09:55

Status:

Reviewed & tested by the community

» Fixed

Committed 8c62a32 and pushed to 10.0.x. Also committed the MR in 9.4.x. Thanks!

Comment #41

11 February 2022 at 09:59

Status:

Fixed

» Closed (fixed)

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

Refactor (if feasible) uses of the jQuery text function to use vanillaJS

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3239123

Comments

Parent issue

Referenced by