Unnecessary <strong> element in Umami's form-element template may produce invalid markup

Thanks @huzooka for the patch.

Patch applied successfully and now I can see templates without strong element.

Govind

We need a change record for this change.

Updated and still works with 8.7.x

I ll work on this issue .

Please find the patch.

Updated coding standard. Have a look and please review it.

@Swapnil_Kotwal Thank you for your contribution! However, after looking at #17, I'm not sure I understand how the changes you've made are relevant to this issue. Drupal's issue scoping guidelines can be found in this documentation page.

Patch from #15 still needs review.

@Lauri Eskola (lauriii) Thank you for your review. But just FYI there were 6 coding standard issues in patch #15, I have updated patch (classy-form-error-strong-3010558-16.patch) and resolved those coding standard issues.
Hereby attached a screenshot for the same.
Kindly review the updated patch.

These are coding standards problems with the pre-existing code. Coding standards problems not introduced by new changes should be fixed in their own issues. Feel free to open a new issue that contains fixes you've made and link it here. I'll provide review for your changes there. 🙂

Re-queued patch #15 for re-testing. Coding standards indeed need to be taken separately. Let's wait for the test outcome. Code wise patch 15 looks ok to me.

#15 passed testing. In that case I would say it's RTBC.

https://www.drupal.org/pift-ci-job/1199534

I ran into the same thing at #2419131-99: [PP-1] #states attribute does not work on #type datetime. Glad to see this is a separate issue to fix that part. ;)

Added the requested change notice. Reviews and fixes welcome.

We've had a bunch of patches without interdiffs in here. The difference between #11 and #15 is that 15 also touches the one template in seven with the same problem: core/themes/seven/templates/details.html.twig

The issue title doesn't reflect this, but I think it's still in scope for this issue (since seven is a subtheme of classy, and it's the same bug). I mentioned that template in the CR for good measure (even though seven is "internal" and in theory, no one should care).

Generally, +1 to the RTBC of #15.

Thanks, everyone!

Cheers,
-Derek

Thanks. Works well with 8.7.x

Quick question, is there a reason to not displaying a colour?

/* Form error message styles. */
.form-item--error-message {
  color: #e32700;
}

Re: #25: That's way out of scope for this issue. This is simply about removing bogus <strong> tags. Colors have nothing to do with this.

Cheers,
-Derek

Make sense, thanks.

About the change in seven

/core/themes/seven/css/components/form.css

.form-item--error-message {
  margin-top: 0.15em;
  color: #e32700;
}

font-weight: bold; is missing?

Re: #27 sure, good point. ;)

This is such a trivial change, I'm leaving it at RTBC, even though it's my patch.

I'll let @lauriii decide if they like it. :)

Thanks,
-Derek

Changes look good, so still RTBC :)

Will look at this.

Re #30:
Removed the unnecessary font-weight: bold change.

Sorry I was being hasty in #28. #32 is identical to #15. Agreed with RTBC.

RTBC :)

Re: #36 yes, good point. ;)

Note: the d.o testbot is all sorts of broken right now. It will be many hours before this is tested.

Weird, I would have thought this applied cleanly to both branches. #37 is for 8.7.x. Here's a re-roll for 8.6.x.

Queued for re-testing.

Bot continues to be confused. :/ Let's try some fresh patches for 8.7.x and 8.6.x.

RTBC again :)

#46 is about a random fail, unrelated to this patch:

There was 1 failure:

1) Drupal\Tests\media\FunctionalJavascript\MediaStandardProfileTest::testMediaSources
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'example_1.jpeg'
+'f4qi3bDv.txt'

/var/www/html/core/modules/media/tests/src/FunctionalJavascript/MediaStandardProfileTest.php:220
/var/www/html/core/modules/media/tests/src/FunctionalJavascript/MediaStandardProfileTest.php:79

Also, there's mention of 6 CS fails, but none of them are touched by the patch:

/var/lib/drupalci/workspace/jenkins-drupal_patches-85683/source/core/themes/classy/css/components/form.css ✗ 6 more
line 35	Element (tr.odd) is overqualified, just use .odd without element name.
36	Element (tr.even) is overqualified, just use .even without element name.
44	Element (label.option) is overqualified, just use .option without element name.
53	margin can't be used with display: inline.
85	Element (abbr.tabledrag-changed) is overqualified, just use .tabledrag-changed without element name.
86	Element (abbr.ajax-changed) is overqualified, just use .ajax-changed without element name.

re-queued #43 for 8.7.x. Hopefully that's enough to reset the bot confusion. :/

Cheers,
-Derek

The reason this hasn't been committed is that I some concerns about the backwards compatibility of this change. My main concern is that a theme could have changed the font weight of the error message to normal by overriding the template, and removing the <strong> tag. They could have kept form-item--error-message class there, and now the text in the error would be bold again.

We could add a new class in the template so that this would only effect themes that haven't overridden the template. We could remove the existing class form-item--error-message since it doesn't follow BEM conventions, but we don't have a way to deprecate HTML classes. 🤷‍♂️

Another idea would be to create a new library that is attached in the default template so that if someone has overridden the template, the library wouldn't get attached and it wouldn't affect their theme.

Seems that adding a new class is the most straightforward way.

Could be...

.form-item-errors .messages--error 

I am at Drupalcon Seattle and looking into this issue.

Changing the branch to the latest version.

