Prefix form-item-* classes with js-

Reviewing.

I 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.

Another reroll was needed.

Another reroll.

Just 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.

I 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

The commit before that is:

commit fe15dd23de065d29890a90918b71583b1b43fb23
Author: webchick <webchick@24967.no-reply.drupal.org>
Date:   Thu Jan 17 13:45:26 2013 -0800

    Issue #1886108 by linclark, scor: Rewrite CommentAttributesTest to parse RDFa.

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.

I 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

Checking another one core/modules/path/path.js

This one controls the URL Path Settings section.

The other 3 don't exist in this version of Drupal 8, so we need to look for them elsewhere:

• core/modules/comment/comment-entity-form.js
• core/modules/menu_ui/menu_ui.js
• core/modules/views_ui/js/views-admin.js

The common thread in all of these files is drupalSetSummary() which is no longer used because #1838114: Change node form’s vertical tabs into details elements

• core/modules/comment/comment-entity-form.js
• core/modules/menu_ui/menu_ui.js
• core/modules/node/node.js
• core/modules/path/path.js
• core/modules/views_ui/js/views-admin.js

So, 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:

ag drupalSetSummary core/modules/

core/modules/block/block.js
10:    // The drupalSetSummary method required for this behavior is not available
12:    // behavior is processed only if drupalSetSummary is defined.
13:    if (typeof jQuery.fn.drupalSetSummary === 'undefined') {
29:    $('#edit-visibility-node-type').drupalSetSummary(checkboxesSummary);
30:    $('#edit-visibility-language').drupalSetSummary(checkboxesSummary);
31:    $('#edit-visibility-role').drupalSetSummary(checkboxesSummary);
33:    $('#edit-visibility-path').drupalSetSummary(function (context) {

core/modules/book/book.js
12:    $(context).find('.book-outline-form').drupalSetSummary(function (context) {

core/modules/comment/comment-node-form.js
13:    $context.find('.comment-node-settings-form').drupalSetSummary(function (context) {
18:    $context.find('.comment-node-type-settings-form').drupalSetSummary(function(context) {

core/modules/filter/filter.admin.js
41:        tab.details.drupalSetSummary(function (tabContext) {

core/modules/menu/menu.js
7:    $(context).find('.menu-link-form').drupalSetSummary(function (context) {

core/modules/node/content_types.js
14:    $context.find('#edit-submission').drupalSetSummary(function(context) {
19:    $context.find('#edit-workflow').drupalSetSummary(function(context) {
29:    $('#edit-language', context).drupalSetSummary(function(context) {
40:    $context.find('#edit-display').drupalSetSummary(function(context) {

core/modules/node/node.js
13:    $context.find('.node-form-revision-information').drupalSetSummary(function (context) {
28:    $context.find('.node-form-author').drupalSetSummary(function (context) {
37:    $context.find('.node-form-options').drupalSetSummary(function (context) {
51:    $context.find('fieldset.node-translation-options').drupalSetSummary(function (context) {

core/modules/path/path.js
11:    $(context).find('.path-form').drupalSetSummary(function (context) {

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.

I created the follow-up issue: #2540170: Remove unused JS after node form’s vertical tabs were changed into details elements

@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.

+++ b/core/modules/system/templates/form-element.html.twig
@@ -50,7 +50,7 @@
-    'form-item-' ~ name|clean_class,
+    'js-form-item-' ~ name|clean_class,

We 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.

Fixed @alexpott indication in #22.

Since 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.

heres my comment and also YEAH! to see the last of this getting done

*Really* pleased to see this on progressing - it's been an interesting journey and I've learned lots on the way!

#22 yes we do want both .js-foo and .foo to both be there so a visual changes can targeted.

In 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...

Quite a lot of conflicts (we've all been very busy!) meant a fun reroll...

rerolling

Manual 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?

Reviewing per #34.

I've gone through and captured screenshots and html snippets to compare for the various changes.

Changes that were tested successfully

• block_content.js, revisions vertical tab summary: HTML, visual appearance and JS interaction confirmed
• ckeditor.admin.js, hide toolbar button configuration textfield: HTML, visual appearance and JS interaction confirmed
• field_ui.js, labels for new field, new field with machine name error label, and reuse existing field: HTML and visual appearance confirmed
• tour.tour.language.yml, HTML and visual appearance of tour confirmed for admin/config/regional/language
• menu_ui.js, Auto-fill the title: HTML, visual appearance and JS interaction confirmed
• content_types.js, Content type edit form virtual tabs: HTML, visual appearance and JS interaction confirmed
• views-admin.js, Rearrange filters and Select All option for filters: HTML visual appearance and JS interactions confirmed

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.

• menu_ui.js, menu settings summary
• node.js, revisions and translation status summary
• path.js, display path alias summary

Other comments

1. +++ b/core/modules/block_content/js/block_content.js
   @@ -40,13 +39,13 @@
          $context.find('fieldset.block-content-translation-options').drupalSetSummary(function (context) {
            var $translationContext = $(context);
            var translate;
   -        var $checkbox = $translationContext.find('.form-item-translation-translate input');
   +        var $checkbox = $translationContext.find('.js-form-item-translation-translate input');
    
            if ($checkbox.size()) {
              translate = $checkbox.is(':checked') ? Drupal.t('Needs to be updated') : Drupal.t('Does not need to be updated');
            }
            else {
   -          $checkbox = $translationContext.find('.form-item-translation-retranslate input');
   +          $checkbox = $translationContext.find('.js-form-item-translation-retranslate input');
              translate = $checkbox.is(':checked') ? Drupal.t('Flag other translations as outdated') : Drupal.t('Do not flag other translations as outdated');
            }
   

   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.

2. +++ b/core/modules/comment/comment-entity-form.js
   @@ -15,7 +15,7 @@
   -        return Drupal.checkPlain($(context).find('.form-item-comment input:checked').next('label').text());
   +        return Drupal.checkPlain($(context).find('.js-form-item-comment input:checked').next('label').text());
   

   I could not find where this functionality is being used.

3. +++ b/core/modules/system/templates/form-element.html.twig
   @@ -51,7 +51,8 @@
   -    'form-item-' ~ name|clean_class,
   +    'form-item' ~ name|clean_class,
   +    'js-form-item-' ~ name|clean_class,
   

   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?

Are 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.

Fixing that typo.

@ #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.

Thanks for the work everyone.

Committed 6a4315d and pushed to 8.0.x. Thanks!

diff --git a/core/modules/menu_ui/menu_ui.js b/core/modules/menu_ui/menu_ui.js
index aca9f61..935d1a3 100644
--- a/core/modules/menu_ui/menu_ui.js
+++ b/core/modules/menu_ui/menu_ui.js
@@ -46,9 +46,9 @@
         // Try to find menu settings widget elements as well as a 'title' field
         // in the form, but play nicely with user permissions and form
         // alterations.
-          var $checkbox = $this.find('.js-form-item-menu-enabled input');
-          var $link_title = $context.find('.js-form-item-menu-title input');
-          var $title = $this.closest('form').find('.js-form-item-title-0-value input');
+        var $checkbox = $this.find('.js-form-item-menu-enabled input');
+        var $link_title = $context.find('.js-form-item-menu-title input');
+        var $title = $this.closest('form').find('.js-form-item-title-0-value input');
         // Bail out if we do not have all required fields.
         if (!($checkbox.length && $link_title.length && $title.length)) {
           return;

Fixed indentation on commit.

Was that commit pushed?