Blocked by: #2853509: Don't render status messages if there are no messages but also include their assets if there might be

Problem/Motivation

Provide a consistent method for displaying messages in JavaScript.

Currently the Settings tray module needs to provide various message via JavaScript in:
#2773601: Display "You are now in edit mode" prompt when user enters edit mode
#2785047: In Outside In mode, messages should appear in the off-canvas tray, not the main page

And also Inline Form Errors needs to be able to update messages through JavaScript:
#2876321: Update inline form errors summary after form validation with AJAX

Proposed resolution

Change div for {{ messages }} to always be rendered and add a data-drupal-messages attribute for JavaScript to select.

Add new methods for:

  • Adding a message
  • Removing one or multiple messages
  • Clearing all messages of a specific type
  • Clearing all messages

Remaining tasks

Review patch
Write change record

API changes

Persistent div for {{ messages }} would be required in themes.

API Additions

var messageInterface = Drupal.message();

var messageId = messageInterface.add(message, type, options);
var messageList = messageInterface.select(type);
messageInterface.remove(messageId);
messageInterface.remove(type);
messageInterface.clear();

Original report by kkaefer

While developing some JavaScript enhancements for Drupal, I am facing the problem of how to indicate the status of an action properly. For example, when editing a label, the user needs to know if the action has been successful (saving the new title) or if it failed. Of course, some widgets can use their own status indicating system (like the autocomplete textfield), but others have to display text.

But – we already have status messages in Drupal! Take a look at theme_status_message(). We just need to make sure that the status message div is always there so that we can inject new items via JavaScript... How about that? Post your opinion or your own suggestion for this issue.

Files: 

Comments

Anonymous’s picture

PLEASE! This was something I wanted a while ago.

Every theme would need to remove the if statement if $message != ''

would be great to have this.

Steven’s picture

There would need to be a style to make the div invisible if empty though, so that JS can reveal it. Otherwise you might get an empty container, depending on the theme.

lilou’s picture

Version: x.y.z » 7.x-dev
casey’s picture

Version: 7.x-dev » 8.x-dev

Yes please.

nod_’s picture

Issue tags: +js-novice

I'd like that, it should be fairly simple.

vineet.osscube’s picture

Assigned: Unassigned » vineet.osscube

Assigned !

vineet.osscube’s picture

FileSize
2.34 KB
PASSED: [[SimpleTest]]: [MySQL] 55,451 pass(es). View

So initial patch for review :)-

shivenduray’s picture

Patch is not working due to white space problem in patch "content_edit_ajax_message-7342336-7.patch" !!!

vineet.osscube’s picture

FileSize
1.65 KB
PASSED: [[SimpleTest]]: [MySQL] 55,343 pass(es). View

Hi Team,

I've updated the toolbar-view.js file to show status message under div as a solution for displaying status message on field edit. Attaching patch for review.

vineet.osscube’s picture

Status: Active » Needs review
FileSize
1.65 KB
PASSED: [[SimpleTest]]: [MySQL] 55,355 pass(es). View

I've updated the toolbar-view.js file to show status message under div as a solution for displaying status message on field edit. Attaching patch for review :)-

nod_’s picture

Status: Needs review » Needs work

The current patch won't work for core. We need something more generic.

If we're going down this road we need to add a JS API to handle messages. Like it was said in #1 we'd need all theme to remove the if ($messages) {} so that the DOM element is always available. We could be using that for the tabledrag and views warning about unsaved changes too.

Ideally I wouldn't mind to have Drupal.announce use that message area to put all it's stuff for screen readers too. Would keep everything at the same place.

nod_’s picture

Status: Needs work » Needs review
FileSize
3.43 KB
PASSED: [[SimpleTest]]: [MySQL] 55,500 pass(es). View

Totally not ready but putting NR for attention.

Patch contains what I mean. Needs to be polished and all, probably merged with drupal.announce too actually.

adds 2 methods:

// Add a message to the message area. Used the same as drupal_set_message().
Drupal.message(message, type);

// Remove everything in the message area.
Drupal.message.clear();

The message area is designated with a data-drupal-messages attribute because id of the zone is different in default template and the seven theme. Also bartik adds a wrapper we don't want to have to deal with.

I don't think we want a fancy "dismiss message" functionality. If you want to get rid of the messages, just reload the page like it's done currently. simple > fancy.

jessebeach’s picture

Needs to be polished and all, probably merged with drupal.announce too actually.

(bolding added).

Yes, indeed. We've already solved an issue with aria-live and near-simultaneous messages: #1959306: Drupal.announce does not handle multiple messages at the same time; Improve the utility so that simultaneously calls are read..

I'd be fine with renaming announce to message or with merging a visual output component to announce in addition to the audio output it currently supports.

I see mention of aria-live in comments in the patch.

+ * Builds a div element with the aria-live attribute and attaches it
+ * to the DOM.

but I don't see the attribute in the code yet.

+Drupal.theme.message = function (message, type) {
+  return '<div class="messages ' + type +'">' + message + '</div>';
+};

vineet.osscube, do you feel like you can take a shot at merging the code here in this issue into the existing announce method?

nod_’s picture

yeah I copied the annouce file and removed all the comments, not all of them it seems. It'll need commenting.

nod_’s picture

FileSize
7.15 KB
PASSED: [[SimpleTest]]: [MySQL] 55,401 pass(es). View

merged in announce.js, not too bad. At least I don't have the whole library mess like before.

ericduran’s picture

+++ b/core/misc/message.jsundefined
@@ -0,0 +1,38 @@
+  messagesElement.style.display = 'block';

Hmm, Do we really need to do this?

Wouldn't it be better to do a simple class toggling.

nod_’s picture

yeah that was a very rough patch. new one is better.

ericduran’s picture

Also a lot of people are going to dis-like the forcing of the message block to be printed at all times.

oresh’s picture

print only when it fails :)

Wim Leers’s picture

Status: Needs review » Needs work

Great to see this nicely integrated with Drupal.announce :)

