Problem/Motivation

Provide consistent container for messages that JavaScript would be able to utilize. Current use case is to display AJAX errors to fix #1232416: Drupal alerts "An AJAX HTTP request terminated abnormally" during normal site operation, confusing site visitors/editors.

Proposed resolution

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

Remaining tasks

Manual testing

User interface changes

No changes, API addition.
API is very simple, messages can only be added to the message div. Nothing provided to hide individual messages or empty the message area.

API changes

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

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: 
CommentFileSizeAuthor
#112 js-1.png18.24 KBdroplet
#111 interdiff.txt2.53 KBgoogletorp
#111 a_place_for_javascript-77245-111.patch10.55 KBgoogletorp
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 ]
#108 interdiff.txt1.71 KBgoogletorp
#108 a_place_for_javascript-77245-108.patch9.83 KBgoogletorp
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,777 pass(es).
[ View ]
#105 a_place_for_javascript-77245-105.patch10.44 KBgoogletorp
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,775 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#94 interdiff.txt2.12 KBnod_
#93 core-js-status-messages-77245-93.patch9.74 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 64,426 pass(es).
[ View ]

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

StatusFileSize
new2.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

StatusFileSize
new1.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
StatusFileSize
new1.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
StatusFileSize
new3.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

StatusFileSize
new7.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
StatusFileSize
new8.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

StatusFileSize
new8.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
StatusFileSize
new6.21 KB
Test request sent.
[ 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
StatusFileSize
new8.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

StatusFileSize
new7.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
StatusFileSize
new7.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

StatusFileSize
new7.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
StatusFileSize
new7.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
StatusFileSize
new8.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
StatusFileSize
new6.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
StatusFileSize
new8.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
StatusFileSize
new8.21 KB
PASSED: [[SimpleTest]]: [MySQL] 63,087 pass(es).
[ View ]
new415 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
StatusFileSize
new1.75 KB
new8.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
StatusFileSize
new1.45 KB
new8.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
StatusFileSize
new8.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

StatusFileSize
new9.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

StatusFileSize
new2.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
StatusFileSize
new10.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
StatusFileSize
new9.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,777 pass(es).
[ View ]
new1.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
StatusFileSize
new10.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 ]
new2.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
StatusFileSize
new18.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?