Add a generic Ajax Message command

This blocks #3081526: Add announcement after clicking 'Save and select' in the media library. and #3059955: It is possible to overflow the number of items allowed in Media Library, both of which are major bugs.

Why not make it more generic?

`Drupal.Message` is Class. You can add another parameter to call the method rather than specific `add` only

e.g.:

message(ajax, response) {
  const {method, args} = response
  Drupal.Message[method].apply(args); // OR Drupal.Message[method](...args);
}

+++ b/core/lib/Drupal/Core/Ajax/MessageCommand.php
@@ -0,0 +1,85 @@
+   * @param string|null $messageWrapperQuerySelector
+   *   The query selector of the element to display messages in when they
+   *   should be displayed somewhere other than the default
+   *   [data-drupal-messages].
...
+    $this->clearPrevious = $clearPrevious;

+++ b/core/misc/ajax.es6.js
@@ -1562,5 +1566,29 @@
+      if (response.clearPrevious) {
+        messages.clear();
+      }

+++ b/core/misc/message.es6.js
@@ -162,6 +162,9 @@
     clear() {
+      if (!this.messageWrapper) {
+        this.messageWrapper = Drupal.Message.defaultWrapper();
+      }

I think these aspects need to be reviewed & approved by the people who led #77245: Provide a common API for displaying JavaScript messages.

EDIT: Pinged @nod_ and @tedbow.

1. +++ b/core/lib/Drupal/Core/Ajax/MessageCommand.php
   @@ -0,0 +1,85 @@
   +   * @var boolean
   +   */
   +  protected $clearPrevious;
   

   I think we are suppose to use @var bool instead

2. +++ b/core/lib/Drupal/Core/Ajax/MessageCommand.php
   @@ -0,0 +1,85 @@
   +   *   The query selector of the element to display messages in when they
   +   *   should be displayed somewhere other than the default
   +   *   [data-drupal-messages].
   

   Could we remove data-drupal-messages and put a @see or mention of the JS method Drupal.Message.defaultWrapper(). Because it is little more complicated.

3. +++ b/core/misc/ajax.es6.js
   @@ -655,7 +655,9 @@
   -        `An error occurred while attempting to process ${this.options.url}: ${e.message}`,
   +        `An error occurred while attempting to process ${this.options.url}: ${
   +          e.message
   +        }`,
   
   @@ -748,7 +750,9 @@
   -        `An error occurred while attempting to process ${ajax.options.url}: ${e.message}`,
   +        `An error occurred while attempting to process ${ajax.options.url}: ${
   +          e.message
   +        }`,
   

   These are unrelated changes. They removed if I run yarn prettier. So this should be run on this patch. https://www.drupal.org/node/2986680

4. +++ b/core/misc/ajax.es6.js
   @@ -1562,5 +1566,29 @@
   +        document.querySelector(response.messageWrapperQuerySelector),
   

   response.messageWrapperQuerySelector needs to be documented in the doc block.

5. +++ b/core/misc/message.es6.js
   @@ -162,6 +162,9 @@
   +      if (!this.messageWrapper) {
   +        this.messageWrapper = Drupal.Message.defaultWrapper();
   +      }
   

   I think this is fixed in #3052526: Drupal.Message.clear throws exception if messageWrapper not initialized if we can get that in first

6. +++ b/core/modules/system/tests/modules/ajax_test/src/Form/AjaxTestMessageForm.php
   @@ -0,0 +1,113 @@
   +class AjaxTestMessageForm extends FormBase {
   

   Nothing from FormBase is actually being used if we change this to implements FormInterface

   the test still passes

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MessageTest.php
   @@ -0,0 +1,62 @@
   +    $assert_session->waitForElementVisible('css', '#alternate-message-container .messages--status:contains("I am a message in an alternate location.")');
   +    $assert_session->pageTextNotContains('I am a message in the default location.');
   

   waitForElementVisible() doesn't actually fail if the element never appears it just returns NULL.

   So these lines pass even if you update \Drupal\ajax_test\Form\AjaxTestMessageForm::makeMessageAlternate() to not pass the optional arg $messageWrapperQuerySelector to the command.

   If the first line is just wrapped in $this->assertNotEmpty() then the pageTextNotContains() is not needed.

   there are 2 other cases like this in the test.

8. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MessageTest.php
   @@ -0,0 +1,62 @@
   +    for ($i = 0; $i < 6; $i++) {
   

   Super nit. If the for loop started with $i = 1 we don't have to test for $i+1. Would also have to change to $i <= 6

9. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MessageTest.php
   @@ -0,0 +1,62 @@
   +      $assert_session->assertWaitOnAjaxRequest();
   +      $assert_session->elementsCount('css', '#alternate-message-container .messages', $i+1);
   

   I think anytime we can avoid assertWaitOnAjaxRequest() we should. I think it is better to explicitly wait for the thing we are expecting.

   This would work

   $all_elements_found = $page->waitFor(10, function () use ($i, $page) {
     return count($page->findAll('css','#alternate-message-container .messages')) === $i+1;
   });
   $this->assertTrue($all_elements_found);
   

10. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/JsMessageTest.php
    @@ -6,7 +6,7 @@
    - * Tests core/drupal.messages library.
    + * Tests core/drupal.message library.
    

    This patch is not touching this file for anything so it feels unrelated. But good fix if we can get it in in this patch

Also I should have mentioned I think this is great new Ajax command!

#3052526: Drupal.Message.clear throws exception if messageWrapper not initialized is in, so this needs to be re-rolled too.

This is going to need a change record for sure.

@oknate thanks all the changes look good.

Sorry I didn't catch 1 other thing before and 1 could have

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MessageTest.php
   @@ -0,0 +1,96 @@
   +    // Test that if MessageCommand::clearPrevious is FALSE, messages will not be cleared.
   

   This comment should wrap at 80 characters

+++ b/core/modules/system/tests/modules/ajax_test/src/Form/AjaxTestMessageForm.php
@@ -0,0 +1,113 @@
+    $response->addCommand(new MessageCommand('I am a message in the default location.'));
+
+    return $response;

super nit and probably doesn't need fixing. But if you agree and are fixing the other point

You can just return the result of addCommand() as this returns the response itself

@oknate thanks. Looks good!

Setting to RTBC! 🎉

Test fail will bump but I think it will be good!

Sorry forgot that we still need the change record. but the patch looks good

Crediting @bnjmnm per #9.

Change record looks good and detailed! Restoring RTBC as per #24, and removing the "Needs subsystem maintainer review" tag.

Thanks, @oknate and @tedbow! Let's land this.

Turns out @tedbow is not, according to MAINTAINERS.txt, a subsystem maintainer of the AJAX system. Whoops.

So...I'm restoring the "needs subsystem maintainer review" tag, but keeping RTBC. And I'm assigning this to @effulgentsia who, as a subsystem maintainer and framework manager, is probably in the best position to review this patch.

Also crediting @tedbow for reviews.

I added Needs subsystem maintainer review specifically for the "message" aspect. I said that in #14, but should've been more explicit probably. I even quoted specifically those parts that I thought needed their sign-off.

The patch does not change the AJAX system. I'm a pseudo maintainer of the AJAX system too (having worked on the "AJAX page state" stuff a lot, as well as BigPipe, which uses the AJAX system too). So I feel comfortable reviewing this new AJAX command too.

1. +++ b/core/lib/Drupal/Core/Ajax/MessageCommand.php
   @@ -0,0 +1,85 @@
   +   * @param string|null $messageWrapperQuerySelector
   ...
   +   * @param array $messageOptions
   ...
   +   * @param bool $clearPrevious
   ...
   +  public function __construct($message, $messageWrapperQuerySelector = NULL, array $messageOptions = [], $clearPrevious = TRUE) {
   

   🐪 These should not use camelCase. Only class properties use camelCase.

2. +++ b/core/lib/Drupal/Core/Ajax/MessageCommand.php
   @@ -0,0 +1,85 @@
   +  public function getAttachedAssets() {
   +    $assets = new AttachedAssets();
   +    $assets->setLibraries(['core/drupal.message']);
   +    return $assets;
   +  }
   

   👍 AnnounceCommand uses the same pattern.

3. +++ b/core/misc/ajax.es6.js
   @@ -1562,5 +1562,31 @@
   +     * Command to use Drupal.Message() via AJAX command.
   

   👎 Command to … via AJAX command. I think we can do better. What about Command to add a message to the message area.

4. +++ b/core/misc/ajax.es6.js
   @@ -1562,5 +1562,31 @@
   +     *   The zone where to add the message. If null a default will be used.
   

   🤓 a default →

   the default
   

5. +++ b/core/misc/ajax.es6.js
   @@ -1562,5 +1562,31 @@
   +     *   If TRUE, clear previous messages.
   

   🤓 TRUE → true (we only capitalize it in PHP, not in JS)

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/MessageTest.php
   @@ -0,0 +1,97 @@
   +    }
   +
   +  }
   

   🤓 Extraneous blank line.

+++ b/core/misc/ajax.es6.js
@@ -1562,5 +1562,31 @@
+    message(ajax, response) {
+      const messages = new Drupal.Message(
+        document.querySelector(response.messageWrapperQuerySelector),
+      );
+      if (response.clearPrevious) {
+        messages.clear();
+      }
+      messages.add(response.message, response.messageOptions);
+    },

So, I see a problem here. The drupal.ajax library does not have a dependency on drupal.message, which means that this might result in a "Drupal.Message is undefined" error unless some other code has coincidentally added the drupal.message library to the page.

I imagine we do NOT want to have drupal.ajax depend on drupal.message, so I would suggest that we modify messages.es6.js so that, if Drupal.AjaxCommands is defined, it adds the `message` callback to Drupal.AjaxCommands.prototype. Something like:

// in messages.es6.js

if (Drupal.AjaxCommands) {
  Drupal.AjaxCommands.prototype.message = (ajax, response) {
    // ...do the magic!
  }
}

re #36

The current pattern in the patch is the same one we used for #3020352: Create a new AnnounceCommand to call Drupal.announce on an AjaxResponse
Drupal.AjaxCommands.announce() should only ever be called by invoking the \Drupal\Core\Ajax\AnnounceCommand on the server and \Drupal\Core\Ajax\AnnounceCommand::getAttachedAssets() ensures that the core/drupal.announce library is attached.

drupal.ajax does not depend on drupal.announce but we can be assured the library will be attached if Drupal.AjaxCommands.announce() is called.

This is the same with drupal.ajax not depending on drupal. message. We can be assured that if Drupal.AjaxCommands.message() is called the drupal. message library will be loaded because \Drupal\Core\Ajax\MessageCommand::getAttachedAssets() includes it.

All of the commands including the announce and message commands on Drupal.AjaxCommands require

* @param {Drupal.Ajax} [ajax]
     *   {@link Drupal.Ajax} object created by {@link Drupal.ajax}.
     * @param {object} response
     *   The response from the Ajax request.

A developer would have to jump through a ton of hoops and really be making some bad decisions to actually construct these arguments from scratch outside of the server-side ajax command system that Drupal provides and call any of these JS command functions directly.
So I think it is safe to assume that any libraries the server-side command classes provide will be attached.

This is the same with drupal.ajax not depending on drupal. message. We can be assured that if Drupal.AjaxCommands.message() is called the drupal. message library will be loaded because \Drupal\Core\Ajax\MessageCommand::getAttachedAssets() includes it.

Ah, okay. This is the thing I missed. Carry on!

Just seeing this for the first time. Spotted it referenced in #3081526: Add announcement after clicking 'Save and select' in the media library..

What happens if an ajax response includes both a MessageCommand and an AnnounceCommand. They will both result in calls to Drupal.announce(), right?

In the old issue that introduced Drupal.message, I pointed out that the use of Drupal.announce is less than ideal, because:

• Anything can call it at any time. It's a free-for-all.
• Priorities get mixed up.
• Depending on timing, messages may be concatenated, or overwrite each other.
• Concatenating lots of messages at once can be unreliable, because any keypress from the user may cancel all of the announcements. Users can carry out actions quicker than it takes to speak the announcements. There's a risk users can miss important information because it comes late in a long announcement.

The possibility for an ajax responses to have a MessageCommand and an AnnounceCommand could get flaky and confusing if these aren't used carefully.

Drupal.announce() should be used sparingly, I think. However I've noticed a trend over the last year or so towards treating it as the go-to solution for all your screen reader needs, and dumping arbitrary messages. Live regions shouldn't be treated like console output; that's not how they work at all (well, role="log" is intended to, but I don't think it's well supported yet).

In the longer term, I think we should move Drupal.message off of Drupal.announce, so it treats messages as live regions in their own right. However we may need to wait for browser and assistive tech support to improve first. In theory, the benefit is that the assistive tech can handle a queue of multiple ARIA live regions, possibly based on user preferences.

In the shorter term, it would be good to provide some kind of guidance about the difference between AnnounceCommand and MessageCommand. In many cases you will want one or the other; seldom both. I don't fully know what that guidance should look like yet, but at minimum it would be along the lines of:

"Developers should take care when using MessageCommand and AnnounceCommand together in the same AJAX response. Manual testing with screen readers is recommended."

Can we add something like that to change record, and class docs?

Wow, I'm so glad you spotted that @andrewmacpherson! 😳 I should've thought of that too. Agreed with your recommendation!

I would suggest this text for the class docs:

Developers should be extra careful if this command and \Drupal\Core\Ajax\AnnounceCommand are included in the same response. Unless the `announce` option is set to an empty string (''), this command will result in the message being announced to screen readers. When
combined with AnnounceCommand, this may result in unexpected behavior. Manual testing with a screen reader is strongly recommended.

Here are examples of how to suppress announcements:
@code
$command = new MessageCommand("I won't be announced", NULL, [
  'announce' => '',
]);
@endcode

@see \Drupal\Core\Ajax\AnnounceCommand

Also updating for explicit test coverage of this allowance.

@andrewmacpherson thanks for bring this up.

@oknate I think the changes look good

Back to RTBC!

The latest patch adds advice to the MessageCommand class docs.

Shouldn't we also add corresponding advice to the AnnounceCommand class too?

Committed 7dde35c and pushed to 8.8.x. Thanks!

Published the change record

Yay, this unblocked two issues!

1. #3081526-23: Add announcement after clicking 'Save and select' in the media library. (RTBC!)
2. #3059955-44: It is possible to overflow the number of items allowed in Media Library