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
Comment | File | Size | Author |
---|---|---|---|
#46 | core-js-removeid-2346799-46.patch | 6.36 KB | impalash |
#30 | core-js-removeid-2346799-30.patch | 7.09 KB | nod_ |
#25 | interdiff-2346799-14-25.txt | 5.53 KB | mathysp |
#25 | replace_id_selectors-2346799-25.patch | 11.39 KB | mathysp |
Comments
Comment #1
nod_Still very relevent
Comment #2
pguillard CreditAttribution: pguillard commentedHere occurrences that I found.
If I'm not on the wrong track, I'll start creating a patch
Comment #3
nod_Updated IS explaining the situation.
Attaching a patch fixing id issues in the block module as an example.
Comment #4
himanshupathak3 CreditAttribution: himanshupathak3 as a volunteer and at Srijan | A Material+ Company commentedComment #5
Manjit.SinghTriggering test bot :)
Comment #6
dawehnerIsn't that critical then?
Comment #7
himanshupathak3 CreditAttribution: himanshupathak3 as a volunteer and at Srijan | A Material+ Company commentedComment #8
himanshupathak3 CreditAttribution: himanshupathak3 as a volunteer and at Srijan | A Material+ Company commentedchanged 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.
Comment #9
himanshupathak3 CreditAttribution: himanshupathak3 as a volunteer and at Srijan | A Material+ Company commentedComment #10
nod_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.
Comment #11
nod_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.
Comment #12
himanshupathak3 CreditAttribution: himanshupathak3 as a volunteer and at Srijan | A Material+ Company commentedNod_, Thank you for your instructions. Let's work on if after the above issue is resolved.
Comment #13
olli CreditAttribution: olli commented#1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests landed.
Comment #14
pguillard CreditAttribution: pguillard commentedLittle Reroll
Comment #17
borisson_Requeued for testing because I can't seem to reproduce the exception locally.
Comment #18
mathysp CreditAttribution: mathysp at Mia Interactive commented> Toolbar
> Tour
> SimpleTest
Comment #19
dgcollard CreditAttribution: dgcollard commentedComment #20
thatpatguy CreditAttribution: thatpatguy commentedworking on the Language task at DrupalConBarcelona. Being mentored by Justafish.
Comment #21
thatpatguy CreditAttribution: thatpatguy commentedran 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).
Comment #22
pguillard CreditAttribution: pguillard commentedReset to Needs Review, as I don't know if there is something to do there.
Comment #23
Jelle_SLanguage module had 1 id selector that needed to be replaced.
NR for bot.
Comment #24
mathysp CreditAttribution: mathysp at Mia Interactive commentedJust 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.
Comment #25
mathysp CreditAttribution: mathysp at Mia Interactive commentedAlright. 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!
Comment #27
mathysp CreditAttribution: mathysp at Mia Interactive commentedI'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
Comment #29
pguillard CreditAttribution: pguillard commentedThis patch is OK for me.
Comment #30
nod_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).
Comment #31
Ankit Agrawal CreditAttribution: Ankit Agrawal as a volunteer commentedNeeds review. Patch to be ported.
Comment #32
nileema.jadhav CreditAttribution: nileema.jadhav commentedComment #33
nileema.jadhav CreditAttribution: nileema.jadhav at TATA Consultancy Services for Pfizer, Inc. commentedWorking on porting the patch
Comment #35
Manjit.SinghIs there anything pending ?
Comment #36
pguillard CreditAttribution: pguillard commentedComment #37
ashishdalviWe will take this issue on Drupal Mumbai Code sprint.
Comment #38
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedComment #39
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedI could apply the patch, so no reroll is needed.
Comment #40
droplet CreditAttribution: droplet commentedshould 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
Comment #44
ranaivo CreditAttribution: ranaivo as a volunteer and commentedWorking on CKEditor at DrupalCon Vienna 2017 #drupalconeur mentor drpal
The related issue:
https://www.drupal.org/node/2912706
Comment #46
impalash CreditAttribution: impalash at Google Summer of Code commentedissue fixed for 8.6.x
Comment #51
kreyol_dev CreditAttribution: kreyol_dev as a volunteer commentedI'm working on this for #drupalcon2020. Feel free to ping me on Slack.
Comment #52
igorbarato CreditAttribution: igorbarato as a volunteer commentedI'm working to check if the list of modules to fix are update. I'm working for DrupalCon2020
Comment #53
igorbarato CreditAttribution: igorbarato as a volunteer commentedI 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:
Are these cases need to update following the same rule?
Comment #54
nod_I would leave those 4 alone for now, they might be tricky.
Comment #55
nod_Comment #56
droplet CreditAttribution: droplet commentedSometimes, 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.
Comment #61
GuyPaddock CreditAttribution: GuyPaddock at Inveniem commentedAlso related to https://www.drupal.org/node/2503277.