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.

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

Re-roll of patch.

ccjjmartin’s picture

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

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

@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

@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

@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
danmuzyka’s picture

Status: Needs work » Needs review

The TESTS_ONLY patch failed as expected.

jhedstrom’s picture

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

Assigned: Unassigned » Cottser

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

alexpott’s picture

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

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

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

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...
Cottser’s picture

Version: 8.2.x-dev » 8.1.x-dev
Assigned: Cottser » 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.