After upgrading to Drupal 8.3.1 I've noticed drupal messages displayed by AJAX calls completely lose styling - the messages.css file is not loaded dynamically, tested on Seven and Bartik themes with the same effect.

Reproduce:

  1. add drupal_set_message('some message') in any AJAX form callback
  2. trigger AJAX on a page where no messages were displayed previously (messages.css wasn't already loaded)

May be related to:
https://www.drupal.org/node/1988968

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Graber created an issue. See original summary.

Graber’s picture

Issue summary: View changes
cilefen’s picture

A `git bisect` is probably needed if we are not sure.

droplet’s picture

drupal_set_message set a flash message and displayed after a page reload, right? the CSS doesn't load from AJAX.

No errors on my testing (8.4.x)

EDIT:
and Bartik should always load the messages.css. @see bartik.libraries.yml

droplet’s picture

alexpott’s picture

Status: Active » Needs review
FileSize
593 bytes

Here's a fix. Basically prior to #2853509: Don't render status messages if there are no messages but also include their assets if there might be all classy themes were guaranteed to have classy's messages.css loaded - we can easily do just that.

alexpott’s picture

In fact here's a better fix that preserves the inheritance of libraries so overrides etc work as expected.

cilefen’s picture

Status: Needs review » Needs work

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.57 KB
4.84 KB
4.89 KB

@xjm pointed out that we'd need a change clear to pick up the libraries change. Patch attached also fixes the tests. Big Pipe's life becomes a bit simpler because it no longer needs to add the messages library for status messages.

alexpott’s picture

Here's specific tests to ensure that classy's messages.css is loaded for both classy and themes that extend classy like Bartik and Seven.

The last submitted patch, 11: 2875947-11.test-only.patch, failed testing.

alayham’s picture

this might be completely related, or completely unrelated, depending on the approach:
#2876314: Allow a view to load style libraries

Graber’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the fast action!

The #10 fixed the problem, to be precise the single line added in classy.info.yml:

libraries:
- classy/base
+ - classy/messages
- core/normalize

I'll update the issue status to "fixed" when this will be released.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/system.install
@@ -1794,6 +1794,12 @@ function system_update_8301() {
 
 /**
+ * Clear caches to ensure Classy's message library is always added.
+ */
+function system_update_8302() {
+}
+
+/**

Let's add an empty post update rather than an empty hook_update_N() to minimize numbering conflicts and similar with other patches.

Also: shouldn't CSS added via AJAX callbacks be added to the page? We have a whole set of functionality around that in the AJAX system (ajax page state etc.) so I'm surprised it's not just working to be honest.

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.84 KB
1.43 KB

Nice idea @catch - now one patch for 8.3.x and 8.4.x. Also @Graber emailed me and mentioned that the attach in classy's status-messages.html.twig is no longer necessary. Which is true.

@catch I don;t think the Ajax page state thing is working because the JS libraries for tabledrag don't depend on the message library - this is because the message library is only a thing in Classy and not core.

Status: Needs review » Needs work

The last submitted patch, 16: 2875947-16.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.28 KB
8.31 KB

One less new line in D8 output...

catch’s picture

Can't classy add a hook_libraries_info_alter() to add itself as a dependency?

alexpott’s picture

@catch but we can't know all the libraries in contrib and custom that would need this. It was added to every page.

nod_’s picture

xjm’s picture

Status: Needs review » Closed (duplicate)

Rather than having a regression, a critical, a major, a BC break, and disruption to ongoing work, I've reverted the original issue and so closing this as duplicate. If someone can recapture the work here over there I'd appreciate it. Thanks!

Graber’s picture

Status: Closed (duplicate) » Needs review

@xjm if this is a duplicate, please provide the link to the original issue.

alexpott’s picture

Graber’s picture

Great, thanks!