Make tabledrag warning message show when row weights are enabled, and add WAI-ARIA live region

Subscribe, this would be awesome.

This sounds like a great contribution.

Lets do this consistently in core, sounds like Everett outlined nicely what a patch would need to contain.

Adding tag for WAI-ARIA.

Also, we need someone to roll the patch. Any takers?

Tagging... Also, tabledrag's moved to:

core/misc/tabledrag.js

@Bojhan do we have lists of places where there are interactive messages like in Drag/Drop?

I'd like to make this consistent, but not sure how many places this might be done in D8.

@Everett, I'm not sure this is what you were looking at but decided to start the ball rolling by simply adding role="alert" to the warning. The "Changed" note should also be explained I think but at the very least the text would need to change.

$.extend(Drupal.theme, {
  tableDragChangedMarker: function () {
    return '<abbr class="warning tabledrag-changed" title="' + Drupal.t('Changed') + '">*</abbr>';
  },
  tableDragIndentation: function () {
    return '<div class="indentation">&nbsp;</div>';
  },
  tableDragChangedWarning: function () {
    return '<div class="tabledrag-changed-warning messages warning" role="alert">' + Drupal.theme('tableDragChangedMarker') + ' ' + Drupal.t('Changes made in this table will not be saved until the form is submitted.') + '</div>';
  }
});

#12: aria2dragNdrop-1272990-12.patch queued for re-testing.

Just needs a review it seems.

This is a pretty simple patch to review.. Only included the addition of:
role="alert"

However, it might be better here to use aria-live="polite":
https://developer.mozilla.org/en-US/docs/Accessibility/ARIA/ARIA_Live_Re...

Either would be an improvement.

#12: aria2dragNdrop-1272990-12.patch queued for re-testing.

Note that the Edit module is already doing this! Thanks to jessebeach.

If this patch solves the acute issue, I say we commit it for the addition info it provides.

We've got custom implementations of aria-live in Edit and Toolbar. Frankly, any UI I touch going forward will get aria-live messaging because, well, that's what building a UI is all about - making the state of the application known.

After feature freeze I'd like to consolidate the Toolbar and Edit module area-live implementations into a common implementation under something like Drupal.inform. There's no reason to hold a quick fix for a bigger fix, though.

Committed to 8.x, thanks! Will push in a few mins.

Wonder if this is worth a backport to D7.

Realized there's a problem with the approach taken here for Safari & IE9.

Looking forward to @jessebeach's API #1915302: Provide an API for aria-live region update announcements that module developers may leverage which should provide a standardized way to do this.

Rough patch to use Drupal.announce function. We still need role="alert", but can't count on it to announce it to AT.

