Olivero: Messages can be malformed when JS creates messages and PHP messages already exist

Context:

Patch attached to fix the issue. The root of this issue is that the core Drupal.Message.add() was anticipating one more level of wrapping divs than Olivero was providing, so wrapping the iterator with another div fixed the issue (compare to Bartick's status-messages.html.twig).

In addition to the additional markup, I've added core/drupal.message as a dependency of Olivero's global styles library. In our .info file we're usinglibraries-override to customize Drupal.theme.message(), but that function in our code is relying on the existence of the class Drupal.Message. As such, instead of calling our own library (olivero/messages), we should call core's library and then allow the override to replace what it needs to.

Testing Instructions:

1. Apply patch to latest 9.3.x
2. Add status/warning/error messages via PHP. In olivero.theme's preprocess_html function:
   
   

   $messenger = Drupal::messenger();
   $messenger->addStatus('PHP Status message');
   $messenger->addWarning('PHP Warning message');
   $messenger->addError('PHP Error message');

3. Confirm messages display & close properly
4. Refresh page and add messages via javascript in the console:
   
   

   const messenger = new Drupal.Message;
   messenger.add('js message', {type: 'status'});
   messenger.add('js message', {type: 'error'});

5. Confirm all messages are displayed & close properly

I agree, it definitely feels heavy-handed. To avoid declaring that as a dependency, we'll probably need to refactor olivero's messages.es6.js file to avoid using the Drupal.Message class.

The solution we discussed on our call was to add {{ attach_library('core/drupal.message') }} in status-messages.html.twig, but this will lead to having the ability to add messages via JS when PHP adds messages, and not if there are no server-side messages.

I don't think it's bigger than olivero, I think it's just that olivero is bundling functionality overriding core/drupal.message with custom additional functionality to dismiss system messages.

We might just want to split that file into messages.es6.js and drupal-messages-override.es6.js

New patch with a different approach - moving Drupal.theme.message to its own javascript file & library to extend core's drupal.message library. (Patch is not incremental with #3, so no interdiff)

Testing Instructions:

1. Apply patch to latest 9.3.x
2. Add status/warning/error messages via PHP. In olivero.theme's preprocess_html function:
   
   

   $messenger = Drupal::messenger();
   $messenger->addStatus('PHP Status message');
   $messenger->addWarning('PHP Warning message');
   $messenger->addError('PHP Error message');

3. Confirm messages created server-side display & close properly
4. Confirm Drupal.Message and Drupal.theme.message are *NOT* available via the console
5. Add core/drupal.message to olivero's list of libraries in the info file & dump cache
6. Refresh page and add messages via javascript in the console:
   
   

   const messenger = new Drupal.Message;
   messenger.add('js message', {type: 'status'});
   messenger.add('js message', {type: 'error'});

7. Confirm both php- & js-created messages are displayed & close properly

Not sure why that wasn't included. New patch attached!

Ah - olivero/messages needs to be there for olivero/drupal.message to work correctly. Added to the patch.

Before splitting the js into two files we were doing two different - and mostly unrelated - things.

On the one hand we're creating closeMessage() and putting it into the "messages" behavior. This function creates a new DOM element (the close button) & event handler (the close button's action) for each message created by PHP.

On the other hand, we're extending core's drupal.messages library, replacing the core Drupal.theme.message() function with our own. This function isn't actually ever used by Olivero, however. This function is used by the Drupal.Message class when a new message is created via javascript, and overrides the core version so new messages match the markup/classes olivero needs to display the messages properly. So we don't need this function unless we're loading up Drupal.Message from core/drupal.message

That being said, our version of Drupal.theme.message() needs to be able to pull in the closeMessage() functionality from the other javascript file to wire up the click event without duplicating code, hence the dependency declaration.

With this setup, then if we only have PHP messages:

• olivero/messages is loaded from status-message.html.twig
• Drupal.Message class and Drupal.theme.message() are not sent to the client

and if we need JS message capability:

• core/drupal.message is extended by olivero/drupal.message
• olivero/drupal.message loads its dependency olivero/messages
• Drupal.Message.add() calls Drupal.theme.message() from olivero
• Drupal.theme.message() calls Drupal.behaviors.messages.closeMessage() from olivero/messages so we keep the close button/functionality in one place.

Is it possible to add tests for this?

Moving to needs work since we need tests.

My first attempt at writing tests!

Patch is a reroll of #12 and adds testing, interdiff only shows added nightwatch testing.

New patch fixes formatting of #19.

Patch #20 needs a lot of work. The original work in #12 was done before all JS selectors went to the data-drupal-selector format.

Reworked #20 to use the refactored JS & twig, let's see if these tests pass 🤞

Attached wrong file.

New patch with updated @file statement & a second with only tests

Thanks for the update. Patch not applying so needs work.

Added testing steps from #3 to the issue summary.

Updating testing instructions in issue summary

Tagging for testing.

Updated the issue summary to use the template.

Tested manually with the steps in issue summary.

Few comments on the MR and the D10 patch needs reroll.

I've reviewed the most recent changes in the MR after the reroll:

1. Drupal.behaviors.messages.closeMessage => Drupal.olivero.closeMessage

2. message-override.js => message.theme.js

and these address the feedback in #39.

Tagging for manual testing. I'll try to do that now.

Confirmed that D9 patch applies cleanly to 9.4 and 9.5 and D10 patch applies cleanly to 10.0.

Should this be tested on 9.4, 9.5, and/or 10?

@andy-blum Suggested testing on 9.4 and 10 via Slack.

I tested on 9.4 using the testing instructions from the issue summary and all messages render okay and can be closed.

Not sure if the undefined after const messenger = new Drupal.Message; in the console matters?

Updated the testing instructions to be more clear.

Successfully tested on 9.5 but still need test on 10.0.

Tested on 10.0.x and it works fine as well.

The only thing I'm not sure of is if we need to show that a test-only patch fails before this fix, but I'm marking RTBC based on the following.

1. Code was reviewed previously.

2. Feedback was addressed from review.

3. Successfully tested on 9.4, 9.5, and 10.

4. Issue summary is clear and includes steps to reproduce.

5. Patch only addresses the issue noted in the issue summary.

6. Tests have been added.

Committed fd92a54 and pushed to 10.0.x. Also committed to 9.5.x and cherry-picked to 9.4.x. Thanks!

Assigning credit from the previous issue.