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

  1. Navigate to admin/structure/types/manage/article/fields/add-field
  2. Test that the machine name functionality still works

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug report, there is currently a class missing from the non-Classy text-format-wrapper.html.twig.
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

Files: 
CommentFileSizeAuthor
#38 prefix_form_item-2473945-38.patch20.09 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,850 pass(es). View
#35 prefix_form_item-2473945-35.patch19.54 KBrachel_norfolk
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,464 pass(es). View
#34 prefix_form_item-2473945-34.patch20.09 KBrachel_norfolk
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,431 pass(es). View
#10 2473945-form-item-split-10.patch20.41 KBemma.maria
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2473945-form-item-split-10.patch. Unable to apply patch. See the log in the details link for more information. View
#6 2473945-form-item-split.patch20.3 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2473945-form-item-split_0.patch. Unable to apply patch. See the log in the details link for more information. View
#1 2473945-form-item-additional.patch21.95 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,641 pass(es). View

Comments

Cottser’s picture

Status: Active » Needs review
Issue tags: +banana
FileSize
20.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,623 pass(es), 2 fail(s), and 0 exception(s). View
21.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,641 pass(es). View
1.65 KB

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

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

LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Needs work

I manually tested the patch and it works. I also grepped and couldn't find any other changes.

+++ b/core/modules/options/src/Tests/OptionsWidgetsTest.php
@@ -477,8 +477,8 @@ function testEmptyValue() {
-    $this->assertTrue($this->xpath('//div[@id=:id]//div[@class=:class]//input[@value=:value]', array(':id' => 'edit-card-1', ':class' => 'form-item form-type-radio form-item-card-1', ':value' => '_none')), 'A test radio button has a "None" choice.');
-    $this->assertTrue($this->xpath('//div[@id=:id]//div[@class=:class]//label[@for=:for and text()=:label]', array(':id' => 'edit-card-1', ':class' => 'form-item form-type-radio form-item-card-1', ':for' => 'edit-card-1-none', ':label' => 'N/A')), 'A test radio button has a "N/A" choice.');
+    $this->assertTrue($this->xpath('//div[@id=:id]//div[@class=:class]//input[@value=:value]', array(':id' => 'edit-card-1', ':class' => 'js-form-item form-item form-type-radio form-item-card-1', ':value' => '_none')), 'A test radio button has a "None" choice.');
+    $this->assertTrue($this->xpath('//div[@id=:id]//div[@class=:class]//label[@for=:for and text()=:label]', array(':id' => 'edit-card-1', ':class' => 'js-form-item form-item form-type-radio form-item-card-1', ':for' => 'edit-card-1-none', ':label' => 'N/A')), 'A test radio button has a "N/A" choice.');

I think here we could also just test for the functional class as well?

Cottser’s picture

Yeah IMO that XPath shouldn't care that much about the classes. Updating that in a separate issue would make 2-3 of these patches smaller.

Cottser’s picture

Cottser’s picture

Status: Needs work » Needs review
FileSize
20.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2473945-form-item-split_0.patch. Unable to apply patch. See the log in the details link for more information. View

Reuploading the initially split patch after #2474107: Make OptionsWidgetsTest::testEmptyValue() care less about markup was committed.

Also these issues all need commit credits I'm realizing!

Cottser’s picture

Issue summary: View changes

Adding suggested commit message.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Yeah IMO that XPath shouldn't care that much about the classes. Updating that in a separate issue would make 2-3 of these patches smaller.

Ok in that case we can fix the test in a followup. I knight this issue RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2473945-form-item-split.patch, failed testing.

emma.maria’s picture

Status: Needs work » Needs review
FileSize
20.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2473945-form-item-split-10.patch. Unable to apply patch. See the log in the details link for more information. View

I rerolled the patch.

Cottser’s picture

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Great! Thanks.

Cottser’s picture

Category: Task » Bug report
Issue summary: View changes

Changing to bug report because non-Classy text-format-wrapper.html.twig is currently missing a class that is used in JS. Tweaked beta evaluation accordingly.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2473945-form-item-split-10.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
20.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,354 pass(es). View

Another reroll, just a small conflict in core's form-element.html.twig.

Cottser’s picture

FileSize
20.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,590 pass(es). View
Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

cilefen’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
20.09 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,034 pass(es). View

rerolled

davidhernandez’s picture

Just making a note that this one still applies cleanly.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Nice work everyone. This is identical to the pervious patch I RTBC'd

alexpott’s picture

  1. +++ b/core/modules/filter/templates/text-format-wrapper.html.twig
    @@ -15,7 +15,7 @@
    -<div class="js-text-format-wrapper">
    +<div class="js-text-format-wrapper js-form-item">
    

    huh?

  2. +++ b/core/modules/language/templates/language-negotiation-configure-form.html.twig
    @@ -23,6 +23,7 @@
    +      'js-form-item',
           'form-item',
    

    Here we add...

    +++ b/core/modules/system/templates/field-multiple-value-form.html.twig
    @@ -20,7 +20,7 @@
    -  <div class="form-item">
    +  <div class="js-form-item">
    
    +++ b/core/modules/system/templates/fieldset.html.twig
    @@ -24,7 +24,7 @@
    -    'form-item',
    +    'js-form-item',
    
    +++ b/core/modules/system/templates/form-element.html.twig
    @@ -48,7 +48,7 @@
    -    'form-item',
    +    'js-form-item',
    
    +++ b/core/themes/classy/templates/form/fieldset.html.twig
    @@ -22,7 +22,7 @@
    -    'form-item',
    +    'js-form-item',
    

    Here we are taking away. I think all the changes from form-item => js-form-item should instead by additions to be consistent with the rest of the patch.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Cottser’s picture

I think my comment on #2473951-26: Prefix form-required classes with js- applies here as well, but I'd be fine with changing gears and doing additions only in the interests of getting this and similar issues in :)

+++ b/core/themes/classy/templates/form/fieldset.html.twig
@@ -22,7 +22,7 @@
-    'form-item',
+    'js-form-item',

And more specifically this is the only really problematic part I can see because we shouldn't be removing classes from Classy at this point.

axe312’s picture

Assigned: Unassigned » axe312

i am working on this

axe312’s picture

Status: Needs work » Needs review
FileSize
33.96 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,677 pass(es). View
2.71 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch inter.diff. Unable to apply patch. See the log in the details link for more information. View

I added the missing form-item classes and removed the change to the text-format-wrapper which might be added accidentally in the last patch.

Also the patch is now applying to most current dev.

Status: Needs review » Needs work

The last submitted patch, 25: inter.diff, failed testing.

LewisNyman’s picture

Status: Needs work » Needs review
yannickoo’s picture

Assigned: axe312 » Unassigned
LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Ok great. Thanks for working on this.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/classy/templates/content-edit/text-format-wrapper.html.twig
@@ -6,21 +6,21 @@
-<div class="js-text-format-wrapper text-format-wrapper form-item">
+<div class="js-text-format-wrapper js-form-item form-item">

This is about form-item we shouldn't be removing text-format-wrapper here.

yannickoo’s picture

Status: Needs work » Needs review
FileSize
33.98 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,701 pass(es). View
582 bytes

I have added the text-format-wrapper back but I'm kinda confused why there that class is missing in core/modules/filter/templates/text-format-wrapper.html.twig.

davidhernandez’s picture

@yannickoo, thetext-format-wrapper class isn't needed in the core version of the template. It is in the Classy version.

+++ b/core/modules/filter/templates/text-format-wrapper.html.twig
@@ -8,16 +8,16 @@
-<div class="js-text-format-wrapper">
+<div class="js-text-format-wrapper js-form-item form-item">

I don't understand why these form-item classes are being added here. All the patches in this issue have this. Am I missing something?

alexpott’s picture

+++ b/core/modules/filter/filter.filter_html.admin.js
@@ -64,21 +64,21 @@
-        that.$allowedHTMLDescription = that.$allowedHTMLFormItem.closest('.form-item').find('.description');
+        that.$allowedHTMLDescription = that.$allowedHTMLFormItem.closest('.js-form-item').find('.description');

@davidhernandez I think this javascript was broken by a previous over-zealous removal of classes. This one #2349677: Copy filter templates to Classy

rachel_norfolk’s picture

FileSize
20.09 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,431 pass(es). View

I'm kinda hoping I've got this re-roll correct...

rachel_norfolk’s picture

FileSize
19.54 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,464 pass(es). View
1.12 KB

I have looked into the comments at #34 and can't find any need for the form-item and js-form-item classes to appear on the wrappers, as @davidhernandez mentions. The actual form items within that group contain the classes, anyway.

I wasn't able to work out the issue at #33 and maybe I'm just not seeing a problem in my own manual testing?

Cottser’s picture

Issue summary: View changes

Thanks @rachel_norfolk :)

