Problem/Motivation

Use HTML5 data-drupal-selector attribute instead of #id selectors in JavaScript files. When appropriate use [name=""] when the #id targets a form element, no need to add a data attribute for them.

This issue is blocking #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests because generated IDs break all JS that rely on predictable ids.

Proposed resolution

  • Take out JS references to $("#id") and replace them with $('[data-drupal-selector="id"]') or $('[name="field-name"]')

Remaining tasks

  • Block
  • CKEditor
  • Color
  • Editor
  • Field UI
  • Filter
  • Image
  • Language
  • Layout Builder
  • Locale
  • Media
  • Media Library
  • Menu UI
  • Node
  • Quickedit
  • Settings Tray
  • System
  • Taxonomy
  • Toolbar
  • Tour
  • User
  • Views
  • Views UI

All those modules, expect Views UI, have about 5 selectors needing to be replaced, the vast majority targeting form elements.

User interface changes

NONE

API changes

NONE

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

Issue tags: -

Still very relevent

pguillard’s picture

Status: Active » Needs work

Here occurrences that I found.
If I'm not on the wrong track, I'll start creating a patch

core/modules/node/content_types.js:
32:vals.push($(".form-item-language-configuration-langcode select option:selected", context).text());
core/modules/locale/locale.admin.js:
10:var $form = $("#locale-translate-edit-form").once('localetranslatedirty');
34:var $form = $("#locale-translate-edit-form").removeOnce('localetranslatedirty');
core/modules/views_ui/js/ajax.js:
67:$("#views-tabset a").once('views-ajax').on('click', function () {
72:$("#views-live-preview #preview-display-id").val(display_id);
core/misc/vertical-tabs.js:
105:$(".vertical-tabs__pane :input:visible:enabled").eq(0).trigger('focus');
nod_’s picture

Title: Replace #ID and .classname selectors for data-drupal-* attributes » Replace #ID selectors for data-drupal-selector attribute
Assigned: nicobot » Unassigned
Issue summary: View changes
Issue tags: -html, -sprint, -target, -frontend, -Amsterdam2014 +Novice
Related issues: +#1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests
FileSize
4.3 KB

Updated IS explaining the situation.

Attaching a patch fixing id issues in the block module as an example.

himanshupathak3’s picture

Assigned: Unassigned » himanshupathak3
Issue tags: +SrijanSprintNight
Manjit.Singh’s picture

Status: Needs work » Needs review

Triggering test bot :)

dawehner’s picture

Isn't that critical then?

himanshupathak3’s picture

Status: Needs review » Needs work
himanshupathak3’s picture

changed js files in color module, not sure how to add code related to this in .php files.
@nod_ can you provide some help snippet regarding how to update same in .php files ?
Added patch and interdiff.

himanshupathak3’s picture

Status: Needs work » Needs review
nod_’s picture

Status: Needs review » Needs work

For information I'm waiting on feedback on #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests to know whether this issue is just a normal/major task or a critical one. While waiting make sure you don't work on the parts already rolled in the other patch.

For the color module, when there is an id with "edit-" it means it's a form element and there is most likely a name attribute you can use to select it (no need for data-drupal-selector) all the time. I'd say try to check which element is selected and use it's name to select it.

nod_’s picture

Status: Needs work » Postponed

Let's wait on #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests to be resolved, it might introduce some very handy code to facilitate this conversion.

himanshupathak3’s picture

Assigned: himanshupathak3 » Unassigned

Nod_, Thank you for your instructions. Let's work on if after the above issue is resolved.

olli’s picture

pguillard’s picture

Status: Needs work » Needs review
FileSize
5.86 KB

Little Reroll

Status: Needs review » Needs work

The last submitted patch, 14: replace_id_selectors-2346799-14.patch, failed testing.

Status: Needs work » Needs review
borisson_’s picture

Requeued for testing because I can't seem to reproduce the exception locally.

mathysp’s picture

Assigned: Unassigned » mathysp
Status: Needs review » Needs work

> Toolbar
> Tour
> SimpleTest

dgcollard’s picture

Issue summary: View changes
thatpatguy’s picture

Assigned: mathysp » thatpatguy

working on the Language task at DrupalConBarcelona. Being mentored by Justafish.

thatpatguy’s picture

Assigned: thatpatguy » Unassigned

ran out of time, wasn't able to find anything that needed changing for the Language module (but I had to jump off before able to say for certain that nothing needs to get changed).

pguillard’s picture

Status: Needs work » Needs review

Reset to Needs Review, as I don't know if there is something to do there.

Jelle_S’s picture

Language module had 1 id selector that needed to be replaced.

NR for bot.

mathysp’s picture

Status: Needs review » Needs work
Issue tags: +Barcelona2015

Just to confirm, as per comment #18, I am working on this at the sprinting session @DrupalCon Barcelona. My mentor is jp_stacey. I am working based on patch in #14.

mathysp’s picture

Status: Needs work » Needs review
FileSize
11.39 KB
5.53 KB

Alright. I've finished my work on:
> Toolbar
> Tour
> SimpleTest

The patch included starts from the patch from comment #14. See #24 for a bit more info.

This DOES NOT include Jelle_S's work. So #23 is not included!

Status: Needs review » Needs work

The last submitted patch, 25: replace_id_selectors-2346799-25.patch, failed testing.

mathysp’s picture

Status: Needs work » Needs review

I've reviewed both the failed-patch-log and my code, and I can't seem to find a good reason as to why my patch failed. So I'm resubmitting this for testing. /em crosses fingers

pguillard’s picture

This patch is OK for me.

nod_’s picture

Version: 8.0.x-dev » 8.1.x-dev
FileSize
7.09 KB

Merged the two patch and remove some extra changes that are not needed. A bunch of things making this patch easier got in.

The fact that data-drupal-selector attribute is automatically added for every element that has an id helps a lot. Means that this patch doesn't need to change PHP to add things it can just reuse available markup.

I've omitted the changes to #gradient-X selectors since we don't use them anymore. I'll open an issue to get rid of them (instead we use css gradients).

Ankit Agrawal’s picture

Needs review. Patch to be ported.

nileema.jadhav’s picture

Assigned: Unassigned » nileema.jadhav
nileema.jadhav’s picture

Issue tags: +drupalconasia2016

Working on porting the patch

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.

Manjit.Singh’s picture

Status: Needs review » Needs work

Is there anything pending ?

pguillard’s picture

Assigned: nileema.jadhav » Unassigned
ashishdalvi’s picture

We will take this issue on Drupal Mumbai Code sprint.

rasikap’s picture

Assigned: Unassigned » rasikap
rasikap’s picture

Assigned: rasikap » Unassigned
Status: Needs work » Needs review

I could apply the patch, so no reroll is needed.

droplet’s picture

Status: Needs review » Needs work

should we postpone it until more JS test cases covered in CORE? And I think we better to do it module-by-module instead.

this project may help Core & Contributed modules for this transform.
https://github.com/facebook/jscodeshift

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.

ranaivo’s picture

Working on CKEditor at DrupalCon Vienna 2017 #drupalconeur mentor drpal

The related issue:
https://www.drupal.org/node/2912706

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.

impalash’s picture

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.

kreyol_dev’s picture

Issue tags: -JavaScript +JavaScript

I'm working on this for #drupalcon2020. Feel free to ping me on Slack.

igorbarato’s picture

I'm working to check if the list of modules to fix are update. I'm working for DrupalCon2020

igorbarato’s picture

Issue summary: View changes

I searched on the core modules for JS files with references by '#' and updated the list of modules to update.

I found some cases in which the '#' is used with a concatenation, like these codes below:

core/modules/ckeditor/js/ckeditor.off-canvas-css-reset.js:
  10:     var selectorPrefix = '#drupal-off-canvas ';
core/modules/ckeditor/js/views/ControllerView.es6.js:
  185:         CKEDITOR.inline($(`#${hiddenCKEditorID}`).get(0), CKEditorConfig);
core/modules/editor/js/editor.es6.js:
  20:     return $(`#${fieldId}`).get(0);
core/modules/image/js/editors/image.js:
  161:       var $toolgroup = $("#".concat(fieldModel.toolbarView.getMainWysiwygToolgroupId()));

Are these cases need to update following the same rule?

nod_’s picture

I would leave those 4 alone for now, they might be tricky.

nod_’s picture

droplet’s picture

Sometimes, I'm confused what we're trying to do.. or which one is correct usage

I've seen smiliar changes and commits before I asked here...

https://www.drupal.org/project/drupal/issues/3084916

Do we really needed both HTML Class & data attribute at the same time?

We even has another `js-prefix`...

So that's a chance we have 3 same things.....

We only need ONE I believed.

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.

GuyPaddock’s picture

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.