For now attaching reroll patch for 8.8.x, as there is still some discussion going on for the better way of handling this.

Triggering bots

#50 option 3 seems the safest method for a good balance between the legacy and change

Are we waiting for someone to implement a new library? If so, I can do it

Same issue is there in drupal 9.1
element there at core/themes/classy/templates/form/form-element.html.twig template
Can we have a patch for 9.1?

@tanubansal: Once again, the latest patch applies cleanly to 9.1.x branch. Please always try that yourself before asking others to do the work for you.

Thanks!
-Derek

Thanks @dww. Its working fine.
RTBC+1
But no where its mentioned that patch will work on 9.1 as well. So, i thought it might not work on that version. Thanks for the clarification.

Great help

I’m using this as an issue to demonstrate #3161760: Provide automated testing environments for Drupal Core issues (using Tugboat) and #3167029: Opt-in core issues into the Drupal.org Issue Forks and Merge Requests Beta. The merge request is the patch from #54, successfully applied to 9.1.x.

Cool, thanks!

#54: https://www.drupal.org/pift-ci-job/1808390 shows "6 coding standards messages"
vs.
#64: https://www.drupal.org/pift-ci-job/1808327 shows "3945 coding standards messages"

So something seems off about the CS checking on the MR testbots...

Cheers,
-Derek

Added fix for failing tests by updating hash of the affected files. Also updated css and twig templated of classy directory files in other themes.

The patch applies cleanly and the <strong> tags have been removed.

But I'm setting back to "Needs work" as lauriii's concerns in comment #50 have not been addressed.

Either a class needs to be added to the form-item--error-message elements, that is used to apply the increased font weight. Or a new library that is attached in the template needs to be created and used (preferred).

Hi, Updating the patch as considering the #50 so i have created a new class as mentioned, added reroll file.

+++ b/core/profiles/demo_umami/themes/umami/templates/classy/form/datetime-wrapper.html.twig
@@ -25,8 +25,8 @@
 {{ content }}
 {% if errors %}
-  <div class="form-item--error-message">
-    <strong>{{ errors }}</strong>
+  <div class="form-item--error-message form-item-errors">
+    {{ errors }}
   </div>

This is still valid for Umami in core, which has its own copy of the classy template, so moving there.

Made changes as suggested by #82.
Please review.

Re-titling.

Patch #83 applied successfully on Drupal 10.1.x-dev and PHP 8.1.6.
The patch work properly for me.
Thanks.

Addressed #50, also there are more files have <strong> tag with them. I have added all the remaining files, Also I have added the required css to make the error message bold. Please review

Checked and applied patch it is working for 10.1.x

The failing test on this is: in Nightwatch tests:

01:03:25 - Loading url: http://php-apache-jenkins-drupal-patches-161302/subdirectory/admin/people/permissions
01:03:25 
01:03:25   ℹ Loaded url http://php-apache-jenkins-drupal-patches-161302/subdirectory/admin/people/permissions in 374ms
01:03:25 (node:13866) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [CommandLoader]. Use emitter.setMaxListeners() to increase limit
01:03:26 - Loading url: http://php-apache-jenkins-drupal-patches-161302/subdirectory/admin/people/permissions
01:03:26 
01:03:26   ℹ Loaded url http://php-apache-jenkins-drupal-patches-161302/subdirectory/admin/people/permissions in 143ms
01:03:26   Error
01:03:26    unhandledRejection: stale element reference: element is not attached to the page document
01:03:26   (Session info: headless chrome=106.0.5249.103)
01:03:26   (Driver info: chromedriver=106.0.5249.61 (511755355844955cd3e264779baf0dd38212a4d0-refs/branch-heads/5249@{#569}),platform=Linux 4.9.0-0.bpo.6-amd64 x86_64)
01:03:26 StaleElementReferenceError: stale element reference: element is not attached to the page document
01:03:26   (Session info: headless chrome=106.0.5249.103)
01:03:26   (Driver info: chromedriver=106.0.5249.61 (511755355844955cd3e264779baf0dd38212a4d0-refs/branch-heads/5249@{#569}),platform=Linux 4.9.0-0.bpo.6-amd64 x86_64)
01:03:26     at Object.checkLegacyResponse (/var/www/html/core/node_modules/selenium-webdriver/lib/error.js:558:15)
01:03:26     at parseHttpResponse (/var/www/html/core/node_modules/selenium-webdriver/lib/http.js:581:13)
01:03:26     at Executor.execute (/var/www/html/core/node_modules/selenium-webdriver/lib/http.js:514:28)
01:03:26     at runMicrotasks (<anonymous>)
01:03:26     at processTicksAndRejections (node:internal/process/task_queues:96:5)
01:03:26     at async Object.execute (/var/www/html/core/node_modules/selenium-webdriver/lib/webdriver.js:740:17)
01:03:26 
01:03:26 ───────────────────────────────────────────────────────────────────────────────────────────────────
01:03:26 
01:03:26   ️TEST FAILURE (42.436s): 
01:03:26    - 1 error during execution; 
01:03:26    - 0 assertions failed; 68 passed
01:03:26 
01:03:26 

I requeued it to see if it is reproducible.

Tests are green again.

This looks good to me, even though it's all in Umami, only comitting to 10.1.x just in case someone somewhere is relying on the <strong> tags.

I didn't see a commit hash here, so I tracked it down.

b52100c11ee6ec96548d73ab32b795473e297700

And update and publish the change record