If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add screen reader support for the social toolbar badges

RESOLVED FIXED in Firefox 21

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jaws, Assigned: markh)

Tracking

({access})

Trunk
Firefox 21
access
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Blocks: 759414

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 710530 [details] [diff] [review]
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...
Attachment #710530 - Flags: feedback?(marco.zehe)
Attachment #710530 - Flags: feedback?(jaws)

Comment 3

5 years ago
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!
Attachment #710530 - Flags: feedback?(jaws) → feedback+
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.
Attachment #710530 - Flags: feedback?(marco.zehe) → feedback+
(Assignee)

Comment 5

5 years ago
Created attachment 711103 [details] [diff] [review]
feedback comments addressed

As requested, but I used a string ID of "social.aria.toolbarButtonBadgeText" as it seemed more consistent with the existing strings.
Assignee: nobody → mhammond
Attachment #710530 - Attachment is obsolete: true
Attachment #711103 - Flags: review?(jaws)
(Assignee)

Comment 6

5 years ago
Created attachment 711158 [details] [diff] [review]
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)
Attachment #711103 - Attachment is obsolete: true
Attachment #711103 - Flags: review?(jaws)
Attachment #711158 - Flags: review?(jaws)
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.
(Assignee)

Comment 8

5 years ago
(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 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.
Attachment #711158 - Flags: review?(jaws) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8c6eb40240f
Flags: in-testsuite+
(Assignee)

Comment 11

5 years ago
Created attachment 711585 [details] [diff] [review]
As landed

Removed the redundant attribute checks.
Attachment #711158 - Attachment is obsolete: true
Attachment #711585 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b8c6eb40240f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Marco, can you confirm that this is fixed in Firefox 21.0b6?
QA Contact: marco.zehe
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.
You need to log in before you can comment on or make changes to this bug.