824785 - Add screen reader support for the social toolbar badges

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

From bug 811835 comment #16,

> Is this badge text accessible anywhere for screen readers? From what I read,
> many screen readers don't see generated content because it doesn't exist in
> the markup.
> 
> Also, I don't see any ARIA/a11y code here to notify users of changes.  It
> seems like ARIA live regions or the XUL equivalent should be used for the
> badges. I think the labels were readable in the previous implementation so
> this may cause a regression. I'm fine with a11y of badges being fixed in a
> follow-up where you can get more input from others.

I think we can use an aria-label on the toolbarbutton with the name of the button with the badge number following in parentheses.

I didn't include this with the patch for bug 811835 because it may introduce new user-facing strings that will block uplifting.

Comment 1

[Marco Zehe (:MarcoZ)]

This sounds about right for the buttons. The changes would have to be either the full button or a notification text much like the push notification on a smartphone, where it says "new message from <whoever>", and the badge increasing without any extra speech. I think such a notification should be more vocal than just saying a changed number.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Add an aria-label with the button label, optionally followed by the "badge" in parens.

(In reply to Marco Zehe (:MarcoZ) from comment #1)
> This sounds about right for the buttons. The changes would have to be either
> the full button or a notification text much like the push notification on a
> smartphone, where it says "new message from <whoever>", and the badge
> increasing without any extra speech.

Unfortunately we can't do something smart like "new message from <whoever>" as we don't know either that the new thing is a "message", nor do we know who "whoever" is.  All we have is the label for the button which is supplied by the provider, and in the case of Facebook, it doesn't change as new notifications come in.

Facebook currently provides the labels as "See All Notifications", "See All Messages" and "See All Friend Requests".  So I think the best we can do is what Jared suggested - appending the badge number in parentheses to the label.  Please let me know if I'm on the wrong track here...

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 710530 [details] [diff] [review]
Add an aria-label with the button label, optionally followed by the "badge" in parens.

f=me, this is what it should look like. Thanks!

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 710530 [details] [diff] [review]
Add an aria-label with the button label, optionally followed by the "badge" in parens.

Review of attachment 710530 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +909,5 @@
>  
>        let badge = icon.counter || "";
>        if (toolbarButton.getAttribute("badge") != badge)
>          toolbarButton.setAttribute("badge", badge);
> +      let ariaLabel = icon.label;

Icon labels are not required, so please use:
let ariaLabel = icon.label || "";

@@ +911,5 @@
>        if (toolbarButton.getAttribute("badge") != badge)
>          toolbarButton.setAttribute("badge", badge);
> +      let ariaLabel = icon.label;
> +      if (badge)
> +        ariaLabel += " (" + badge + ")";

This string should be added to the http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#374 file with the other social strings. It would need to be something like:
# LOCALIZATION NOTE: %1$S is the label for the toolbar button, %2$S is the associated badge numbering that the social provider may provide.
ariaToolbarButtonBadgeText=%1$S (%2$S)

If the badge is 0/null/undefined, then we don't need to use the .properties string because we can just set ariaLabel to icon.label.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: feedback comments addressed

As requested, but I used a string ID of "social.aria.toolbarButtonBadgeText" as it seemed more consistent with the existing strings.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: Back to not special-casing .label being null and now check the current attribute value before setting it.

As discussed on IRC, if icon.label is null other bad things will happen - but that's true for a number of other attributes too.  FWIW, nothing *too* bad happens - just "strange" values (eg, the label might say "undefined" or "null".

So this patch goes back to using icon.label unconditionally and also checks if the new attribute value is the same as the existing one before setting it.  I opened bug 838964 to track the validation of the data (at which point we will make .label a mandatory field)

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 711158 [details] [diff] [review]
Back to not special-casing .label being null and now check the current attribute value before setting it.

>+      if (toolbarButton.getAttribute("aria-label") != ariaLabel)

This check isn't useful.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 711158 [details] [diff] [review]
> >+      if (toolbarButton.getAttribute("aria-label") != ariaLabel)
> 
> This check isn't useful.

Heh - yeah, that's what I thought, but then saw just a few lines above the new code:

      if (toolbarButton.getAttribute("badge") != badge)
        toolbarButton.setAttribute("badge", badge);

so thought I'd try avoid a request to add the check ;)  So shall I also remove that other one?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 711158 [details] [diff] [review]
Back to not special-casing .label being null and now check the current attribute value before setting it.

Review of attachment 711158 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +914,5 @@
> +      // if there is a badge value, we must use a localizable string to insert it.
> +      if (badge)
> +        ariaLabel = gNavigatorBundle.getFormattedString("social.aria.toolbarButtonBadgeText",
> +                                                        [ariaLabel, badge]);
> +      if (toolbarButton.getAttribute("aria-label") != ariaLabel)

Yeah, you can remove this check and the one below.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8c6eb40240f

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Attachment: As landed

Removed the redundant attribute checks.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b8c6eb40240f

Comment 13

[u279076]

Marco, can you confirm that this is fixed in Firefox 21.0b6?

Comment 14

[Marco Zehe (:MarcoZ)]

Sorry for getting back so late, Anthony! I currently don't have a Facebook account, so can't verify it right away. However, a way to verify this would be
a) Install DOM Inspector.
b) Make badges come up.
c) Run DOMi and switch to accessibility view.
d) Drill down and look for the toolbar buttons that represent the badges.
If the AccessibleName contains the number of unread items, it's definitely working as it should.