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

Reference: https://www.drupal.org/core/beta-changes
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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Status: Active » Needs review
Issue tags: +banana
FileSize
14.8 KB
17.89 KB
3.1 KB

Initial patch split from the parent, some additional changes from grepping, and interdiff between the two.

The last submitted patch, 1: 2473947-form-item--split.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2473947-form-item--additional.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review

Random test fail from \Drupal\simpletest\Tests\SimpleTestBrowserTest, something about https.

emma.maria’s picture

Reviewing.

emma.maria’s picture

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.

star-szr’s picture

Thanks 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!

star-szr’s picture

Issue summary: View changes

Adding suggested commit message.

star-szr’s picture

Needed a reroll.

cilefen’s picture

Another reroll was needed.

davidhernandez’s picture

Another reroll.

davidhernandez’s picture

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.

YesCT’s picture

FileSize
88.02 KB

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

alimac’s picture

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.

alimac’s picture

Issue summary: View changes
FileSize
133.04 KB
1.41 KB

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

alimac’s picture

Issue summary: View changes
FileSize
397 bytes
119.55 KB

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

This one controls the URL Path Settings section.

alimac’s picture

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
alimac’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

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.

alimac’s picture

alimac’s picture

Issue tags: -Needs manual testing

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
555 bytes
16.16 KB

Fixed @alexpott indication in #22.

YesCT’s picture

Issue summary: View changes

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.

mortendk’s picture

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

rachel_norfolk’s picture

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

mortendk’s picture

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

rachel_norfolk’s picture

FileSize
16.13 KB

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

Anonymous’s picture

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

rachel_norfolk’s picture

FileSize
17.09 KB

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

Status: Needs review » Needs work

The last submitted patch, 30: prefix_form_item-2473947-30.patch, failed testing.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
17.09 KB

rerolling

RainbowArray’s picture

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?

akalata’s picture

Reviewing per #34.

akalata’s picture

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?

davidhernandez’s picture

Status: Needs review » Needs work

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.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
17.04 KB
595 bytes

Fixing that typo.

akalata’s picture

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

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the work everyone.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: prefix_form_item-2473947-38.patch, failed testing.

LewisNyman’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: prefix_form_item-2473947-38.patch, failed testing.

akalata’s picture

Status: Needs work » Reviewed & tested by the community

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

davidhernandez’s picture

Was that commit pushed?

  • alexpott committed b9ae4cd on 8.0.x
    Issue #2473947 by Cottser, davidhernandez, rachel_norfolk, rteijeiro,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.