+++ b/core/misc/tabledrag.jsundefined
@@ -1244,7 +1244,9 @@ $.extend(Drupal.theme, {
+    return '<div class="tabledrag-changed-warning messages warning" role="alert">' + Drupal.theme('tableDragChangedMarker') + ' ' + changeWarning + '</div>';
+    Drupal.announce(changeWarning);

You're returning before Drupal.announce() is called; hence it won't be called :)

Right.. Thanks Wim. Juggling a few things today and missed that obvious one. Have to do local testing too.

Ok, I'm getting a bit further with this, but still getting:

Uncaught TypeError: Object #<Object> has no method 'announce' tabledrag.js:1248

I'm not sure why this isn't working Drupal.announce(changeWarning, 'assertive');

#26: Drupal.announce-function-dragNdrop-26.patch queued for re-testing.

Sounds like a JS lib dependency problem: it seems Drupal.announce is loaded after the calling code.

Drupal.announce is part of drupal.js, which is loaded before tabledrag.js, so that shouldn't cause any troubles. Maybe use window.Drupal.announce() instead? Or wrap tabledrag code like this?

(function ($, Drupal, drupalSettings) {
...
})(jQuery, Drupal, drupalSettings);

Well, we can try it.

Well, the patch works in SimplyTest.me, but not for VoiceOver based on my testing.

Drupal.announce(changeWarning, 'assertive');

I'm not seeing what I'd expect from: #1915302: Provide an API for aria-live region update announcements that module developers may leverage

In this case, I think you added code to the tabledrag.js tableDragChangedWarning method, but this method is overridden in blocks.js, which is probably why you weren't hearing the announcement. That's my guess in any case.

mgifford, do we need Drupal.announce here? Shouldn't the role="alert" attribute be sufficient to get the warning message read? That's what I did in this patch, with some cleanup of the strings. If we really do need Drupal.announce here, let me know and I'll update it or I'm glad to review an update if you want to do it.

It should have been enough, but in testing this in Safari with VoiceOver I realize I didn't see it. Searched found http://blog.paciellogroup.com/2012/06/html5-accessibility-chops-aria-rol...

Thought we could use the Drupal.announce approach here instead:
https://drupal.org/node/1272990#comment-7063974

It works as expected in ChromeVox. Just tested just now.

EDIT: Patch above doesn't seem to get VoiceOver to work in Safari either unfortunately.

It's a pity that Safari doesn't support role="alert"! Oh well, I'll put the Drupal.announce back in that you originally had.

mgifford, I made some changes to Drupal.announce to deal with the Overlay. VoiceOver and ChromeVox don't seem to be reading aria-live element changes that occur in nodes inside of iframes. So we always need to be manipulating the live element in the parent window.

Let me know if this patch works for you in Safari. The tabledrag.js warning is not quite working correctly yet. If you navigate from the Blocks page to a Content type field display page and alter the table, it will read the warning from the Blocks page. We need to be passing the text for the warning into the theme function, rather than simply including the text in the theme function.

That worked great in Safari/VoiceOver. Thanks for re-rolling the patch.

Where are we at with this patch? What else needs to happen before we let the bots at it.

mgifford, this is the one piece we need to address:

The tabledrag.js warning is not quite working correctly yet. If you navigate from the Blocks page to a Content type field display page and alter the table, it will read the warning from the Blocks page. We need to be passing the text for the warning into the theme function, rather than simply including the text in the theme function.

It "works" right now, but you navigate around the Overlay, you can end up getting the wrong message across pages that use the Drag-drop warnings. Whatever the first message you get is, that's the one you'll have for the rest of your overlay session.

Ok, thanks for the update. I was just trying to touch on some of the issues that have dropped a bit lower in the priority list over the past month.

#35: tabledrag-alert-1272990-35.patch queued for re-testing.

@Jesse, can re-roll this patch?

This is just a straight reroll on D8:60a1055049ac218322c09b9bd753289ac39a2259. I don't have time to fix the issue I mentioned in #39 right at the moment.

Want to see if #44 runs into problems with the bots.

Still have to deal with #39 as @jessebeach says.

Reroll with visually-hidden...

Go bot.

#47: tabledrag-alert-1272990-47.patch queued for re-testing.

47: tabledrag-alert-1272990-47.patch queued for re-testing.

just a re-roll.

Thanks for finding that @Jalandhar.

Patch in #54 still applies. Adding the manual testing tag.

Oh ya, this should be basically identical to the last one. Just a bit of fuzz:

patching file core/misc/tabledrag.js
Hunk #1 succeeded at 1277 with fuzz 1 (offset 16 lines).
patching file core/modules/block/js/block.js
Hunk #1 succeeded at 132 (offset 1 line).

We can drop the parent.window thing. No overlay anymore.

That makes it simpler. No need to modify core/misc/announce.js then, right?

+++ b/core/misc/tabledrag.js
@@ -1277,6 +1277,14 @@
+    // Announce the changes.

+++ b/core/modules/block/js/block.js
@@ -132,4 +132,16 @@
+  // Announce the changes.

redundant comments

Ok, I removed that line from core/modules/block/js/block.js

We should not replace the Drupal.theme.element globally.

Passing var in patch still not ideally way, but sadly there's limitation on Drupal.theme().

What does the bot think of this new patch?

I've tested this in ChromeVox & looked at the page source. Drupal.announce is being properly presented.

I think this is good to go.

+++ b/core/misc/tabledrag.js
@@ -779,7 +779,13 @@
+          if (!self.tableDragChangedWarningTheme) {
...
+          }

Should we just have a (documented) var on the tabledrag object which defaults to 'tableDragChangedWarning'

I'm unsure why we'd insert an element with warning text in a div with role=alert and also invoke Drupal.announce. I presume this would result in the message getting announced twice. But then, role=alert doesn't have great support. So maybe it's a valid hedge.

I think comment #69 is still an issue to look at?

So I tested the patch on both ChromeVox and Voiceover. In ChromeVox it works fine, in Voiceover I don't hear the message announced. Either way, I get the same results for the version with and without role=alert . I would suggest to remove the role=alert since the Drupal announce already reads the text.

OK, that sounds good to me.

Support for aria-live="assertive" varies between different screenreaders (and role="alert" is supposed to imply aria-live="assertive").

It sounds like we have run into that problem, and we can't fix it in Drupal. Instead, by avoiding role=alert we can get the Drupal.announce() message instead.

What else do we need to do before marking this RTBC? This sounds like the right direction to go.

Andrew, have you tested this code independently? If not, I guess that's the next step.

I dont fully understand why we are doing "Make tabledrag warning message show when row weights are enabled"

@Bojhan I'm confused by your response.

Does the issue need a clearer description?

We need to use Drupal.announce() to tell screen readers that this text has appeared on the page.

Yes, but it doesn't need to be displayed right?

I agree @Bojhan there should be no visible changes. Just WAI-ARIA.

Updating the title to better match the direction of the discussion and the fix, feel free to tweak further.

This now seems to come down to support for role="alert" vs using Drupal.announce. The role="alert" change landed in #20.
We are almost 10 years on from when this landed, is support for this still lacking, or are we okay now with relying on role="alert"?

I'm following up issues that were committed and then re-opened. In Bugsmash, we found that such issues tend to linger. Therefore I am restoring the title on this and making a followup with the new title (thank you lendude).

The followup issue is #3279596: Announce tabledrag warning message to Assistive tech.

Therefore restoring the fixed status on this issue.