Issue #3364713 by lauriii, bnjmnm, andy-blum, Dave Reid, cosmicdreams: Claro:...

Issue #3364713 by lauriii, bnjmnm, andy-blum, Dave Reid, cosmicdreams: Claro: Messages can be malformed when JS creates messages and PHP messages already exist

core/modules/system/tests/modules/js_message_test/js_message_test.routing.yml

@@ -5,3 +5,11 @@ js_message_test.links:
     _title: 'JsMessageLinks'
   requirements:
     _access: 'TRUE'
+
+js_message_test.links_with_system_messages:
+  path: '/js_message_test_link_with_system_messages'
+  defaults:
+    _controller: '\Drupal\js_message_test\Controller\JSMessageTestController::messageLinksWithSystemMessages'
+    _title: 'JsMessageLinks'
+  requirements:
+    _access: 'TRUE'

core/modules/system/tests/modules/js_message_test/src/Controller/JSMessageTestController.php

@@ -29,6 +29,22 @@ public static function getMessagesSelectors() {
     return ['', '[data-drupal-messages-other]'];
   }
 
+  /**
+   * Displays links to show messages via JavaScript with messages from backend.
+   *
+   * @return array
+   *   Render array for links.
+   */
+  public function messageLinksWithSystemMessages() {
+    // Add PHP rendered messages to the page.
+    $messenger = \Drupal::messenger();
+    $messenger->addStatus('PHP Status');
+    $messenger->addWarning('PHP Warning');
+    $messenger->addError('PHP Error');
+
+    return $this->messageLinks();
+  }
+
   /**
    * Displays links to show messages via JavaScript.
    *

core/modules/system/tests/modules/olivero_test/olivero_test.module

@@ -15,25 +15,6 @@ function olivero_test_preprocess_field_multiple_value_form(&$variables) {
   }
 }
 
-/**
- * Implements hook_preprocess().
- */
-function olivero_test_preprocess_page(&$variables) {
-  $route = \Drupal::routeMatch()->getRouteName();
-
-  switch ($route) {
-    case 'js_message_test.links':
-      $messenger = \Drupal::messenger();
-      $messenger->addStatus('PHP Status');
-      $messenger->addWarning('PHP Warning');
-      $messenger->addError('PHP Error');
-      break;
-
-    default:
-      break;
-  }
-}
-
 /**
  * Implements hook_preprocess_html().
  */

core/profiles/nightwatch_testing/nightwatch_testing.info.yml

@@ -5,3 +5,4 @@ version: VERSION
 hidden: true
 install:
   - js_testing_log_test
+  - nightwatch_theme_install_utility

core/tests/Drupal/Nightwatch/Tests/Claro/claroDrupalMessageTest.js

+const mainContent = '.region-content';
+const mainMessagesContainer =
+  '[data-drupal-messages] > .messages-list__wrapper';
+const secondaryMessagesContainer = '[data-drupal-messages-other]';
+
+const mainButtons = {
+  addStatus: '#add--status',
+  removeStatus: '#remove--status',
+  addError: '#add--error',
+  removeError: '#remove--error',
+  addWarning: '#add--warning',
+  removeWarning: '#remove--warning',
+  clearAll: '#clear-all',
+};
+
+const secondaryButtons = {
+  addStatus: '[id="add-[data-drupal-messages-other]-status"]',
+  removeStatus: '[id="remove-[data-drupal-messages-other]-status"]',
+  addError: '[id="add-[data-drupal-messages-other]-error"]',
+  removeError: '[id="remove-[data-drupal-messages-other]-error"]',
+  addWarning: '[id="add-[data-drupal-messages-other]-warning"]',
+  removeWarning: '[id="remove-[data-drupal-messages-other]-warning"]',
+};
+
+module.exports = {
+  '@tags': ['core', 'claro'],
+  before(browser) {
+    browser
+      .drupalInstall()
+      .drupalInstallModule('js_message_test')
+      .drupalEnableTheme('claro');
+  },
+  after(browser) {
+    browser.drupalUninstall();
+  },
+  'Verify default placement of javascript-created messages': (browser) => {
+    browser
+      .drupalRelativeURL('/js_message_test_link_with_system_messages')
+      .waitForElementVisible(mainContent)
+      .assert.elementPresent(mainMessagesContainer)
+
+      // We should load 3 messages on page load from \Drupal::messenger()
+      .assert.elementCount(`${mainMessagesContainer} > .messages-list__item`, 3)
+
+      // We should have one message of each type
+      .assert.elementCount(`${mainMessagesContainer} > .messages--status`, 1)
+      .assert.elementCount(`${mainMessagesContainer} > .messages--warning`, 1)
+      .assert.elementCount(`${mainMessagesContainer} > .messages--error`, 1)
+
+      // Trigger new messages via javascript
+      .click(mainButtons.addStatus)
+      .click(mainButtons.addWarning)
+      .click(mainButtons.addError)
+
+      // We should have 6 total messages
+      .assert.elementCount(`${mainMessagesContainer} > .messages-list__item`, 6)
+
+      // We should have 2 messages of each type
+      .assert.elementCount(`${mainMessagesContainer} > .messages--status`, 2)
+      .assert.elementCount(`${mainMessagesContainer} > .messages--warning`, 2)
+      .assert.elementCount(`${mainMessagesContainer} > .messages--error`, 2);
+  },
+
+  'Verify customized placement of javascript-created messages': (browser) => {
+    browser
+      .drupalRelativeURL('/js_message_test_link_with_system_messages')
+      .waitForElementVisible(mainContent)
+      .assert.elementPresent(secondaryMessagesContainer)
+
+      // We should load 3 messages on page load from \Drupal::messenger()
+      .assert.elementCount(
+        `${secondaryMessagesContainer} > .messages-list__item`,
+        0,
+      )
+
+      // Trigger new messages via javascript
+      .click(secondaryButtons.addStatus)
+      .click(secondaryButtons.addWarning)
+      .click(secondaryButtons.addError)
+
+      // We should have 6 total messages
+      .assert.elementCount(
+        `${secondaryMessagesContainer} > .messages-list__item`,
+        3,
+      )
+
+      // We should have 2 messages of each type
+      .assert.elementCount(
+        `${secondaryMessagesContainer} > .messages--status`,
+        1,
+      )
+      .assert.elementCount(
+        `${secondaryMessagesContainer} > .messages--warning`,
+        1,
+      )
+      .assert.elementCount(
+        `${secondaryMessagesContainer} > .messages--error`,
+        1,
+      );
+  },
+};