+++ b/core/themes/classy/templates/content-edit/text-format-wrapper.html.twig
@@ -13,7 +13,7 @@
-<div class="js-text-format-wrapper text-format-wrapper form-item">
+<div class="js-text-format-wrapper text-format-wrapper">

Based on #33 it sounds like this should stay and a js-form-item be added, and these classes also be added to the core template. I don't know how to manually test that JS though.

LewisNyman’s picture

Status: Needs review » Needs work

Sounds like this needs work to add these classes back in.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
20.09 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,850 pass(es). View
1.12 KB

Added the classes back.

akalata’s picture

Currently doing some in-depth manual testing.

akalata’s picture

I've manually tested the changes in the following files:

Javacript

core/misc/machine-name.js via admin/structure/types/manage/article/fields/add-field
core/modules/filter/filter.filter_html.admin.js via node/add/page
core/modules/locale/locale.admin.js via admin/config/regional/translate

Modules/System/other default using Stark as admin theme

core/modules/filter/templates/text-format-wrapper.html.twig via node/add/page
core/modules/system/form-element.html.twig via admin/structure/types/manage/article/fields/add-field

Classy as admin theme

core/themes/classy/templates/content-edit/text-format-wrapper.html.twig via node/add/page
core/themes/classy/templates/form/form-element.html.twig via admin/structure/types/manage/article/fields/add-field

