Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Please (see #8) also credit
mortendk
rachel_norfolk
Task
Prefix form-item-* classes with js- to separate classes needed for JavaScript functionality from those used for styling and make it clear which classes can safely be removed without breaking JavaScript functionality.
Remaining tasks
- Patch
- Patch review
- Manual testing
Steps to test
@todo
Beta phase evaluation
Issue category | Task |
---|---|
Issue priority | Normal |
Unfrozen changes | Unfrozen because it mostly just affects templates and JS which are not frozen. |
Prioritized changes | The main goal of this issue is to improve themer experience and arguably it reduces fragility where JavaScript and markup intersect. |
Disruption | Shouldn't be too disruptive as it is mostly affecting CSS classes that are added to markup. Themes extending Classy will only have classes added. Themes not extending Classy will have classes removed but they can be added back via template overrides. |
User interface changes
None for themes extending Classy. Possible visual changes for other themes.
API changes
n/a
Comment | File | Size | Author |
---|---|---|---|
#38 | interdiff-jsprefix-33to38.txt | 595 bytes | davidhernandez |
#38 | prefix_form_item-2473947-38.patch | 17.04 KB | davidhernandez |
#36 | manual_testing_2473947-33.zip | 1.39 MB | akalata |
#33 | prefix_form_item-2473947-33.patch | 17.09 KB | davidhernandez |
#30 | prefix_form_item-2473947-30.patch | 17.09 KB | rachel_norfolk |
Comments
Comment #1
star-szrInitial patch split from the parent, some additional changes from grepping, and interdiff between the two.
Comment #5
star-szrRandom test fail from \Drupal\simpletest\Tests\SimpleTestBrowserTest, something about https.
Comment #6
emma.mariaReviewing.
Comment #7
emma.mariaI applied 2473947-form-item--additional.patch and worked through the files in the patch to manually test the components affected in the UI. Here are my results...
Managed to test and it works:
core/modules/block_content/js/block_content.js
Found on /block/add
It works!
core/modules/ckeditor/js/ckeditor.admin.js
Found on /admin/config/content/formats/manage/basic_html
It works!
core/modules/field_ui/field_ui.js
Found on /admin/structure/types/manage/article/fields/add-field
It works!
core/modules/language/config/optional/tour.tour.language.yml
Found on /admin/config/regional/language and click on tour.
It works!
core/modules/node/content_types.js
Found on /node/add/article
It works!
core/modules/system/templates/form-element.html.twig
Textfield prints with the classes ...
form-item form-type-textfield js-form-item-subject-0-value
It works!
core/themes/classy/templates/form/form-element.html.twig
Textarea prints with the classes ...
form-item form-type-textarea js-form-item-comment-body-0-value form-item-comment-body-0-value
It works!
Things I couldn't find in Drupal to test
Changes in:
core/modules/comment/comment-entity-form.js
core/modules/menu_ui/menu_ui.js
core/modules/node/node.js
core/modules/path/path.js – Testing the path module ended with 'Site encountered error' error.
core/modules/views_ui/js/views-admin.js
Other review work:
I also searched through the Core folder and could not find any more instances of "form-item-*" in .js files that may have been missed.
Comment #8
star-szrThanks for the review @emma.maria, super helpful!
Here's an updated patch without the OptionsWidgetsTest changes. Interdiff replaces the one in #1, it just shows one less change vs. the initially split patch.
Also I (or someone) should add the people who worked on the original issue to this so they get commit credits!
Comment #9
star-szrAdding suggested commit message.
Comment #10
star-szrNeeded a reroll.
Comment #11
cilefen CreditAttribution: cilefen commentedAnother reroll was needed.
Comment #12
davidhernandezAnother reroll.
Comment #13
davidhernandezJust making a note. The patch still applies. I also don't find any instances that were missed. I'd rtbc it, but I'm trying to figure out how to manually test the things Emma wasn't able to test in #7. Some of these are not very obvious.
Comment #14
YesCT CreditAttribution: YesCT commentedI think the javascript for the advanced settings details were these
which I git bisected to find were removed in #1838114: Change node form’s vertical tabs into details elements
Comment #15
alimac CreditAttribution: alimac at University of Illinois at Chicago commentedThe commit before that is:
I am going to check out this commit hash and find out which Javascript functions from the files mentioned in #7 were used by the advanced settings in the screenshot from #14.
Comment #16
alimac CreditAttribution: alimac at University of Illinois at Chicago commentedI got a site up and running from commit fe15dd23de065d29890a90918b71583b1b43fb23.
I am looking at the Javascript files listed in #7 by @emma.maria. First up
core/modules/node/node.js
I made some modifications to insert this issue number and my user name into the strings that this JS inserts into the page. See screenshot for results, and the attached file shows what I altered. So, the functionality of this JS file is tied to the advanced summary which was removed in #1838114: Change node form’s vertical tabs into details elements
Comment #17
alimac CreditAttribution: alimac at University of Illinois at Chicago commentedChecking another one
core/modules/path/path.js
This one controls the URL Path Settings section.
Comment #18
alimac CreditAttribution: alimac at University of Illinois at Chicago commentedThe other 3 don't exist in this version of Drupal 8, so we need to look for them elsewhere:
Comment #19
alimac CreditAttribution: alimac at University of Illinois at Chicago commentedThe common thread in all of these files is
drupalSetSummary()
which is no longer used because #1838114: Change node form’s vertical tabs into details elementsSo, we should make a new issue to remove all the unused Javascript from these files (in some cases, remove the entire file). Looking for this function in HEAD, I found additional files that should be checked:
I think the changes in the patch can stay and we can remove the unused JS as part of a follow-up issue.
I am marking this issue as RTBC, and adding Needs followup to create this follow-up issue.
Comment #20
alimac CreditAttribution: alimac at University of Illinois at Chicago commentedI created the follow-up issue: #2540170: Remove unused JS after node form’s vertical tabs were changed into details elements
Comment #21
alimac CreditAttribution: alimac at University of Illinois at Chicago commented@emma.maria did manual testing in #7, and I verified that the remaining JS code is not in use, and will be dealt with in a follow-up issue. So, I am removing Needs manual testing tag.
Comment #22
alexpottWe have css in modules that relies on form-item-BLAH - so I don't think we can do this removal in D8. For example, dblog.module.css, block-admin.css and locale.admin.css.
Comment #23
rteijeiro CreditAttribution: rteijeiro at Tieto commentedFixed @alexpott indication in #22.
Comment #24
YesCT CreditAttribution: YesCT commentedSince the credits are stored in the d.o database (and in the commit message), I think it might help the committers more, to list the additional people that should be credited, instead of a whole suggested commit message... and we should ask the people we want to give credit to, to comment on this issue cause ... #2474609: Not possible to credit people who didn't comment in an issue I updated the issue summary and I'll contact mortendk and rachel_norfolk and ask them to make a comment.
Comment #25
mortendk CreditAttribution: mortendk as a volunteer commentedheres my comment and also YEAH! to see the last of this getting done
Comment #26
rachel_norfolk*Really* pleased to see this on progressing - it's been an interesting journey and I've learned lots on the way!
Comment #27
mortendk CreditAttribution: mortendk as a volunteer commented#22 yes we do want both
.js-foo
and.foo
to both be there so a visual changes can targeted.Comment #28
rachel_norfolkIn reviewing, I found the patch needed re-rolling.
It's very closely conjoined with https://www.drupal.org/node/2473951, which was recently committed, and clashed a bit in field_ui.js and I think I've chosen the correct things to copy across, if someone can check...
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous as a volunteer commentedJust tested this patch, applies fine. Does indeed seem to do what it is supposed to, I checked through all instances of 'form-item-*' in core, and does highlight the difference between the js and styling classes.
Comment #30
rachel_norfolkQuite a lot of conflicts (we've all been very busy!) meant a fun reroll...
Comment #33
davidhernandezrerolling
Comment #34
RainbowArrayManual review of the code changes looks good to me.
Things to do:
- Grep for changed classes to make sure CSS does not rely on them?
- Visual regression testing?
Comment #35
akalata CreditAttribution: akalata commentedReviewing per #34.
Comment #36
akalata CreditAttribution: akalata commentedI've gone through and captured screenshots and html snippets to compare for the various changes.
Changes that were tested successfully
Changes that could not be fully tested
While I was able to review the visual and HTML changes, the javascript changes could not be confirmed since
<details>
to not have summary/error handling - see #2493957: Add back errors support to native HTML5 details tag.Other comments
The fieldset that this javascript is looking for does not exist - it's not being altered at it should be according to BlockContentTranslationHandler. This issue exists in head.
I could not find where this functionality is being used.
Since all of my tests were done in Seven, this template was not tested. However, this patch removes form-item-name in favor of form-itemname (no hyphen) which I'm thinking should not be the case?
Comment #37
davidhernandezAre the things you couldn't test the same as what alimac reported earlier? I think some of that is orphaned JS.
'form-item' ~ name|clean_class,
Yeah, that looks like a typo.
Comment #38
davidhernandezFixing that typo.
Comment #39
akalata CreditAttribution: akalata commented@ #37, yes, those items do seem to be a part of what alimac had referenced in #20 (the issue she created was marked won't fix in favor of the other issue I've referenced). I just wanted to document that lack of ability to manual test for completness' sake.
Comment #40
LewisNyman CreditAttribution: LewisNyman at Wunder commentedThanks for the work everyone.
Comment #42
LewisNyman CreditAttribution: LewisNyman at Wunder commentedComment #44
akalata CreditAttribution: akalata commentedComment #46
alexpottCommitted 6a4315d and pushed to 8.0.x. Thanks!
Fixed indentation on commit.
Comment #47
davidhernandezWas that commit pushed?