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

CommentFileSizeAuthor
#30 interdiff.txt2.26 KByannickoo
#30 drupal-prefix_form_required-2473951-30.patch20.61 KByannickoo
#23 prefix_form_required-2473951-23.patch13.07 KBdavidhernandez
#22 prefix_form_required-2473951-22.patch13.05 KBcilefen
#20 prefix_form_required-2473951-20.patch13.05 KBcilefen
#16 prefix_form_required-2473951-16-refactored-test-only.patch3.18 KBstar-szr
#15 interdiff.txt1.58 KBstar-szr
#15 prefix_form_required-2473951-15.patch12.99 KBstar-szr
#15 prefix_form_required-2473951-15-refactored-test-only.patch3.19 KBstar-szr
#9 2473951-7.patch11.41 KBstar-szr
#7 interdiff.txt727 bytesstar-szr
#7 2473951-7.patch727 bytesstar-szr
#1 2473951-form-required-split.patch11.41 KBstar-szr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Active » Needs review
FileSize
2473951-form-required-split.patch11.41 KB

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

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
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?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Issue summary: View changes

Adding suggested commit message.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Assigned: Unassigned » nod_

Save us nod!

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented

@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

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
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 :)

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
FileSize
2473951-7.patch727 bytes
interdiff.txt727 bytes

Small docs touch up.

Status: Reviewed & tested by the community » Needs work

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

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Status: Needs work » Reviewed & tested by the community
FileSize
2473951-7.patch11.41 KB

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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented

Looked like an unrelated/random fail.

mortendk’s picture

mortendk CreditAttribution: mortendk as a volunteer commented
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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs work » Needs review
FileSize
prefix_form_required-2473951-15-refactored-test-only.patch3.19 KB
prefix_form_required-2473951-15.patch12.99 KB
interdiff.txt1.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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
FileSize
prefix_form_required-2473951-16-refactored-test-only.patch3.18 KB

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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs work » Needs review

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

cilefen’s picture

cilefen CreditAttribution: cilefen commented
FileSize
prefix_form_required-2473951-20.patch13.05 KB

reroll

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
prefix_form_required-2473951-22.patch13.05 KB

rerolled

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented
FileSize
prefix_form_required-2473951-23.patch13.07 KB

This needed rerolling.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
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?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

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

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

@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

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Needs work

Needs work based on the consensus in #2431671: [meta] Add in js- prefixed classes for separation of JS & CSS functionality

yannickoo’s picture

yannickoo
Location Berlin
CreditAttribution: yannickoo at Wunder commented
Status: Needs work » Needs review
FileSize
drupal-prefix_form_required-2473951-30.patch20.61 KB
interdiff.txt2.26 KB

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

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Simple! Thanks.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
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.