565029 - Create an accessible name for a xul:toolbar from the @toolbarname attribute

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Attachment: Patch to add support for the toolbarname attribute

This is used to name a toolbar. All toolbars in the Firefox UI have one except the "tabs tool bar". Screen readers such as NVDA will then be able to identify at least most of the toolbars without us having to use WAI-ARIA on them to name them.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 444635 [details] [diff] [review]
Patch to add support for the toolbarname attribute

r=me with nits:
After name.CompressWhitespace() you could check name.IsEmpty() and do:
return nsAccessible::GetNameInternal(aName);
otherwise continue.
Can you confirm we have a toolbar test that has an aria derived name (perhaps aria-label). If we don't have one, please add one.

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to comment #1)
> (From update of attachment 444635 [details] [diff] [review])
> r=me with nits:
> After name.CompressWhitespace() you could check name.IsEmpty() and do:
> return nsAccessible::GetNameInternal(aName);

What exactly would you want this for? The toolbar element has no other attributes one could derive a name from.

> Can you confirm we have a toolbar test that has an aria derived name (perhaps
> aria-label). If we don't have one, please add one.

Added it, and this works without a fall-back in the actual method, since doing the name calc from ARIA is done beforehand in nsAccessible::GetName. So when both @aria-label and @toolbarname are present, @aria-label wins.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 444635 [details] [diff] [review]
Patch to add support for the toolbarname attribute


>+nsresult
>+nsXULToolbarAccessible::GetNameInternal(nsAString& aName)
>+{
>+  nsCOMPtr<nsIContent> content = nsCoreUtils::GetRoleContent(mDOMNode);
>+  if (!content)
>+    return NS_OK;

it's not needed because GetName() called isDefunct() already

>+  nsAutoString name;
>+  if (content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::toolbarname,
>+                       name)) {
>+    name.CompressWhitespace();
>+    aName = name;

if the reason of local string variable having is CompressWhitespace then we need to file bug to allow CompressWhitespace called on nsAString, because we need to avoid double copy of strings.

>+      // toolbar
>+      accTree = {
>+        role: ROLE_TOOLBAR,
>+        name: "My toolbar",
>+        children: [ ]
>+      };
>+
>+      testAccessibleTree("toolbar", accTree);

if you like to keep the test here then it makes sense to add toolbarbuttons or any other content used inside an toolbar to make accessible tree testing, otherwise it makes sense to keep name test in test_name.xul.

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to comment #3)
> (From update of attachment 444635 [details] [diff] [review])
> 
> >+nsresult
> >+nsXULToolbarAccessible::GetNameInternal(nsAString& aName)
> >+{
> >+  nsCOMPtr<nsIContent> content = nsCoreUtils::GetRoleContent(mDOMNode);
> >+  if (!content)
> >+    return NS_OK;
> 
> it's not needed because GetName() called isDefunct() already

OK.

> 
> >+  nsAutoString name;
> >+  if (content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::toolbarname,
> >+                       name)) {
> >+    name.CompressWhitespace();
> >+    aName = name;
> 
> if the reason of local string variable having is CompressWhitespace then we
> need to file bug to allow CompressWhitespace called on nsAString, because we
> need to avoid double copy of strings.

I basically took this from nsAccessible::GetName, which uses the same mechanism. There is already a comment there about this double copy.

> >+      // toolbar
> >+      accTree = {
> >+        role: ROLE_TOOLBAR,
> >+        name: "My toolbar",
> >+        children: [ ]
> >+      };
> >+
> >+      testAccessibleTree("toolbar", accTree);
> if you like to keep the test here then it makes sense to add toolbarbuttons or
> any other content used inside an toolbar to make accessible tree testing,
> otherwise it makes sense to keep name test in test_name.xul.

I'll add a test for a toolbar button. Thanks!

Comment 5

[Marco Zehe (:MarcoZ)]

Landed on 1.9.3 with nits addressed: - http://hg.mozilla.org/mozilla-central/rev/7470a477f42f