Prefix form-item classes with js-

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?

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.

I rerolled the patch.

Great! Thanks.

rerolled

Just making a note that this one still applies cleanly.

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

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.

i am working on this

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.

Ok great. Thanks for working on this.

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

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.

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

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

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

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?

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

Added the classes back.

Currently doing some in-depth manual testing.

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')),

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?

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

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

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

Thanks for adding the beta evaluation to the issue summary.