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

  • vertical-tabs
  • Block
  • CKEditor
  • Color
  • Editor
  • Field UI
  • Filter
  • Language
  • Locale
  • Menu UI
  • Node
  • Quickedit
  • Simpletest
  • System
  • Toolbar
  • Tour
  • User
  • Views
  • Views UI (by far the one needing the most work, might be good to make a new issue).

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

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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,669 pass(es). View

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

FileSize
7.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,682 pass(es). View
3.43 KB

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
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,467 pass(es). View

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

FileSize
7.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,955 pass(es). View

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
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,971 pass(es). View
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
Ashish.Dalvi’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.