Issue #2473951 by Cottser, cilefen, yannickoo, davidhernandez, LewisNyman: Prefix form-required classes with js-

Task

Prefix form-required 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. Install the Language module
  2. Navigate /admin/config/regional/language/add
  3. Select "Custom language"
  4. Observe "js-form-required" in the mark up

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

Files: 
CommentFileSizeAuthor
#30 drupal-prefix_form_required-2473951-30.patch20.61 KByannickoo
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,674 pass(es). View
#15 interdiff.txt1.58 KBCottser
#15 prefix_form_required-2473951-15.patch12.99 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,264 pass(es). View
#9 2473951-7.patch11.41 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,269 pass(es), 2 fail(s), and 0 exception(s). View
#7 interdiff.txt727 bytesCottser
#7 2473951-7.patch727 bytesCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2473951-7.patch. Unable to apply patch. See the log in the details link for more information. View
#1 2473951-form-required-split.patch11.41 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,486 pass(es). View

Comments

Cottser’s picture

Status: Active » Needs review
FileSize
11.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,486 pass(es). View

Patch split from parent issue. I wasn't able to find anything additional by grepping.

LewisNyman’s picture

Issue summary: View changes

I manually tested this patch, and I can't really figure out how this is used in Javascript. I can see the js-form-required class added but all Javascript seems to do is add/remove the form-required class for styling. It doesn't seem to use it for functionality.

Can @nod_ confirm?

Cottser’s picture

Issue summary: View changes

Adding suggested commit message.

LewisNyman’s picture

Assigned: Unassigned » nod_

Save us nod!

nod_’s picture

@lewis: indeed. I can't see form-required used for JS functionality.

The form-required is hardcoded in the states.js file though, that's ok with you? I see it's removed from the default datetime wrapper thing, just asking.

LewisNyman’s picture

Assigned: nod_ » Unassigned
Status: Needs review » Reviewed & tested by the community

Ok, thanks for clarifying that. I'm ok with this patch, we can always improve this separation down the road, this is not a good time to get our hands dirty in states.js :)

Cottser’s picture

FileSize
727 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2473951-7.patch. Unable to apply patch. See the log in the details link for more information. View
727 bytes

Small docs touch up.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2473951-7.patch, failed testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,269 pass(es), 2 fail(s), and 0 exception(s). View

Two interdiffs :(

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2473951-7.patch, failed testing.

Status: Needs work » Needs review

Cottser queued 9: 2473951-7.patch for re-testing.

Cottser’s picture

Looked like an unrelated/random fail.

mortendk’s picture

Status: Needs review » Reviewed & tested by the community

works as expected also in stark
and yes we can fix & pretty up more further down the road.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2473951-7.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
3.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,261 pass(es), 2 fail(s), and 0 exception(s). View
12.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,264 pass(es). View
1.58 KB

A test got added in #980144: Issues with "required, multiple" fields in forms using assertRaw for exact markup. Refactoring to use XPath. The attached test-only patch reverts the bug fix part from #980144: Issues with "required, multiple" fields in forms to prove that the revised test still catches the bug fixed there.

Cottser’s picture

FileSize
3.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,251 pass(es), 1 fail(s), and 0 exception(s). View

Okay so the test-only patch has to not use js-form-required, it's late :) still fails, should be only 1 fail like on #980144-47: Issues with "required, multiple" fields in forms.

The last submitted patch, 15: prefix_form_required-2473951-15-refactored-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 16: prefix_form_required-2473951-16-refactored-test-only.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review

Alright so the test only patches did what they were supposed to. Back to needs review.

cilefen’s picture

FileSize
13.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,928 pass(es). View

reroll

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
13.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,024 pass(es). View

rerolled

davidhernandez’s picture

FileSize
13.07 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,013 pass(es). View

This needed rerolling.

LewisNyman’s picture

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

Thanks. I manually tested this patch and the JS still kicks in.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/includes/theme.inc
@@ -1581,6 +1581,7 @@ function template_preprocess_field_multiple_value_form(&$variables) {
+      $header_attributes['class'][] = 'js-form-required';
       $header_attributes['class'][] = 'form-required';

+++ b/core/modules/system/templates/datetime-wrapper.html.twig
@@ -17,7 +17,7 @@
-    required ? 'form-required',
+    required ? 'js-form-required',

+++ b/core/modules/system/templates/fieldset.html.twig
@@ -33,7 +33,7 @@
-      required ? 'form-required',
+      required ? 'js-form-required',

+++ b/core/modules/system/templates/form-element-label.html.twig
@@ -18,7 +18,7 @@
-    required ? 'form-required',
+    required ? 'js-form-required',

This seems inconsistent - we're adding both classes in the form API but we're not in the default templates - how come?

Cottser’s picture

Because we can only make the differentiation between Classy and default when we're in the theme layer (for practical purposes this means preprocess and templates).

The parent issue shows the plan for Classy vs. core templates.

Edit: also we've been getting patches committed that treat Classy markup differently than markup in this meta…

#2473949: Prefix form-type-* classes with js-
#2473957: Prefix text-* classes with js-

alexpott’s picture

@Cottser yes but those issue completely removed the class they were changing from core - this issue (and the other) has not managed to do that. Therefore the current patch leaves core very inconsistent wrt to whether the form-required class is there.

Cottser’s picture

@alexpott thanks! I thnk form-type-* completely removed it but text-* didn't, look at the field plugins.

As I said on #2473945: Prefix form-item classes with js- happy to change course to get this wrapped up :)

LewisNyman’s picture

Status: Needs review » Needs work
yannickoo’s picture

Status: Needs work » Needs review
FileSize
20.61 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,674 pass(es). View
2.26 KB

I have added the form-required class back to 3 twig templates :)

LewisNyman’s picture

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

Simple! Thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 01e703d and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 01e703d on 8.0.x
    Issue #2473951 by Cottser, cilefen, yannickoo, davidhernandez,...

Status: Fixed » Closed (fixed)

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