+++ b/core/misc/announce.jsundefined
@@ -25,6 +28,9 @@
+      if (!messagesElement) {
+        messagesElement = document.querySelector('[data-drupal-messages]');
+      }
       // Create only one aria-live element.
       if (!liveElement) {
         liveElement = document.createElement('div');
@@ -32,7 +38,7 @@

@@ -32,7 +38,7 @@
         liveElement.className = 'element-invisible';
         liveElement.setAttribute('aria-live', 'polite');
         liveElement.setAttribute('aria-busy', 'false');
-        document.body.appendChild(liveElement);
+        messagesElement.parentNode.insertBefore(liveElement, messagesElement);

- What if a theme specifically doesn't want a <div data-drupal-messages />? Shouldn't that still be allowed?
- When a theme specifically doesn't have such an element, this will break Drupal.announce() too, because it now effectively depends on such an element.

+++ b/core/misc/announce.jsundefined
@@ -41,34 +47,47 @@
+    if (messages.length) {
+      text = [];
+      for (var n = 0, nl = messages.length; n < nl; n++) {
+        message = messages.pop();

I don't like that Drupal.announce() is now also handling the rendering for Drupal.message().

AFAICT there are valid use cases to use Drupal.announce() *without* showing a visible message, so I don't think we should let this internal overlap occur.

seutje’s picture

It also seems to imply a single instance, is that something we want to force people into, or would supporting multiple messages areas be a bit over the top?

Wim Leers’s picture

#21: Great point!

nod_’s picture

about [drupal-data-messages] I think it should be required for theme to have an element with the attribute all the time.

Technically it's not Drupal.announce() that takes care of displaying, it's announce(). If I rename it processQueues() would that help? it's what it's doing after all. If you want announce and not a message then don't fill both queues at the same time :p

I'd say we keep it in only one place. If contrib want to use it somewhere else they'll probably want to be able to "stick" one message over pages, close a specific message, make it show up fancy and all. All sort of stuff we're not supporting with the current patch. I'm pretty sure someone will throw together a module with some jQuery plugin to handle that (and they'll just need to override Drupal.message()).

nod_’s picture

Status: Needs work » Needs review
FileSize
8.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-status-messages-77245-24.patch. Unable to apply patch. See the log in the details link for more information. View

Updated patch, there is a problem in how the debounce is actually used.

I separated the messages and announce processing so that should make you happy Wim :) They're even in two files just in case contrib wants to mess with it.

nod_’s picture

FileSize
8.15 KB
PASSED: [[SimpleTest]]: [MySQL] 56,060 pass(es). View

ok little small patch creation issue :p

YesCT’s picture

Issue tags: +Needs manual testing

tagging

nod_’s picture

BTW to see what's happening try that in the JS console:

Drupal.message('good message');
Drupal.message('Bad message', 'error');
Drupal.message('Careful message', 'warning');
Drupal.message('Whatever', 'whatever');
attiks’s picture

taging

designfitsu’s picture

Patch doesn't apply anymore since removal of tpl.php files for TWIG. Tagging for reroll.

socketwench’s picture

I'll reroll it.

socketwench’s picture

Assigned: socketwench » Unassigned
FileSize
6.21 KB
None View

Re-rolled.

jibran’s picture

Status: Needs review » Needs work

I think some page.html.twig changes are missing. Thanks for the re-roll.

nod_’s picture

Status: Needs work » Needs review
FileSize
8.03 KB
PASSED: [[SimpleTest]]: [MySQL] 55,715 pass(es). View

reroll with template changes.

mr_tuff’s picture

Status: Needs review » Needs work

At DrupalCon Portland 2013 I was assigned to manually test this patch. Patch 25 worked for me but changes happened rapidly and I was unsure how to reset my git branch to the latest.

Patch 25 tests I completed:

System:
OS X 10.8.3 (12D78)

Browsers tested:
Safari
6.0.4 (8536.29.13) - PASSED

Firefox
13.0.1 and 21.0 - PASSED

Opera
12.15 build 1748 - PASSED

UPDATE: I did some reading and figured out how to update my branch and apply the patch but now I get this:

MBP:drupal user$ patch -p1 < core-js-status-messages-77245-33.patch
patching file core/misc/announce.js
The next patch would create the file core/misc/message.js,
which already exists! Assume -R? [n] yes
patching file core/misc/message.js
patching file core/modules/system/system.module
Hunk #1 succeeded at 1212 (offset -13 lines).
patching file core/modules/system/templates/page.html.twig
patching file core/themes/bartik/templates/page.html.twig
patching file core/themes/seven/templates/page.html.twig
MBP:drupal user$ git status
# On branch issue_77245
# Changes not staged for commit:
# (use "git add ..." to update what will be committed)
# (use "git checkout -- ..." to discard changes in working directory)
#
# modified: core/misc/announce.js
# modified: core/modules/system/system.module
# modified: core/modules/system/templates/page.html.twig
# modified: core/themes/bartik/templates/page.html.twig
# modified: core/themes/seven/templates/page.html.twig
#
# Untracked files:
# (use "git add ..." to include in what will be committed)
#
# core-js-status-messages-77245-33.patch
# core/modules/system/system.module.orig
# sites/default/files/
# sites/default/settings.php
no changes added to commit (use "git add" and/or "git commit -a")

I've exhausted my current level of skills. Hope this helps.

nod_’s picture

Status: Needs work » Needs review

The important part is the testing. For git as long as the test bot doesn't complain it's all good.

sarav.din33’s picture

Once i apply #33 using following command,

In Terminal:
git apply core-js-status-messages-77245-33.patch

My patch file is in my root folder. Its named as "core-js-status-messages-77245-33.patch"

I got the following error:
error: patch failed: core/misc/announce.js:16
error: core/misc/announce.js: patch does not apply

Is there any mistake in unix command?

Wim Leers’s picture

Status: Needs review » Needs work

#36: No, Drupal 8 has just changed so much already that the patch no longer is applicable. It needs to be rerolled. Hence setting to "needs work".

sarav.din33’s picture

FileSize
7.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-status-messages-77245-38.patch. Unable to apply patch. See the log in the details link for more information. View

#33 Patch rerolled. Refer (https://drupal.org/patch/reroll)

sarav.din33’s picture

Status: Needs work » Needs review

Can any one apply the rerolled patch??

nod_’s picture

Thanks for the reroll, works fine. Review away :)

nod_’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs manual testing, +js-novice

The last submitted patch, core-js-status-messages-77245-38.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
7.9 KB
PASSED: [[SimpleTest]]: [MySQL] 57,621 pass(es). View

reroll

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Needs manual testing, -js-novice

The last submitted patch, core-js-status-messages-77245-43.patch, failed testing.

sarav.din33’s picture

Status: Needs work » Needs review
Wim Leers’s picture

nod_’s picture

rteijeiro’s picture

FileSize
7.79 KB
PASSED: [[SimpleTest]]: [MySQL] 58,850 pass(es). View

Re-rolled.

dealancer’s picture

Messaging functionality works fine for me, but I have not tested screen reading yet.

nod_’s picture

Issue tags: +markup

Patch is good to go, pinged morten about adding empty div everywhere.

mortendk’s picture

Issue tags: -markup

Can we please not use ID's if they are not absolutely extremely nessesary - they make the css work further down the road harder.

  1. +++ b/core/modules/system/templates/page.html.twig
    @@ -104,7 +104,7 @@
    -  {{ messages }}
    +  <div id="messages" data-drupal-messages>{{ messages }}</div>
    

    please dont use id's unless its absolutely nessesary, id to make the life of the css harder.
    maybe a class="drupalsystem--messages" instead

    Im generally not happy for having empty divs floating around unless theres a value inside of em. if we can test on {{ messages }} and test if its empty its fine with me even that its a bit cluttered

  2. +++ b/core/themes/bartik/templates/page.html.twig
    @@ -128,11 +128,9 @@
    +  <div id="messages"><div class="section clearfix" data-drupal-messages>
    +    {{ messages }}
    +  </div></div> <!-- /.section, /#messages -->
    

    bartik is its own monster (that the frontend team will take on later based on the principles of d8 theming) anyways please dont add more id's in use a class like drupalsystem--messages instead

jessebeach’s picture

This should be the only drupal system messages div on the page, so for the purposes of JavaScript, having an ID makes finding it easier. The document keeps a list of ID'ed elements on the page and looking them up is very fast.

For the purposes of styling, I agree, we shouldn't be using the ID. I don't see any CSS in this patch, so it's not introducing any ID-keyed styling.

jessebeach’s picture

Status: Needs review » Needs work
--- /dev/null
+++ b/core/misc/message.jsundefined
@@ -0,0 +1,58 @@
+/**
+ *
+ */

The messages.js file is missing the file doc block.

+++ b/core/misc/message.jsundefined
@@ -0,0 +1,58 @@
+    // Save the text and priority into a closure variable. Multiple simultaneous
+    // announcements will be concatenated and read in sequence.

This is a copy-paste block of comment from announce.js. It should be updated.

rteijeiro’s picture

Assigned: Unassigned » rteijeiro

I will fix it ;)

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,814 pass(es). View

I guess this patch contains all @mortendk and @jessebeach suggestions. Tried to comment the function but I guess it's too verbose. I accept more suggestions regarding that :)

Re-rolled with the latest changes and updated page.html.twig for Seven too.

Also I have a question about the return of Drupal.theme.message function:

return '<div class="messages messages--' + type + '">' + message + '</div>';

It includes a new div so I'm not sure if we should update it according to @mortendk suggestions.

nod_’s picture

You can't put the if messages. The element has to be on the page all the time, othewise JS can't find it and the whole thing can't work.

jessebeach’s picture

Also I have a question about the return of Drupal.theme.message function:

return '<div class="messages messages--' + type + '">' + message + '</div>';

It includes a new div so I'm not sure if we should update it according to @mortendk suggestions.

rteijeiro, I'm not sure how you're suggesting to update this div. Can you elaborate?

jessebeach’s picture

Also I have a question about the return of Drupal.theme.message function:

return '<div class="messages messages--' + type + '">' + message + '</div>';

It includes a new div so I'm not sure if we should update it according to @mortendk suggestions.

rteijeiro, I'm not sure how you're suggesting to update this div. Can you elaborate?

rteijeiro’s picture

Status: Needs review » Needs work

@nod_: Ok, I will fix it :'(

@jessebeach: I mean include the "drupalsystem--messages" class suggested by @mortendk instead the "messages" class.

mortendk’s picture

urgh just rmemebered the updated name conversions for css classes

the classname should be prefixed with a js- (js-drupalmessages) that will tell the themers to stay away from messing iwth that class & follow our standards (https://drupal.org/node/1887918#separate-concerns)

rteijeiro’s picture

@mortendk: The js-drupalmessages class should be in the tpl div or in the div returned by Drupal.theme.message function in the message.js file?

I will try to ping you in IRC in order to get this issue fixed ;)

nod_’s picture

Category: feature » task
Status: Needs work » Needs review
Issue tags: -Needs issue summary update +JavaScript
FileSize
8.27 KB
PASSED: [[SimpleTest]]: [MySQL] 58,749 pass(es). View

Talked some more with mortendk, here is the reroll.

This patch adds a (usually empty) <div data-drupal-messages> on all pages for all themes to be able to print errors and warnings from the JS (to finally fix #1232416: Drupal alerts "An AJAX HTTP request terminated abnormally" during normal site operation, confusing site visitors/editors).

I did not touch the classes/ids or anything because it's purely a JS patch. To mess with the markup a follow-up is needed, as long as the data- attribute is still around, the JS will work regardless of what happens to the markup/classes/ids.

The patch also fixes a bad use of debounce in announce.js.

nod_’s picture

Issue summary: View changes

Update issue summary

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

It looks good. Patch applies well and it creates the div placeholder for messages.

Maybe RTBC now?

seutje’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/misc/announce.js
@@ -42,38 +42,43 @@
-    var priority = 'polite';
-    var announcement;
+    var priority, announcement;

Looks like this breaks the default priority, making it default to "undefined".

Also, the initial implementation didn't seem to follow the spec, as that says that "off" is the default, not "polite".

ref: http://www.w3.org/TR/wai-aria/states_and_properties#aria-live

Also, I was under the impression the idea was to have the messages trigger the announce, otherwise it would be impossible for a11y users to know there was an error, wouldn't it?

nod_’s picture

Making .message use .annonce is interesting. for now I just want whatever to close down the HTTP:0 issue

rteijeiro’s picture

Assigned: rteijeiro » Unassigned
rteijeiro’s picture

Issue summary: View changes

update for #62 patch

nod_’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.53 KB
FAILED: [[SimpleTest]]: [MySQL] 59,324 pass(es), 0 fail(s), and 53 exception(s). View

rerolled to fixed the polite thing raised by seutje.

Status: Needs review » Needs work

The last submitted patch, 67: core-js-status-messages-77245-67.patch, failed testing.

rteijeiro’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 67: core-js-status-messages-77245-67.patch, failed testing.

The last submitted patch, 67: core-js-status-messages-77245-67.patch, failed testing.

Wim Leers’s picture

The patch in #67 simply does not include message.js, hence the test failures.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
8.22 KB
PASSED: [[SimpleTest]]: [MySQL] 59,414 pass(es). View

Thanks @Win Leers for the help :)

Added message.js file to #67 patch

jibran’s picture

+++ b/core/modules/system/system.module
@@ -983,6 +984,19 @@ function system_library_info() {
+  ¶

Extra white space.

javisr’s picture

Issue tags: +D8SVQ, +SprintWeekend2014
FileSize
8.21 KB
PASSED: [[SimpleTest]]: [MySQL] 63,087 pass(es). View
415 bytes

remove whitespace commented in #74

idflood’s picture

Tested the patch in #75 by calling the following javascript from the chrome dev tool:

Drupal.message('Testing javascript message error', 'error');
Drupal.message('Testing javascript message status', 'status');
Drupal.message('Testing javascript message warning', 'warning');

This worked beautifully and added the message to the page (tested with bartik, stark and seven themes). The patch also applies cleanly so maybe this can be marked RTBC.

bannockree’s picture

Issue tags: -Needs manual testing

I read through all the comments, and while I didn't see any specifics on what needed manual testing, it appears that adequate manual testing has been done, so I'm removing the "needs manual testing" tag. Please put it back if I'm wrong!

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Ah yes manual testing was in #76. Because no code is using it yet, needs to be triggered manually :)

RTBC per #76.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

I'm sorry, there are a bunch of nitpicks left:

  1. +++ b/core/misc/message.js
    @@ -0,0 +1,58 @@
    + *
    

    Missing file-level docs.

  2. +++ b/core/misc/message.js
    @@ -0,0 +1,58 @@
    +
    +
    

    2 newlines, should probably be 1.

  3. +++ b/core/misc/message.js
    @@ -0,0 +1,58 @@
    +   * to the DOM.
    ...
    +   * Builds a div element with the aria-live attribute and attaches it
    

    Incorrect wrapping: more of the text fits on the first line.

  4. +++ b/core/misc/message.js
    @@ -0,0 +1,58 @@
    +  Drupal.theme.message = function (message) {
    

    Docs missing.

  5. +++ b/core/misc/message.js
    @@ -0,0 +1,58 @@
    +  Drupal.message = function (message, type) {
    

    Docs missing.

  6. +++ b/core/misc/message.js
    @@ -0,0 +1,58 @@
    +  function processMessages () {
    

    I *think* that we also want docs for this.

idflood’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
8.81 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View

This patch should fix the nitpicks described in #79. However there are certainly better comments to add than what I have written.

Status: Needs review » Needs work

The last submitted patch, 80: core-js-status-messages-77245-80.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
idflood’s picture

Wim Leers’s picture

  1. +++ b/core/misc/message.js
    @@ -41,6 +45,15 @@
    +   * Display a message on the page.
    
    @@ -23,7 +24,10 @@
    +   * Display all queued messages in one pass instead of one after the other.
    

    s/Display/Displays/

  2. +++ b/core/misc/message.js
    @@ -41,6 +45,15 @@
    +   * @param message
    ...
    +   * @param type
    
    @@ -51,6 +64,16 @@
    +   * @param message
    ...
    +   * @return
    

    Parameter types missing

  3. +++ b/core/misc/message.js
    @@ -51,6 +64,16 @@
    +   *   This function return a string.
    

    Inconsistent with other JS theme functions in core.

  4. +++ b/core/misc/message.js
    @@ -1,5 +1,7 @@
    + * @file
      *
    + * Message API.
    

    Extraneous empty line in the middle.

Wim Leers’s picture

Status: Needs review » Needs work
idflood’s picture

Status: Needs work » Needs review
FileSize
1.45 KB
8.87 KB
PASSED: [[SimpleTest]]: [MySQL] 63,308 pass(es). View

Thanks @Wim Leers for the quick review.

I'm not sure about the new @return but I found this in toolbar.js and dropbuttons.js.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks good now! :)

The last submitted patch, 80: core-js-status-messages-77245-80.patch, failed testing.

nod_’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.74 KB
PASSED: [[SimpleTest]]: [MySQL] 63,214 pass(es). View

This is a reroll because the indentation standard changed for JavaScript files. No need for commit credit because of this reroll.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Reroll seems same as last patch so back to RTBC then.

droplet’s picture

few questions:

- Why no functions to remove messages posted by Drupal.message?
- Why not filter the undefined messages.
eg:

Drupal.message();
//return: <div class="messages messages--status">undefined</div>

- What if there's a lightbox windows. How to display messages there?
- Should it "throw new Error" that block whole scripts from being executed?
- It will be more useful if debouncedProcessMessages() can be triggered via events.

catch’s picture

Status: Reviewed & tested by the community » Needs review
nod_’s picture

FileSize
9.74 KB
PASSED: [[SimpleTest]]: [MySQL] 64,426 pass(es). View

Nothing to remove messages because I don't want this script to be complicated. The current errors we get from PHP can't be removed, you have to reload to page to get them to disapear. This is intented to display ajax errors (as you can see by the dependency), nothing else in core. If we give the ability to remove messages it's a lot more complex to handle, there are UX concerns and that always takes time to solve.

The lightbox question is a good point. Didn't think of that.

The throw won't stop anything else from executing. We catch attach behaviors is wrapped in a try/catch, that won't prevent anything else from running.

About events not sure it's very useful. Instead of Drupal.message() we'd call $().trigger('message', 'mytext'); ? Events are not the fastest thing ever, we add a dep on jQuery and I don't see how it'd be better. Do you have something in mind?

Here is a reroll to prevent undefined messages to show up.

nod_’s picture

FileSize
2.12 KB
droplet’s picture

  • This is intented to display ajax errors (as you can see by the dependency),

    It would be more important to have a function to remove the message then. Even an UI changes with "Close" button.

    Imagine there's a uploader that only allow to upload JPG and 100kb of the max size. And then you uploaded a PNG.

  • First error: wrong image type

    And then you upload a JPG and files size larger than 100

    Second error: max file size problem

    Now, you have 2 error messages. Do you still upload a wrong type ??

  • About events not sure it's very useful. Instead of Drupal.message() we'd call $().trigger('message', 'mytext'); ? Events are not the fastest thing ever, we add a dep on jQuery and I don't see how it'd be better. Do you have something in mind?

    Eg. we could register a function to remove the message after 10s...

  • Also,
    if it accepts selectors, it could reuse on other modules, eg. Using on tabledrag to display message (tableDragChangedWarning)
  • +++ b/core/misc/message.js
    @@ -0,0 +1,82 @@
    +  Drupal.behaviors.drupalMessage = {
    

    And I think we can remove the behaviors. and setup it on first message call.

Bevan’s picture

Status: Needs review » Needs work
lanchez’s picture

Looked a bit into this and it needs to be at least converted to use libraries.yml. I can look more into it tomorrow.

rteijeiro’s picture

Issue tags: +FUDK
lanchez’s picture

The message adding is going to break after console div is removed in this issue https://www.drupal.org/node/2213583 . The messages could be probably added before region-content or after main-content link?

Also now with core.libraries.yml the js needs to be added as dependency to other script to be included everywhere. Which one should it be? I didn't make a patch yet, but the content would be pretty much this in core.libraries.yml:

drupal.message:
version: VERSION
js:
misc/message.js: {}
dependencies:
- core/drupal
- core/drupal.debounce

LewisNyman’s picture

heddn’s picture

Is this still being worked on for 8.0 or is it bumped to 8.1 now?

Wim Leers’s picture

heddn’s picture

Issue tags: +Needs beta evaluation
nod_’s picture

Issue tags: +Needs reroll

I would like to have it in 8.0.x but it needs someone to work on it.

googletorp’s picture

Status: Needs work » Needs review
FileSize
10.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,775 pass(es), 6 fail(s), and 0 exception(s). View

This is basically a reroll of #94.

The patch from is so old, that I won't try to actually make an interdiff.txt. The JS from the patch is untouched, the changes is the "new" way of defining a library, attaching it to the system_messages and changes to new templates, since system message has changed a lot since the patch from a year ago.

googletorp’s picture

Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 105: a_place_for_javascript-77245-105.patch, failed testing.

googletorp’s picture

Status: Needs work » Needs review
FileSize
9.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,777 pass(es). View
1.71 KB

We shouldn't attached the drupal.message library by default, also fixes some JSLint issues.

droplet’s picture

Status: Needs review » Needs work

It's more than reroll. @see #95.

googletorp’s picture

Regarding #95

I don't believe that having the messages disappear after a certain amount of time, is a good idea, since we don't know how complex and how many messages will appear, and thus how long they should be on the page.

I don't see that message.js will have any sensible way of determine which messages should be removed, when. We could add a function to remove all messages generated by message.js or provide a context or something similar to that you could remove all validation errors for a specific form upon resubmitting an ajax form. Then it would be up to the scripts invoking the messages, to handle the removal when it makes sense.

Behaviors provides a nice error handling, if we don't have use then, we would need to code our own handling of a case where we don't have a place to put messages.

googletorp’s picture

Status: Needs work » Needs review
FileSize
10.55 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch a_place_for_javascript-77245-111.patch. Unable to apply patch. See the log in the details link for more information. View
2.53 KB

I've added the concept of context, so it's possible to specify where messages are coming from. I've added a method to remove a messages of a certain type/context, though this could be done manually as well.

This should handle the concerns from #95.

It still uses behaviors as I believe this is the best option we have atm.

droplet’s picture

Issue summary: View changes
FileSize
18.24 KB

- On maintenance mode, upload a file
(same problem on unpatch)

googletorp’s picture

@droplet what we are making is a framework, the patch itself only makes the framework available - it's not actually used anywhere (yet).

googletorp’s picture

To test this

  1. You need to include the library on the page, basically add $build['#attached'] = ['library' => ['core/drupal.message']]; somewhere, see patch #105 for more info (this was removed, since we don't want to load a lot of javascript by default on pages)
  2. The javascript code to add a message would look like this: Drupal.message.add('the message', 'the type status/warning/error, 'optional context')

Status: Needs review » Needs work

The last submitted patch, 111: a_place_for_javascript-77245-111.patch, failed testing.

mgifford’s picture

Issue tags: +Drupal.announce
lokapujya’s picture

Do these messages go into the block mentioned in #102?

mgifford’s picture

Status: Needs work » Needs review
FileSize
7.04 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,472 pass(es). View

Just a re-roll.

Cottser’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: -Needs beta evaluation
tic2000’s picture

Status: Needs review » Needs work

The last patch misses message.js

andypost’s picture

Issue tags: +Needs reroll
kostyashupenko’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
10.61 KB
andypost’s picture

You need to include the library on the page

#type=messages element should add this library

Wim Leers’s picture

Status: Needs review » Needs work

+1 for #124.

  1. +++ b/core/core.libraries.yml
    @@ -225,6 +225,15 @@ drupal.machine-name:
    +drupal.message:
    +  version: VERSION
    +  js:
    +    misc/message.js: {}
    +  dependencies:
    +    - core/jquery
    +    - core/drupal
    +    - core/drupal.debounce
    

    Why no dependency on core/drupal.announce?

  2. +++ b/core/themes/classy/templates/misc/status-messages.html.twig
    @@ -21,7 +21,7 @@
    -{{ attach_library('classy/messages') }}
    

    This would be a regression today. So, if you do this, then we should add this to classy.info.yml:

    libraries-extend:
      core/drupal.message:
        - classy/messages
    
tic2000’s picture

Is this really addressing the issue? The announce part is good, because it reads the text, so it doesn't matter where on the page the action was initiated from.

The text is added to the top of the page and the top of the page may not be in view. Imagine this comment form using it. I submit through AJAX and the confirmation is all at the top. Sure in the case of comments you can get feedback by prepending the message to the form or adding the comment in it's place. But you have situations where this is not possible (the action is initiated from a table cell, that cell may be to narrow to display a message; or a subscribe form in the footer, you don't want a message to brake the footer layout, you just want a small message appear informing the user the subscription was successful). Something like https://material.angularjs.org/latest/demo/toast

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mgifford’s picture

lokapujya’s picture

Status: Needs work » Needs review
FileSize
11.13 KB
503 bytes

Rerolled.

#126: Yeah, but isn't that for contrib to do?

Status: Needs review » Needs work

The last submitted patch, 129: 77245-129.patch, failed testing.

jibran’s picture

  1. +++ b/core/misc/message.js
    @@ -0,0 +1,104 @@
    +        messagesElement = document.querySelector('[data-drupal-messages]');
    

    We can use jQuery selector here.

  2. +++ b/core/misc/message.js
    @@ -0,0 +1,104 @@
    +        message, n, nl;
    

    Let's rename n,nl to better helpful name.

joginderpc’s picture

Status: Needs work » Needs review
FileSize
7.98 KB

Updates following things as given in previous comments #131.

+++ b/core/misc/message.js
@@ -0,0 +1,104 @@
+ messagesElement = document.querySelector('[data-drupal-messages]');
We can use jQuery selector here.
+++ b/core/misc/message.js
@@ -0,0 +1,104 @@
+ message, n, nl;

Please review.

Status: Needs review » Needs work

The last submitted patch, 132: javascript_status_msg-77245-132.patch, failed testing.

jibran’s picture

message.js is missing from the patch.

mgifford’s picture

Issue tags: +accessibility

I'm tagging this for accessibility as it's required to get inline form errors as a fully fledged part of Core.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 132: javascript_status_msg-77245-132.patch, failed testing.

andrewmacpherson’s picture

Issue tags: +Needs reroll

The patch from #132 needs a re-roll, today's test says it fails to apply.

BarisW’s picture

Assigned: Unassigned » BarisW

On it

BarisW’s picture

Assigned: BarisW » Unassigned
Status: Needs work » Needs review
FileSize
10.71 KB
1.26 KB

Here is a re-roll. I've also re-added the messages.js file that was missing in patch #132.

Status: Needs review » Needs work

The last submitted patch, 140: a_place_for_javascript-77245-140.patch, failed testing.

hitesh-jain’s picture

Assigned: Unassigned » hitesh-jain
Manuel Garcia’s picture

Issue tags: -Needs reroll
hitesh-jain’s picture

Status: Needs work » Needs review
FileSize
6.38 KB

Patched against 8.3.x branch.

joelpittet’s picture

Trying to fix the twig template, the indentation went a bit wonky and div conditional went missing for the closing div.
Also undid the context removal but I don't think that's needed for this patch.

Bigpipe test should fail because the removal of the library, what's the reason behind that in the classy template?

The last submitted patch, 144: a_place_for_javascript-77245-144.patch, failed testing.

droplet’s picture

Is that intended to remove the JS in #140 and above patches..

the blocker is #91

I agreed with #126 also. We could jump out of the box. Here's an JS lib we can use:
http://codeseven.github.io/toastr/demo.html

joelpittet’s picture

Assigned: hitesh-jain » Unassigned
FileSize
8.82 KB

@droplet, good catch, it likely was a mistake because it wasn't mentioned. I've added it back into the latest patch.

Also probably blocked a bit on changing Classy markup which is API.

The last submitted patch, 145: 77245-145.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 148: 77245-148.patch, failed testing.

mgifford’s picture

Issue tags: +Needs tests

Just wanted to encourage someone to write tests for this patch so we can hopefully meet this deadline:
https://www.drupal.org/node/2504847#comment-11750733

dmsmidt’s picture

Adding my thoughts about message visibility and toast in #126 and #147.

I agree that when, for example, you are scrolled down and a message appears at the top of the page they are not really accessible.
This can however be addressed when we get Inline Form Errors in core (#2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE)).
What we are addressing here is that we have a central place to put the messages with JS, and keep them there.
That is a small enough change, and can later be extended with things like toasts.
Let's do things in small enough steps so that we can at least go on.
The above also covers @droplet's concern in #91.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tedbow’s picture

Status: Needs work » Needs review
FileSize
9.45 KB
16.07 KB

Ok trying to get this going again.

I was working on #2773601: Display "You are now in edit mode" prompt when user enters edit mode but hope we can get this done first so that issue can just use the core messages library.

I correct some eslint errors in messages.js. Also a couple logic errors.

Also had to update \Drupal\Core\Render\Element\StatusMessages::renderMessages because it was not showing the place holder if there were no messages.

I also add a test. But it is failing for me now. Not sure why.
If you enable the test module js_message_test you can see the show and remove message links. The module works for me but test doesn't. Not sure why

Status: Needs review » Needs work

The last submitted patch, 154: 77245-154.patch, failed testing.

drpal’s picture

  1. +++ b/core/misc/announce.js
    @@ -50,21 +50,23 @@
    +      for (var i = 0; i < il; i++) {
    

    announcements.reverse().forEach()

  2. +++ b/core/misc/announce.js
    @@ -50,21 +50,23 @@
           }
    

    Set announcements back to an empty array at the end.

  3. +++ b/core/misc/announce.js
    @@ -94,27 +101,23 @@
    +   * @param {String} text
    

    string

  4. +++ b/core/misc/announce.js
    @@ -94,27 +101,23 @@
    +   * @param {String} priority
    

    string

  5. +++ b/core/misc/message.js
    @@ -0,0 +1,106 @@
    +        if (!$('[data-drupal-messages]').length) {
    

    Capture this selector and store outside the behavior.

  6. +++ b/core/misc/message.js
    @@ -0,0 +1,106 @@
    +      for (messagesIndex = 0, messagesLength = messages.length; messagesIndex < messagesLength; messagesIndex++) {
    

    array.forEach()

  7. +++ b/core/misc/message.js
    @@ -0,0 +1,106 @@
    +   * @param {String} message
    

    string

  8. +++ b/core/misc/message.js
    @@ -0,0 +1,106 @@
    +   * @param {String} type
    

    string

  9. +++ b/core/misc/message.js
    @@ -0,0 +1,106 @@
    +   * @param {String} context
    

    string

  10. +++ b/core/misc/message.js
    @@ -0,0 +1,106 @@
    +   * @param {Object} message
    

    Update JSDoc format for object/key descriptions.

  11. +++ b/core/misc/message.js
    @@ -0,0 +1,106 @@
    +   * @return {String}
    

    string

tedbow’s picture

Status: Needs work » Needs review
FileSize
9.26 KB
18.65 KB

@drpal thanks for the review!

1,2 I think are out of scope :(

6. had to use messages.reverse().forEach() to keep message order

fixed all the rest.

Also in Drupal.theme.message there is no need for the context parameter.

Got JsMessageTest passing it was missing a library dependency in the test module.

The other tests that are failing I think are because of the changes to \Drupal\Core\Render\Element\StatusMessages::renderMessages but I haven't had time to look at that yet.

Status: Needs review » Needs work

The last submitted patch, 157: 77245-157.patch, failed testing.

drpal’s picture

  1. +++ b/core/misc/announce.js
    @@ -94,27 +101,23 @@
    +   * @param {String} text
    

    string

  2. +++ b/core/misc/announce.js
    @@ -94,27 +101,23 @@
    +   * @param {String} priority
    

    string

  3. +++ b/core/misc/announce.js
    @@ -94,27 +101,23 @@
    +    if (typeof text === 'string') {
    

    In addition to checking that text is a string, should we throw an error if text is not passed it, or is not actually a string?

  4. +++ b/core/misc/message.js
    @@ -0,0 +1,109 @@
    +      debouncedProcessMessages(messages);
    

    No need to pass messages.

  5. +++ b/core/misc/message.js
    @@ -0,0 +1,109 @@
    +    $('.messages--' + type + '.js-messages-context--' + context).remove();
    

    It might be nice to have the ability to remove all messages of a given context regardless of type.

    Drupal.message.remove = function (context, type) {
      context = context || 'default';
      $((type ? '.messages--' + type : '') + '.js-messages-context--' + context).remove();
    };
    
tedbow’s picture

Do want give the option to specify the element where the messages should go?

In #2785047: In Outside In mode, messages should appear in the off-canvas tray, not the main page I would like to just the new messages library but it will not work because the messages div will not be on the tray.

  1. +++ b/core/misc/message.js
    @@ -0,0 +1,109 @@
    +  Drupal.behaviors.drupalMessage = {
    +    attach: function () {
    +      if (!messagesElement) {
    +        var $messagesWrapper = $(messageWrapperSelector);
    +        if (!$messagesWrapper.length) {
    +          throw new Error(Drupal.t('There is no element with a [data-drupal-messages] attribute'));
    +        }
    +        messagesElement = $messagesWrapper[0];
    +      }
    +    }
    +  };
    

    Then we wouldn't want to do this on attach but when I message is added to check if container is available.

  2. +++ b/core/misc/message.js
    @@ -0,0 +1,109 @@
    +  Drupal.message.add = function (message, type, context) {
    

    Here we could add containerSelector which would default to '[data-drupal-messages]'.

I think beyond the Settings Tray module any interaction in a dialog is going to run into this same problem. I think upcoming Media work will need this ability because the browser will be in a dialog.

tedbow’s picture

Status: Needs work » Needs review
FileSize
7.71 KB
20.3 KB

@drupal thanks for review in #159

1-2 fixed
3. Won't if (typeof text === 'string') { not be true if it wasn't passed. I just added an else here and threw an error.
4. fixed
5. Yes I think this could be good. If a module wants to remove all messages it added but not other modules they could use context.
I added this and added test coverage.

Status: Needs review » Needs work

The last submitted patch, 161: 77245-161.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
20.33 KB

Just test fixes.

BigPipe test will still fail. Looking into that.

Wim Leers’s picture

+++ b/core/themes/classy/templates/misc/status-messages.html.twig
@@ -21,7 +21,7 @@
-{{ attach_library('classy/messages') }}

Why would classy/messages no longer be necessary?

Wim Leers’s picture

+++ b/core/themes/classy/templates/misc/status-messages.html.twig
@@ -21,7 +21,7 @@
+<div data-drupal-messages>
 {% block messages %}
 {% for type, messages in message_list %}
   {%
@@ -54,3 +54,4 @@

@@ -54,3 +54,4 @@
   {% set attributes = attributes.removeClass(classes) %}
 {% endfor %}
 {% endblock messages %}
+</div>
 

This changed the markup for status messages. BigPipe's expectations therefore need to be updated as well.

This fixes the "no-js" BigPipe fail, and part of the other BigPipe fail.

Wim Leers’s picture

+++ b/core/themes/classy/templates/misc/status-messages.html.twig
@@ -21,7 +21,7 @@
-{{ attach_library('classy/messages') }}

Why would classy/messages no longer be necessary?

Because this asset library is no longer being attached in the template, that's another BigPipe expectation we need to update: when BigPipe is using JS, it no longer needs to update the AJAX page state nor load the CSS file in this asset library.

I am posting this fix separately (from #165) because I'm not at all convinced that removing that asset library is correct. So then it's easy to revert this change.

Finally, if this change is correct, then you will need to expand BigPipe's test coverage with a case where it is explicitly loading some CSS or JS. Because this is effectively reducing BigPipe's test coverage.

The last submitted patch, 163: 77245-163.patch, failed testing.

The last submitted patch, 165: 77245-165.patch, failed testing.

Pavan B S’s picture

  1. +++ b/core/misc/message.js
    @@ -0,0 +1,110 @@
    +   * Builds a div element with the aria-live attribute and attaches it to the DOM.
    

    LIne exceeding 80 characters

  2. +++ b/core/misc/message.js
    @@ -0,0 +1,110 @@
    +      // Save the text and priority into a closure variable. Multiple simultaneous
    

    Line exceeding 80 characters

Fixed coding standard error in the patch, please give me suggestion if i am wrong

Status: Needs review » Needs work

The last submitted patch, 169: 77245-167.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
22.52 KB

@Pavan B S thanks for the patch but you have a bunch of unrelated files in there.

Re #164 I think this is mistake. I can confirm with the library the regular messages are not theme correctly.

Ok added back "classy/messages" library.
Update BigPipeTest

Status: Needs review » Needs work

The last submitted patch, 171: 77245-171.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
17.06 KB
26.97 KB

Ok so this implements the ability to send messages to different divs. See my idea in #160

This will be needed for the Settings Tray module.

tedbow’s picture

Title: A place for JavaScript status messages » Provide a common API for displaying JavaScript messages
Issue summary: View changes

Update summary.

Right now Drupal.message.add() does not use Drupal.announce. I think it should right?

tedbow’s picture

Ok. so this calls Drupal.annouce() inside of Drupal.message.add().

It seems like every on screen message should be "announced" for screen readers but maybe I don't know the idea behind it.

@todo add test coverage is this is the correct behaviour.

Status: Needs review » Needs work

The last submitted patch, 175: 77245-175.patch, failed testing.

nod_’s picture

Been a while since I checked this issue :p

I'm not happy about adding a jquery dependency for this script. There are no events involved, I don't see the need for it. When we first wrote announce.js es6 wasn't nowhere near a possibility. We could make those array processing more modern. Patch incoming.

drpal’s picture

Here is a slight refactor. I talked with @tedbow and this skips the work in #175.

drpal’s picture

Status: Needs work » Needs review
nod_’s picture

All right, simplified the JS.

  • Got debounce out, when the method to add a message is called, it's added right away to the page. If someone uses it in a loop they'll have a bad time since a lot of messages will trash the page. That can always be added back if it's a real issue later.
  • Remove the context thing. Unless there is a good use-case I don't think we should handle it in core, if people want to group their message they can handle it in their module/code.
  • Made the API more generic, in case a module wants to override it (and I'm sure they'll want to).
  • Drupal.announce is called when a message is displayed. It's possible to add a separate string for the screenreader version of the message.

Made a few changes in the test js file to show how it can be used but no tests have been updated to pass.

There is an issue with the HTML, we should have a template for status wrapper and status message. To let themes add the data-drupal-messages attribute where they want. It shouldn't be in classy like that. Anyway if the js looks good we can look at the tests.

nod_’s picture

Issue summary: View changes

Oh and Drupal.message() expects an HTMLElement, no selector.

Status: Needs review » Needs work

The last submitted patch, 180: core-js-status-messages-77245-180.patch, failed testing.

tedbow’s picture

diff --git a/core/misc/announce.js b/core/misc/announce.js

diff --git a/core/misc/announce.js b/core/misc/announce.js
index acf850a..cd1ee5b 100644

@nod_ do you know why this patch contains changes to this file? It seems unrelated to the messages changes.

drpal’s picture

Status: Needs work » Needs review
FileSize
35.2 KB
20.39 KB
  • Fixes coding standards error from #180
  • Refactors remove functionality. Remove messages by id, nth-clild, arrays of both or just a single type
  • Tests. Thanks @tedbow

Status: Needs review » Needs work

The last submitted patch, 184: 77245-184.patch, failed testing.

nod_’s picture

Looking at the interdiff. Could you keep the queryselectorall and instead of select+remove in the foreach build an array of selectors (map would work better than foreach). Would help readability.

The separation of select and remove in separate steps was on purpose.

drpal’s picture

Status: Needs work » Needs review
FileSize
27.77 KB
13.23 KB

Interdiff is between the patch in #180.

drpal’s picture

Interdiff is between the patch in #180. Poor internet at DDD.

tedbow’s picture

  1. +++ b/core/misc/message.js
    @@ -70,27 +70,30 @@
    +      var removeSelector = (messages instanceof Array ? messages : [messages])
    

    Since this will be array of selectors at this point for each "message" should we change the name to removeSelectors?

  2. +++ b/core/misc/message.js
    @@ -70,27 +70,30 @@
    +      var remove = this.element.querySelectorAll(removeSelector.join(', '));
    

    Agree with @nod_ that it is more readable seperatable

  3. +++ b/core/misc/message.js
    @@ -70,27 +70,30 @@
    +            // If the index is numeric remove the element based on the DOM index.
    +            '[data-drupal-message]:nth-child(' + messageIndex + ')';
    

    I really don't see a use case for specify the nth child message.

    Presumably there would be JS from multiple modules calling messageAdd(). How would any 1 module know which index their messages were. They could easily know the order of their messages but they would not know if another module set a message in between theirs.

    It seems like they would have actually inspect the DOM to know the order of the messages. If they are inspecting the DOM anyways why would they not just read the data-drupal-message attribute to get the string index?

drpal’s picture

Addressed the feedback in #189.

tedbow’s picture

@drpal thanks

That is much more readable to me.

Just want clarify for anyone readying through that in #190 @drpal removed the ability to remove the nth child message as was asking for in #189.3

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

I think this is re my comment in #191

nod_’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for all the tests :)

Few things before commit: In message.js

We could do better for docs of the main entry point of this new API.

  /**
   * woot

s/%/@/

        throw new Error(Drupal.t('There is no @element on the page.', {'%element': defaultMessageWrapperSelector}));

Here I'd prefer Array.isArray().

messages instanceof Array

I'm wondering if we could simplify the api and not expose messageWrapper at all. Why would we have that in the api, It's passed to Drupal.message to begin with? I know it might be a bit different from our older apis but I'd like to start making conscious choices.

drpal’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
27.75 KB
1.76 KB

Some minor fixes from #193.

I removed messageWrapper from the exposed response from the constructor which should simplify everything.

Status: Needs review » Needs work

The last submitted patch, 194: 77245-194.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
283 bytes
27.78 KB
+++ b/core/misc/message.js
@@ -99,7 +103,6 @@
-      element: messageWrapper,

This patch just reverts this change. The add and remove methods will not work if this is removed.

  1. +++ b/core/modules/system/tests/modules/js_message_test/js/js_message_test.js
    @@ -0,0 +1,64 @@
    +          messageObjects[divSelector] = Drupal.message(document.querySelector(divSelector));
    

    Since how this API is used is you call Drupal.message() and receive by an object with the methods on it.

  2. +++ b/core/misc/message.js
    @@ -0,0 +1,160 @@
    +      this.element.innerHTML += Drupal.theme('message', {text: message, type: type}, options);
    ...
    +      var remove = this.element.querySelectorAll(removeSelectors.join(', '));
    

    Then the add and remove methods both need this.element so they can manipulate it.

+++ b/core/misc/message.js
@@ -16,10 +16,14 @@
-   * woot
+   * Constructs a new instance of the Drupal.message object.

I think we can all agree this is better ;)

I found another problem though.

I was testing manually in Bartik.
I forgot you need the test theme to make Javascript on the test module js_message_test.js to work.

So I was clicking the links that should make the messages appear in the div with data-drupal-messages-other
The message where appearing in the default message div because data-drupal-messages-other didn't exist.

Took me a second to figure out what was going on.

+++ b/core/modules/system/tests/modules/js_message_test/js/js_message_test.js
@@ -0,0 +1,64 @@
+          messageObjects[divSelector] = Drupal.message(document.querySelector(divSelector));

document.querySelector(divSelector) was returning null because the element wasn't on that page.

  1. +++ b/core/misc/message.js
    @@ -0,0 +1,160 @@
    +    if (typeof messageWrapper === 'string') {
    +      throw new Error(Drupal.t('Drupal.message() expect an HTMLElement as parameter.'));
    +    }
    +    if (!messageWrapper) {
    +      messageWrapper = document.querySelector(defaultMessageWrapperSelector);
    +      if (!messageWrapper) {
    +        throw new Error(Drupal.t('There is no @element on the page.', {'@element': defaultMessageWrapperSelector}));
    +      }
    +    }
    

    But here when messageWrapper is null it will call document.querySelector(defaultMessageWrapperSelector);.

    The message will then appear in the default messages div.

    I think it would be better to throw an error in this case.

Should we be worrying about that?

nod_’s picture

Status: Needs review » Needs work

I wanted to get rid of the use of this too, replace this.element with messageWrapper and it'll work we shouldn't add stuff just because.

If it fails and the error is not helpful, we should fix it. Let's check that if messageWrapper is not undefined messageWrapper.nodeType exists and equals to 1, that way we know we have a DOM Node of some kind. Might need to add this check all over the place too, to avoid surprises. But that's for another issue.

tedbow’s picture

Wrote this before I saw @nod_'s comment in #197

So I think the problem I mentioned in #196 I think is something that we should check for. Because Javascript code easily be written wrong to get the call to Drupal.message() but messages still would be output to the screen just in the default messages div rather than what is expected.

For example in both:
Drupal.message(document.querySelector('#one-character-off'));
and
Drupal.message($('#one-character-off')[0]);

No error will be thrown and the messages will be display in the default messages div which is not the intention.

This could fix both cases:

if ((messageWrapper === null || typeof messageWrapper === 'string')
    || (arguments.length === 1 && typeof messageWrapper === 'undefined')) {
      throw new Error(Drupal.t('Drupal.message() expect an HTMLElement as parameter.'));
}

You have to check for null because document.querySelector('#one-character-off') will send null

$('#one-character-off')[0] will send undefined but you can't simply check for undefined because sending no argument will also make typeof messageWrapper === 'undefined' so you have to check for arguments.length === 1

I think the point of adding these extra checks is not to babysit bad calls but not write messages to the wrong div.

thoughts? is this overkill?

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
27.92 KB
  1. re #197

    I wanted to get rid of the use of this too, replace this.element with messageWrapper and it'll work we shouldn't add stuff just because.

    Yeah whoops fixed this. Also removed doc comment that was referring to it that was never removed.

  2. Also added the extra check I was referring to in #198. As soon as I add this it actually required an update to the test because the "Show Multiple" and "Remove Multiple" in \Drupal\js_message_test\Controller\JSMessageTestController::messageLinks() didn't have a data-selector attribute so
    var divSelector = e.currentTarget.getAttribute('data-selector');
    if (!messageObjects.hasOwnProperty(divSelector)) {
      messageObjects[divSelector] = Drupal.message(document.querySelector(divSelector));
    }
    return messageObjects[divSelector];
    

    Was actually sendingnull as messageWrapper unintentionally.

    I think this makes the point that extra check is actually necessary.

  3. Also fixed a couple spelling errors. "screenreader" is actually two words. "Drupal.annonce" spelled wrong in comment.
andypost’s picture

nod_’s picture

Assigned: Unassigned » nod_
Status: Needs review » Needs work

Js supporting the tests has some issues.

I'm also adding a clear method to the message object.

dmsmidt’s picture

Was about to review, but I'll wait for nod_'s changes.

nod_’s picture

Status: Needs work » Needs review
FileSize
31.53 KB
16.02 KB

Let's see how it goes.

In the test JS messages indexes were stepping over each other and made it easy to have unremovable messages on the page.

nod_’s picture

Assigned: nod_ » Unassigned
dmsmidt’s picture

+++ b/core/misc/message.js
@@ -26,11 +26,13 @@
+      if (!messageWrapper || (messageWrapper && messageWrapper.nodeType !== 1)) {
+++ b/core/misc/message.js
@@ -68,22 +70,23 @@
+      if (!messages || (messages && Array.isArray(messages) && messages.length === 0)) {

Please educate me on why messageWrapper and messages need to be checked again? Can't we do:

 if (!messages || Array.isArray(messages) && messages.length === 0) {
nod_’s picture

lol yeah you're right, my mistake.

Status: Needs review » Needs work

The last submitted patch, 203: core-js-status-messages-77245-203.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
5.11 KB
31.58 KB

Turns out, Array.from isn't supported by IE 11, back to for loop.
Fixed tests and comments from #205

nod_’s picture

Issue summary: View changes

Touched the IS a bit. It'd be quite something to fix an 11 years old issue…

nod_’s picture

Issue summary: View changes
mgifford’s picture

@nod_ I like your comment in #5, 5 years ago :)

dmsmidt’s picture

Issue summary: View changes

11 years, wow, let's do this!

Added another issue from Inline Form Errors to the IS.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt

I'm currently reviewing code and testing with a screen reader.

dmsmidt’s picture

Assigned: dmsmidt » Unassigned
Status: Needs review » Needs work

First of all what a great progress! I'm so happy to see this and immediately want to start experimenting with toast messages for example (in contrib) :-p.

Some remarks:

  1. +++ b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
    @@ -75,19 +75,17 @@ public static function generatePlaceholder(array $element) {
         $render = [];
    

    This can now be removed.

  2. +++ b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
    @@ -75,19 +75,17 @@ public static function generatePlaceholder(array $element) {
    +    // Render the messages.
    

    Needs to say what happens now. Like: "Render the messages or an empty messages container div which can be used by the JavaScript messages API."

    And it would be great to improve the docblock as well so that @return and the description show there will always be a container.

  3. +++ b/core/misc/announce.js
    @@ -81,8 +83,13 @@
    +
    

    Nit: remove empty line

  4. +++ b/core/misc/announce.js
    @@ -96,25 +103,24 @@
    +    }
    

    Nit: Add empty line

  5. +++ b/core/modules/system/tests/modules/js_message_test/src/Controller/JSMessageTestController.php
    @@ -0,0 +1,128 @@
    +        ]
    

    Nit: missing comma

  6. +++ b/core/modules/system/tests/modules/js_message_test/src/Controller/JSMessageTestController.php
    @@ -0,0 +1,128 @@
    +      ]
    

    Nit: missing comma

  7. +++ b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
    index acf850a641..cd1ee5bb1a 100644
    --- a/core/misc/announce.js
    
    --- a/core/misc/announce.js
    +++ b/core/misc/announce.js
    

    All changes to announce.js seem out of scope. We can keep this patch much simpler that way. Although the changes look logical, we should do that in a separate issue. My tests without these changes work just as well!

  8. My tests with ChromeVox are succesful, the messages are read out aloud, jay!
    We should however improve it a bit. Currenlty it is unclear what type of message was added. I propose we by default first tell the type of message (error, status, warning) and then the actual message.
  9. +++ b/core/misc/message.js
    @@ -0,0 +1,188 @@
    +    // Check this. Might be too much.
    +    if (!options.priority && type !== 'status') {
    +      options.priority = 'assertive';
    +    }
    

    Checked here:
    https://www.w3.org/TR/wai-aria/states_and_properties#aria-live

    Let's keep polite as a default (see announce.js).
    And only set the priority to assertive on 'warnings' and 'errors'. This way, if devs introduce their own status (like a notice) it won't be so aggressive by default.

  10. +++ b/core/misc/message.js
    @@ -0,0 +1,188 @@
    +    function messageRemove(messages) {
    

    Can we rename 'messages' to 'index', this is much more understandable for me.

  11. --- a/core/themes/bartik/css/components/messages.css
    +++ b/core/themes/bartik/css/components/messages.css
    

    We need some before/after images for this.

  12. +++ b/core/misc/message.js
    @@ -0,0 +1,188 @@
    +      options.index = Math.random().toFixed(15).replace('0.', '');
    

    This makes it impossible to use a custom generated index. Allowing custom generated indexes opens up a lot of possibilities in contrib. And may be needed for Inline Form Errors.

  13. It is currently impossible to delete specific server side generated messages with the proposed API. (This is needed for Inline Form Errors)
    I think we should add the possibility to add a (custom or generated) index (data-drupal-message) via drupal_set_message().
dmsmidt’s picture

Issue summary: View changes

Update IS: relationship with drupal.announce() is clear now.

nod_’s picture

missing the dependency on the message css library too

drpal’s picture

Status: Needs work » Needs review
FileSize
28.48 KB
6.79 KB
  • 1,2 - fixed
  • 3,4 - I removed the changes to annouce.js for this patch, which could be posted in a new issue.
  • 5,6,7 - fixed
  • 8,9 - fixed
  • 10 - fixed
  • 12 - fixed
  • 13 - I feel like this should be a new issue.
drpal’s picture

FileSize
432 bytes

I'm sorry for breaking announce(). Everything else should still apply.

drpal’s picture

dmsmidt’s picture

Status: Needs review » Needs work

That is looking much better, thanks @drpal.

#13 in a separate issue seems legit.

However from checking the interdiff in #217 I think you misunderstood my accesibility goal.

+++ b/core/misc/message.js
@@ -154,12 +155,15 @@
   function announce(message, type, options) {
     // Check this. Might be too much.
     if (!options.priority && type !== 'status') {
-      options.priority = 'assertive';
+      options.priority = 'polite';
     }
+    // Always announce the priority of the upcomming message.
+    Drupal.announce(options.priority);

The "// Check this" line should be removed, and we can keep the options.priority = 'assertive part, but change type !== 'status' into (type === 'warning' || type === 'error'). This way all other types will be polite.

Also Drupal.announce(options.priority); doesn't sound to friendly or helpful and doesn't get announced with the same priority as the actual message. Can't we just use type as a starter?

drpal’s picture

@dmsmidt

Yep, totally my fault here. We should now be announcing the type of the upcoming message at the same priority as the message itself.

drpal’s picture

Status: Needs work » Needs review

The last submitted patch, 219: 77245-218.patch, failed testing.

The last submitted patch, 219: 77245-218.patch, failed testing.

tedbow’s picture

Pretty sure this will need to be re-rolled because this was reverted #2853509-16: Don't render status messages if there are no messages but also include their assets if there might be

Retesting to last patch to confirm it no longer applies.

Status: Needs review » Needs work

The last submitted patch, 221: 77245-221.patch, failed testing.

dmsmidt’s picture

@drpal, sorry for all the iterations, a11y is better now, but we can still improve.


+++ b/core/misc/message.js
@@ -0,0 +1,191 @@
+    // Always announce the type of the upcomming message.
+    Drupal.announce(type, options.priority);
+
+    // If screen reader message is not disabled announce screen reader specific
+    // text or fallback to the displayed message.
+    if (options.announce !== '') {
+      Drupal.announce(options.announce || message, options.priority);
+    }

Custom announce messages are currently never read out loud due to if (options.announce !== '') {.
Also let's not separately announce the type, it sounds funny/weird (yes I tried), I would concat it to the message like so: type + ': '+ message. But.. we should also make it possible to translate it, and then it becomes much more work, we would need a mapping of type => "read out loud version of type" and then we would like that on the server side generated messages as well (reminder: we could use #status_headings for that).
So let's remove announcing type from this patch and make a follow up (just like #217-13).

Furthermore #214-2 is not solved, comments/dockblock needs update.

@_nod, not sure how we should fix the messages.css dependency in Classy and Bartik.
We add css/components/messages.css to the Classy and Bartik libraries base library and remove all {{ attach_library('classy/messages') }}. Or we have to overwrite Drupal.theme.message in the themes and add it there?

We are getting somewhere, woot!

drpal’s picture

Status: Needs work » Needs review
FileSize
27.12 KB
1.85 KB

@dmsmidt

Haha, don't worry about it.

I think the changes in #214-2 are out of scope right now since we don't actually need to make any changes to this file. I also removed the announcement of the message type.

dmsmidt’s picture

Issue summary: View changes
Status: Needs review » Postponed

I did some more checks and saw that I missed another thing in previous reviews:

+++ b/core/misc/message.js
@@ -0,0 +1,190 @@
+    return '<div class="messages messages--' + message.type + '" ' +
+      'data-drupal-message="' + options.index + '"> ' + message.text + '</div>';

The aria-label is missing. It is present on server-side generated messages. The #status_headings in StatusMessages::renderMessages() should somehow be usable by the Message API.

But before we start refactoring this we should wait on #2853509: Don't render status messages if there are no messages but also include their assets if there might be, it's critical and changes the way StatusMessages::renderMessages() works again. But it'll also make sure we always have message styling.

nod_’s picture

Status: Postponed » Needs work

There is more work needed on the js side no need to hold things off

nod_’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
9.92 KB
29.29 KB

Added a message.select method to the api, put back the announce method like it was before, possibility to disable announce was a feature. Added all the attributes and structure of PHP's messages, that should help a11y.

Potential issues with error messages, since both the announce and the message itself could trigger, hence reading twice the error.

Status: Needs review » Needs work

The last submitted patch, 231: core-js-status-messages-77245-231.patch, failed testing.

drpal’s picture

  1. +++ b/core/misc/message.js
    @@ -0,0 +1,253 @@
    +      if (!index || (Array.isArray(index) && index.length === 0)) {
    

    We can just check that the length exists, not that it equals 0.

  2. +++ b/core/misc/message.js
    @@ -0,0 +1,253 @@
    +        return [];
    

    Maybe we can return a boolean or throw an error in this case.

  3. +++ b/core/misc/message.js
    @@ -0,0 +1,253 @@
    +      Drupal.announce(options.announce || message, options.priority);
    

    Do we want to make sure that options.annouce is a string?

  4. +++ b/core/misc/message.js
    --- a/core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php
    +++ b/core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php
    

    I'm not sure these are needed anymore?

dmsmidt’s picture

@nod_ #231, ah my bad about the announce message. Now I understand, if you explicitly send an empty string the announcement will be skipped. Drpal's third point makes sense: check if it is a string as well. If option.announce is FALSE it would read the default message right? That feels strange.

nod_’s picture

Status: Needs work » Needs review
FileSize
29.29 KB

Reroll.

  1. Makes sense to me like that, we're checking it's an array before. To me NO || (YES && YES) is clearer than NO || (YES && NO).
  2. It's a selection method, if you send it trash it should return an empty array, like querySelectorAll or jQuery.
  3. Meh, don't care. most probably won't see this in the API anyway. If someone should check it's data it's Drupal.announce.
  4. I think we should keep that, the message template changed.

( edit ) I guess false should be taken care of too. No strong feelings either way.

Status: Needs review » Needs work

The last submitted patch, 235: core-js-status-messages-77245-234.patch, failed testing.

drpal’s picture

@nod_

Good points on the JS. I fixed the out of date tests.

alexpott’s picture

  1. +++ b/core/themes/bartik/templates/status-messages.html.twig
    @@ -23,7 +23,7 @@
       {% if message_list is not empty %}
         {{ attach_library('bartik/messages') }}
    -    <div class="messages__wrapper layout-container">
    +    <div class="messages__wrapper layout-container" data-drupal-messages>
    

    This isn't going to work if there are no messages on the page.

  2. +++ b/core/themes/classy/templates/misc/status-messages.html.twig
    @@ -22,6 +22,7 @@
     {{ attach_library('classy/messages') }}
    +<div data-drupal-messages>
    

    I think rather than adding this is .html.twig files we should consider adding it in \Drupal\Core\Render\Element\StatusMessages::renderMessages() that way we can ensure that there is something for javascript to find to add messages regardless of what someone does in the .html.twig files for custom themes etc.

    Doing this will also mean that this change is compatible with #2853509: Don't render status messages if there are no messages but also include their assets if there might be

nod_’s picture

Status: Needs review » Needs work

doesn't apply and #238 needs addressing

pk188’s picture

Status: Needs work » Needs review
FileSize
4.84 KB

Status: Needs review » Needs work

The last submitted patch, 240: 77245-240.patch, failed testing.

pk188’s picture

Status: Needs work » Needs review
FileSize
4.79 KB

Status: Needs review » Needs work

The last submitted patch, 242: 77245-242.patch, failed testing.

droplet’s picture

should rewrite into ES6 Class.

some basic things:

  1. +++ b/core/misc/message.js
    @@ -0,0 +1,253 @@
    +  Drupal.message = function (messageWrapper) {
    +    // If an argument is passed, verify that it's a DOM Node of some kind.
    +    if (arguments.length === 1) {
    +      if (!messageWrapper || messageWrapper.nodeType !== 1) {
    +        throw new Error(Drupal.t('Drupal.message() expect an HTMLElement as parameter.'));
    +      }
    +    }
    

    do not pass in selectors?

    It doesn't work and do not take the options if I read the source code correctly.
    methods: add / remove / select / clear should apply to same messageWrapper
    To imagine one page has 2 messages area

  2. +++ b/core/misc/message.js
    @@ -0,0 +1,253 @@
    +      options.index = type + ' ' + (options.index || Math.random().toFixed(15).replace('0.', ''));
    ...
    +      return options.index;
    

    - a bit odd to assign index back to option;
    - and index should be ID I think;
    - Imagine you adding 10 messages and remove 2 randomly. which one is index 2?
    - To consider using [data-drupal-message-id="adopt-backend-pre-set-message"]