Problem/Motivation

Multiple sequential calls to Drupal.announce result in only the last call being read, because the text from previous calls is overwritten before the UA can read it.

Drupal.announce('This line will not be read.');
Drupal.announce('This line will not be read either.');
Drupal.announce('Only this line will be read.');

Proposed resolution

Near-simultaneous calls to Drupal.announce occur on page load or when a user interaction takes place. If we just collect the text of the messages within a slight delay, all of the text can be written to the ARIA-live region at once and get read.

Remaining tasks

Propose a patch.

User interface changes

None.

API changes

Drupal.announce() will be moved from drupal.js to its own file because it must depend on Drupal.debounce. drupal.js cannot have this dependency. So now modules must include the drupal.announce library if they want this feature.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jessebeach’s picture

Status: Active » Needs review
FileSize
8.28 KB

In order to solve this issue, I need to leverage Drupal.debounce. When Drupal.announce is called, it waits 200ms before writing to the ARIA Live region. If Drupal.announce is called again within that time, the new string is added to the list of strings to append to the ARIA Live region and Drupal.announce begins another 200ms wait. This allows the strings from multiple near-simultaneous calls to be collected and then written to the ARIA Live region at once.

I moved the Drupal.announce method to its own file and its own library entry so it can depend on Drupal.debounce without forcing drupal.js to also depend on debounce.

As I bonus, I wrote tests for this utility as well: http://drupalcode.org/project/fat.git/blob/HEAD:/tests/drupal.announce.t...

jessebeach’s picture

Removed a superfluous return statement.

This

Drupal.announce = function (text, priority) {
  // Save the text and priority into a closure variable. Multiple simultaneous
  // announcements will be concatenated and read in sequence.
  announcements.push({
    text: text,
    priority: priority
  });
  // Immediately invoke the function that debounce returns. 200 ms is right at
  // the cusp where humans notice a pause, so we will wait
  // at most this much time before the set of queued announcements is read.
  return (debounce(announce, 200)());
};

was changed to this

Drupal.announce = function (text, priority) {
  // Save the text and priority into a closure variable. Multiple simultaneous
  // announcements will be concatenated and read in sequence.
  announcements.push({
    text: text,
    priority: priority
  });
  // Immediately invoke the function that debounce returns. 200 ms is right at
  // the cusp where humans notice a pause, so we will wait
  // at most this much time before the set of queued announcements is read.
  (debounce(announce, 200)());
};

where the return statement has been removed. Drupal.announce didn't return anything before this change and there is no need to return anything now either.

Wim Leers’s picture

Issue tags: +sprint, +Spark
mgifford’s picture

Jesse loaded up a SimplyTest.me environment using http://drupal.org/project/fat & the latest patch this evening. It seemed to work quite well in VoiceOver with Safari. It should also be tested in JAWS, but it does work fine in ChromeVox too.

jessebeach’s picture

I'm unable to test with JAWS because of the prohibitive license fee ($895!!!). If we get this committed, it is more likely that someone who uses JAWS will be able to test and report issues.

Wim Leers’s picture

Status: Needs review » Needs work

I also tested this in detail; works perfectly in Chrome's ChromeVox.

I'm going to RTBC without JAWS being tested; Jesse really tried to test in JAWS (even asking for help on Twitter), to no avail, so it shouldn't be postponed from being committed any longer.


One nitpick left — I'll RTBC once it's fixed:

+++ b/core/misc/announce.jsundefined
@@ -0,0 +1,103 @@
+    // Create an array of announcement strings to be joined and appended to the
+    // aria live region.
+    //
+    // If any of the announcements has a priority of assertive, the entire
+    // group of announcements will have this priority.

This is a most unorthodox comment style :)

I think what the rest of Drupal core would do, is just get rid of the empty line in between.

Or: move the second comment into the loop, above the if-test?

Wim Leers’s picture

Assigned: Unassigned » jessebeach

Assigning to Jesse so she can do a quick reroll to fix #6, then this is RTBC.

jessebeach’s picture

Comment styling fixed.

jessebeach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -JavaScript, -Accessibility, -sprint, -Spark

The last submitted patch, drupal-announce-multiplestrings-1959306-8.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Accessibility, +sprint, +Spark
nod_’s picture

Status: Needs review » Needs work

The attach function is not safe to execute multiple time. Needs a if (liveElement) {} or something.

