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.
Comment | File | Size | Author |
---|---|---|---|
#23 | Screenshot_4_19_13_2_33_PM.png | 247.43 KB | jessebeach |
#23 | drupal-announce-multiplestrings-1959306-23.patch | 9.36 KB | jessebeach |
#23 | interdiff_16-to-23.txt | 985 bytes | jessebeach |
#16 | interdiff_8-to-16.txt | 1.9 KB | jessebeach |
#16 | drupal-announce-multiplestrings-1959306-16.patch | 8.11 KB | jessebeach |
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedIn order to solve this issue, I need to leverage
Drupal.debounce
. WhenDrupal.announce
is called, it waits 200ms before writing to the ARIA Live region. IfDrupal.announce
is called again within that time, the new string is added to the list of strings to append to the ARIA Live region andDrupal.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 onDrupal.debounce
without forcingdrupal.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...
Comment #2
jessebeach CreditAttribution: jessebeach commentedRemoved a superfluous
return
statement.This
was changed to this
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.Comment #3
Wim LeersComment #4
mgiffordJesse 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.
Comment #5
jessebeach CreditAttribution: jessebeach commentedI'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.
Comment #6
Wim LeersI 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:
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?
Comment #7
Wim LeersAssigning to Jesse so she can do a quick reroll to fix #6, then this is RTBC.
Comment #8
jessebeach CreditAttribution: jessebeach commentedComment styling fixed.
Comment #9
jessebeach CreditAttribution: jessebeach commentedComment #11
Wim Leers#8: drupal-announce-multiplestrings-1959306-8.patch queued for re-testing.
Comment #12
nod_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.
that raises a jshint error.
Comment #13
jessebeach CreditAttribution: jessebeach commentedWhat 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.Comment #14
Wim LeersAgreed 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.
Comment #15
nod_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 todrupalSettings
that's a different story though (uncoupling the existance of the Drupal object from the drupal.js file)Comment #16
jessebeach CreditAttribution: jessebeach commentedGreat 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.
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:
.
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?
Comment #17
nod_follow up is fine, I'll open it tomorrow, it need some discussions.
Thanks for the changes. all good for me now :)
Comment #19
jessebeach CreditAttribution: jessebeach commented#16: drupal-announce-multiplestrings-1959306-16.patch queued for re-testing.
Comment #20
nod_Follow-up btw #1974580: Introduce domReady to remove jQuery dependency from drupal.js and clean it up.
Comment #21
jessebeach CreditAttribution: jessebeach commentedIf #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.Comment #22
webchickThat was just committed, so this will need a re-roll.
Comment #23
jessebeach CreditAttribution: jessebeach commentedThe 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).
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.
Comment #24
Wim LeersRTBC +1 :)
Comment #25
webchickI 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!
Comment #26
mgiffordThis 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!
Comment #27
Wim LeersNow let's leverage it everywhere :)
Comment #28
Wim LeersComment #29
falcon03 CreditAttribution: falcon03 commentedYAY, 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! :-)
Comment #31
mgiffordWanted 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
Comment #32
mgiffordStarted a handbook roughly here - http://drupal.org/node/2004876
Also worth noting this module http://drupal.org/project/devel_a11y
Comment #33
mgiffordNot sure how the aria tag got nixed.
Comment #34
mgifford