Problem/Motivation

When using template_preprocess_status_messages() to add classes to the class attribute of the wrapper element of status messages, if using the Classy theme, or a subtheme of Classy that does not implement its own instance of status-messages.html.twig, the classes appear as content below the messages element (not just as an attribute value in the markup).

Proposed resolution

Patch file attached.

Remaining tasks

Needs community review.

User interface changes

The values of the class attribute will no longer appear as page content beneath the status messages element.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#8 interdiff.txt1.83 KBdanmuzyka
#8 classy_theme-status_messages_classes_print_on_page-2752413-8-TESTS_ONLY.patch2.05 KBdanmuzyka
#3 interdiff.txt1.58 KBdanmuzyka
#3 classy_theme-status_messages_classes_print_on_page-2752413-3.patch3.81 KBdanmuzyka
classy_theme-status_messages_classes_print_on_page.patch2.52 KBdanmuzyka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

danmuzyka created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, classy_theme-status_messages_classes_print_on_page.patch, failed testing.

danmuzyka’s picture

danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review
FileSize
classy_theme-status_messages_classes_print_on_page-2752413-3.patch3.81 KB
interdiff.txt1.58 KB

Re-roll of patch.

ccjjmartin’s picture

ccjjmartin CreditAttribution: ccjjmartin as a volunteer commented
Status: Needs review » Needs work

@danmuzyka If Classy doesn't define templates it inherits them from the stable theme. In this case, the stable theme does have a status-messages.html.twig file defined. See this file on GitHub:

https://github.com/drupal/drupal/blob/8.2.x/core/themes/stable/templates...

If there is a problem with the classes it should be fixed in the file above (the Stable theme) and it will be fixed in Classy (and other subthemes) without having to add the template file. However, Bartik has a status-messages.html.twig file defined which overwrites the default Stable template. So changes will need to be made there too:

https://github.com/drupal/drupal/blob/8.2.x/core/themes/bartik/templates...

danmuzyka’s picture

danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review

@ccjjmartin,

This template file is already in the Classy theme:
http://cgit.drupalcode.org/drupal/tree/core/themes/classy/templates/misc...
https://github.com/drupal/drupal/blob/8.2.x/core/themes/classy/templates...

There is an error of logic in this line of that file:

{{ attributes.removeClass(classes) }}

The patch file I provided adds a kernel test for this issue, and replaces that line with the following:

{% set attributes = attributes.removeClass(classes) %}

There is nothing in this patch file that introduces a new twig template. The only thing it introduces is a new automated test.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented

@ccjjmartin Bartik calls the parent which comes from Classy, the Classy template overrides the Stable template. Only the Classy template set's attribute classes in this way, so no other template needs changes.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented

@danmuzyka can you attach just the test changes as a test-only patch to demonstrate the issue/fix?

+++ b/core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php
@@ -110,7 +110,7 @@ public static function cases(ContainerInterface $container = NULL, AccountInterf
-          'data' => "\n" . '    <div role="contentinfo" aria-label="Status message" class="messages messages--status">' . "\n" . '                  <h2 class="visually-hidden">Status message</h2>' . "\n" . '                    Hello from BigPipe!' . "\n" . '            </div>' . "\n    \n",
+          'data' => "\n" . '    <div role="contentinfo" aria-label="Status message" class="messages messages--status">' . "\n" . '                  <h2 class="visually-hidden">Status message</h2>' . "\n" . '                    Hello from BigPipe!' . "\n" . '            </div>' . "\n    ",

Are these changes intentional as part of this patch?

danmuzyka’s picture

danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commented
FileSize
classy_theme-status_messages_classes_print_on_page-2752413-8-TESTS_ONLY.patch2.05 KB
interdiff.txt1.83 KB

@jhedstrom, yes, the changes in core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php are intentional. The change in markup in the twig file causes one less linefeed/newline character to appear in the markup string, so I had to change the expected markup string in this test accordingly.

I've attached a tests-only version of the patch.

Status: Needs review » Needs work

The last submitted patch, 8: classy_theme-status_messages_classes_print_on_page-2752413-8-TESTS_ONLY.patch, failed testing.

danmuzyka’s picture

danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review

The TESTS_ONLY patch failed as expected.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/system/tests/src/Kernel/Render/ClassyTest.php
@@ -0,0 +1,49 @@
+    drupal_set_message('But then something nice happened');

I love it when nice things happen!

This looks good to go I think.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Assigned: Unassigned » star-szr

Thanks for working on this! Assigning frontend RTBC issues to Cottser for possible review.

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/themes/classy/templates/misc/status-messages.html.twig
@@ -51,6 +51,6 @@
-  {{ attributes.removeClass(classes) }}
+  {% set attributes = attributes.removeClass(classes) %}

This change confuses me because attribute.addClass is adding to the storage but this is not removing... I can't see why this is necessary... removeClass returns $this. Very weird.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Lol I see... we can't just call attributes.removeClass() inside a {% %} and {{ }} is about outputting this into the template.

alexpott’s picture

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

I'm torn about whether this should go in 8.1.x - it is removing duplicate new lines from the output and therefore the output is changing - but at least one new line is still there, So it probably okay to do this - especially as this is in service of fixing a quite annoying bug.

alexpott’s picture

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

Leaving at rtbc for @Cottser to review

  • Cottser committed 790c021 on 8.2.x 
    Issue #2752413 by danmuzyka, alexpott, jhedstrom: Custom CSS classes...

  • Cottser committed 1014a70 on 8.1.x 
    Issue #2752413 by danmuzyka, alexpott, jhedstrom: Custom CSS classes...
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Version: 8.2.x-dev » 8.1.x-dev
Assigned: star-szr » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: +Twig

I concur with @alexpott in #15 fixing this annoying (and kinda embarrassing) bug IMO outweighs any possible minor disruptions.

#3 has been committed and pushed 790c021 to 8.2.x and 1014a70 to 8.1.x. Thanks!

  • Cottser committed 790c021 on 8.3.x 
    Issue #2752413 by danmuzyka, alexpott, jhedstrom: Custom CSS classes...

  • Cottser committed 790c021 on 8.3.x 
    Issue #2752413 by danmuzyka, alexpott, jhedstrom: Custom CSS classes...

Status: Fixed » Closed (fixed)

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