core/tests/Drupal/Nightwatch/Tests/Olivero/oliveroDrupalMessageTest.js

@@ -35,7 +35,7 @@ module.exports = {
   },
   'Verify default placement of javascript-created messages': (browser) => {
     browser
-      .drupalRelativeURL('/js_message_test_link')
+      .drupalRelativeURL('/js_message_test_link_with_system_messages')
       .waitForElementVisible(mainContent)
       .assert.elementPresent(mainMessagesContainer)
 
@@ -63,7 +63,7 @@ module.exports = {
 
   'Verify customized placement of javascript-created messages': (browser) => {
     browser
-      .drupalRelativeURL('/js_message_test_link')
+      .drupalRelativeURL('/js_message_test_link_with_system_messages')
       .waitForElementVisible(mainContent)
       .assert.elementPresent(secondaryMessagesContainer)
 

core/themes/claro/js/messages.js

@@ -25,7 +25,10 @@
     const messagesTypes = Drupal.Message.getMessageTypeLabels();
     const messageWrapper = document.createElement('div');
 
-    messageWrapper.setAttribute('class', `messages messages--${type}`);
+    messageWrapper.setAttribute(
+      'class',
+      `messages messages--${type} messages-list__item`,
+    );
     messageWrapper.setAttribute(
       'role',
       type === 'error' || type === 'warning' ? 'alert' : 'status',

core/themes/claro/templates/misc/status-messages.html.twig

@@ -23,50 +23,52 @@
  */
 #}
 <div data-drupal-messages class="messages-list">
-{% for type, messages in message_list %}
-  {%
-    set classes = [
-      'messages-list__item',
-      'messages',
-      'messages--' ~ type,
-    ]
-  %}
-  {%
-    set is_message_with_title = status_headings[type]
-  %}
-  {%
-    set is_message_with_icon = type in ['error', 'status', 'warning']
-  %}
+  <div class="messages-list__wrapper">
+    {% for type, messages in message_list %}
+      {%
+        set classes = [
+          'messages-list__item',
+          'messages',
+          'messages--' ~ type,
+        ]
+      %}
+      {%
+        set is_message_with_title = status_headings[type]
+      %}
+      {%
+        set is_message_with_icon = type in ['error', 'status', 'warning']
+      %}
 
-  <div role="contentinfo" aria-labelledby="{{ title_ids[type] }}"{{ attributes.addClass(classes)|without('role', 'aria-label') }}>
-    {% if type == 'error' %}
-      <div role="alert">
-    {% endif %}
-      {% if is_message_with_title or is_message_with_icon %}
-        <div class="messages__header">
-          {% if is_message_with_title %}
-            <h2 id="{{ title_ids[type] }}" class="messages__title">
-              {{ status_headings[type] }}
-            </h2>
+      <div role="contentinfo" aria-labelledby="{{ title_ids[type] }}"{{ attributes.addClass(classes)|without('role', 'aria-label') }}>
+        {% if type == 'error' %}
+          <div role="alert">
+        {% endif %}
+          {% if is_message_with_title or is_message_with_icon %}
+            <div class="messages__header">
+              {% if is_message_with_title %}
+                <h2 id="{{ title_ids[type] }}" class="messages__title">
+                  {{ status_headings[type] }}
+                </h2>
+              {% endif %}
+            </div>
           {% endif %}
-        </div>
-      {% endif %}
-      <div class="messages__content">
-        {% if messages|length > 1 %}
-          <ul class="messages__list">
-            {% for message in messages %}
-              <li class="messages__item">{{ message }}</li>
-            {% endfor %}
-          </ul>
-        {% else %}
-          {{ messages|first }}
+          <div class="messages__content">
+            {% if messages|length > 1 %}
+              <ul class="messages__list">
+                {% for message in messages %}
+                  <li class="messages__item">{{ message }}</li>
+                {% endfor %}
+              </ul>
+            {% else %}
+              {{ messages|first }}
+            {% endif %}
+          </div>
+        {% if type == 'error' %}
+          </div>
         {% endif %}
       </div>
-    {% if type == 'error' %}
-      </div>
-    {% endif %}
+      {# Remove type specific classes. #}
+      {% set attributes = attributes.removeClass(classes) %}
+    {% endfor %}
   </div>
-  {# Remove type specific classes. #}
-  {% set attributes = attributes.removeClass(classes) %}
-{% endfor %}
 </div>