Needs a detach function cleaning up the dom element too.

    while (announcement = announcements.pop()) {

that raises a jshint error.

jessebeach’s picture

Needs a detach function cleaning up the dom element too.

What use case are you thinking of for the detach? If we take away the liveElement, we'll need to raise an error if Drupal.announce is subsequently called because the DOM element to write the strings to will no longer be present.

Wim Leers’s picture

Agreed with #13, this is one of those things were it is by definition a global, always-needed thing. This is kind of doing what browser should be providing to us in the first place.

nod_’s picture

Looking closely, the join('. '), looks like it'll be trouble down the line. Can't get excited when I talk to my blind users!?

Back on the questions: I figured it was a two line function for detaching that, didn't think it'd be controversial.

If it's "global, always-needed" I'd leave it in drupal.js and probably just bypass the whole attach behavior thing.

Then there is the issue of jQuery. Love it that it's a no jQuery thing. On the other hand the dependency on drupal.js "pollute" it. drupal.js will require jquery for the forseable future so it's counter-productive to avoid it.

If we're starting to output Drupal = {}; next to drupalSettings that's a different story though (uncoupling the existance of the Drupal object from the drupal.js file)

jessebeach’s picture

Status: Needs work » Needs review
FileSize
8.11 KB
1.9 KB

Looking closely, the join('. '), looks like it'll be trouble down the line. Can't get excited when I talk to my blind users!?

Great point. I've replaced it with a \n. Tests have been updated as well. Now you can be declarative, interrogative, emphatic!

announce.js now passes JSHint.

The attach function is not safe to execute multiple time. Needs a if (liveElement) {} or something.

That attach function is updated to check for the existence of the liveElement.

I punted on the detach issue. I don't think we need it.

nod_, I'm a little confused by your later comment:

Then there is the issue of jQuery. Love it that it's a no jQuery thing. On the other hand the dependency on drupal.js "pollute" it. drupal.js will require jquery for the forseable future so it's counter-productive to avoid it.

If we're starting to output Drupal = {}; next to drupalSettings that's a different story though (uncoupling the existance of the Drupal object from the drupal.js file)

.

The reason I pull announce() out of drupal.js was because announce.js depends on debounce.js and I didn't want drupal.js to depend on debouce.js as well. Do you see a good way to resolve this? Your comment suggests that you do. Is it something we could resolve in a followup?

nod_’s picture

follow up is fine, I'll open it tomorrow, it need some discussions.

Thanks for the changes. all good for me now :)

Status: Needs review » Needs work
Issue tags: -JavaScript, -Accessibility, -sprint, -Spark

The last submitted patch, drupal-announce-multiplestrings-1959306-16.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Accessibility, +sprint, +Spark
nod_’s picture

jessebeach’s picture

If #1913086: Generalize the overlay tabbing management into a utility library gets committed, then I need to make a small adjustment to this patch to include drupal.announce as a dependency of Contextual and Overlay. Otherwise the JS for these two modules won't load.

webchick’s picture

Status: Needs review » Needs work

That was just committed, so this will need a re-roll.

jessebeach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
985 bytes
9.36 KB
247.43 KB

The image below illustrates the error I was expecting. Since Drupal.announce was moved to its own file, Contextual and Overlay need to declare it as a dependency in their libraries. I've added those two lines in this reroll (see diff).

A screenshot of a Drupal site with the Overlay open. The developer console is open. In the log message area, an error message indicates that the Drupal object does not have a method named announce.

FAT tests all pass.

nod_ says he's good with the patch in #17.
Wim Leers has been advocating RTBC since #7.
I'm setting this to RTBC on the grounds that this patch passes tests (back and front end) and has been code-vetted as well mgifford.

Wim Leers’s picture

RTBC +1 :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I got a kick-ass demo of this the other day, happy to see this in core. I think this is a huge accessibility improvement, and something unique among open source projects!

Committed and pushed to 8.x. Thanks!

mgifford’s picture

This is really amazing. Having centralized alerts & tab ordering (1913086) in core is going to really help improve the experience for screen reader users & keyboard only users. Really happy to see this as part of core now!

Wim Leers’s picture

Now let's leverage it everywhere :)

Wim Leers’s picture

Issue tags: -sprint
falcon03’s picture

YAY, this is really amazing! I'm not sure if any other CMS has a feature similar to this utility!

BTW, have the alerts in toolbar module been already converted to leverage Drupal.announce()? If not, let me know: I can open a follow up! :D

Great work, guys! :-)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

mgifford’s picture

Issue tags: +aria

Wanted to add the aria tag, but also put in a reminder to put in a specific link to the video from DrupalCon that is discussing this:
https://www.youtube.com/user/DrupalAssociation/videos

EDIT: here is the link -> http://www.youtube.com/watch?v=MbVCDRvTgQc

mgifford’s picture

Issue tags: -aria

Started a handbook roughly here - http://drupal.org/node/2004876

Also worth noting this module http://drupal.org/project/devel_a11y

mgifford’s picture

Issue tags: +aria

Not sure how the aria tag got nixed.

mgifford’s picture

Issue summary: View changes
Issue tags: +aria-live