I was able to partially test the following:

core/misc/states.js I tested the :visible, but could not find a usage of :required or :disabled as actual state changes -- I'm thinking this is what @cottser meant in #36?

core/modules/system/fieldset.html.twig and core/themes/classy/templates/form/fieldset.html.twig The html changes were good (tested at search/node), but I could not find a collapsing fieldset in order to test related javascript (that I'm assuming exists).

core/modules/system/templates/field-multiple-value-form.html.twig
and core/themes/classy/templates/form/field-multiple.value-form.html.twig The html changes were good (tested by adding a multi-value text field), but I'm not sure that any javascript was updated so that it actually is looking for this class. (I removed .form-item and it worked, so maybe it's not using either class?)

I also tested the html in about half the files listed below, but I have to ask does it really make sense to add the js- to these markup wrappers? I know they're being added everywhere else for consistency via form-element.html.twig; if people think they should stay I can finish up that testing for good measure.

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -154,7 +154,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
-        '#markup' => '<div class="form-item">' . $this->t('An invalid vocabulary is selected. Please change it in the options.') . '</div>',
+        '#markup' => '<div class="js-form-item form-item">' . $this->t('An invalid vocabulary is selected. Please change it in the options.') . '</div>',

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -1544,7 +1544,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-            '#prefix' => '<div class="form-item description">',
+            '#prefix' => '<div class="js-form-item form-item description">',

@@ -1581,7 +1581,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-            '#prefix' => '<div class="form-item description">',
+            '#prefix' => '<div class="js-form-item form-item description">',

@@ -1653,7 +1653,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-            '#prefix' => '<div class="form-item description">',
+            '#prefix' => '<div class="js-form-item form-item description">',

@@ -1701,7 +1701,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-            '#prefix' => '<div class="form-item description">',
+            '#prefix' => '<div class="js-form-item form-item description">',

@@ -1768,7 +1768,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-          '#markup' => '<div class="description form-item">' . $this->t('If set, any exposed widgets will not appear with this view. Instead, a block will be made available to the Drupal block administration system, and the exposed form will appear there. Note that this block must be enabled manually, Views will not enable it for you.') . '</div>',
+          '#markup' => '<div class="js-form-item form-item description">' . $this->t('If set, any exposed widgets will not appear with this view. Instead, a block will be made available to the Drupal block administration system, and the exposed form will appear there. Note that this block must be enabled manually, Views will not enable it for you.') . '</div>',

@@ -1796,7 +1796,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-            '#prefix' => '<div class="form-item description">',
+            '#prefix' => '<div class="js-form-item form-item description">',

@@ -1832,7 +1832,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-            '#prefix' => '<div class="form-item description">',
+            '#prefix' => '<div class="js-form-item form-item description">',

+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -368,7 +368,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-          '#markup' => '<div class="form-item description">' . $this->t('When providing a menu item as a tab, Drupal needs to know what the parent menu item of that tab will be. Sometimes the parent will already exist, but other times you will need to have one created. The path of a parent item will always be the same path with the last part left off. i.e, if the path to this view is <em>foo/bar/baz</em>, the parent path would be <em>foo/bar</em>.') . '</div>',
+          '#markup' => '<div class="js-form-item form-item description">' . $this->t('When providing a menu item as a tab, Drupal needs to know what the parent menu item of that tab will be. Sometimes the parent will already exist, but other times you will need to have one created. The path of a parent item will always be the same path with the last part left off. i.e, if the path to this view is <em>foo/bar/baz</em>, the parent path would be <em>foo/bar</em>.') . '</div>',

+++ b/core/modules/views/src/Plugin/views/style/Table.php
@@ -413,7 +413,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-      '#markup' => '<div class="description form-item">' . $this->t('Place fields into columns; you may combine multiple fields into the same column. If you do, the separator in the column specified will be used to separate the fields. Check the sortable box to make that column click sortable, and check the default sort radio to determine which column will be sorted by default, if any. You may control column order and field labels in the fields section.') . '</div>',
+      '#markup' => '<div class="js-form-item form-item description">' . $this->t('Place fields into columns; you may combine multiple fields into the same column. If you do, the separator in the column specified will be used to separate the fields. Check the sortable box to make that column click sortable, and check the default sort radio to determine which column will be sorted by default, if any. You may control column order and field labels in the fields section.') . '</div>',

+++ b/core/modules/views_ui/src/Form/Ajax/AddHandler.php
@@ -162,7 +162,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-        '#markup' => '<div class="form-item">' . $this->t('There are no @types available to add.', array('@types' =>  $ltitle)) . '</div>',
+        '#markup' => '<div class="js-form-item form-item">' . $this->t('There are no @types available to add.', array('@types' =>  $ltitle)) . '</div>',

+++ b/core/modules/views_ui/src/Form/Ajax/Analyze.php
@@ -44,7 +44,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      '#prefix' => '<div class="form-item">',
+      '#prefix' => '<div class="js-form-item form-item">',

+++ b/core/modules/views_ui/src/Form/Ajax/ConfigHandler.php
@@ -160,7 +160,7 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
-            '#attributes' => array('class' => array('form-item description')),
+            '#attributes' => array('class' => array('js-form-item form-item description')),
jaxxed’s picture

Should we also look at this from the other side: what JS calls are looking for .form-item, when they could/should be looking for .js-form-item?

davidhernandez’s picture

@jaxxed, yes, the patch should update any JS that is looking for the class. It looks like in your txt file you are finding classes that being with form-item-*. There is a separate issue for that. #2473947: Prefix form-item-* classes with js-

This issue is just for the exact "form-item" classes.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

@jaxxed Yep, the javascript should be calling .js-form-item instead of .form-item. It looks like your grep only turned up classes that start with form-item-*. Those are being covered in #2473947: Prefix form-item-* classes with js-. Maybe that is a good one to review next.

Setting this to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 44e0f2f and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 44e0f2f on 8.0.x
    Issue #2473945 by Cottser, rachel_norfolk, axe312, davidhernandez,...

Status: Fixed » Closed (fixed)

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