453591 - reorganize nsAccessible::GetName to handle ARIA for non XUL/HTML elements

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Now nsAccessible::GetName handles XUL and HTML elements only by getXULName() and getHMTLName(). There we handle ARIA usage. So that getXULName and getHTMLName dublicates the code, also nsXFormsAccessible::GetName() has the same code. We need move ARIA code into "cross XML language" code.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Aaron Leventhal]

Comment on attachment 342557 [details] [diff] [review]
patch

I don't think we can organize it exactly like this, because aria-label can affect the name of a node used in an accessible subtree computation (where aria-labelledby cannot, because it can't be recursive).

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> (From update of attachment 342557 [details] [diff] [review])
> I don't think we can organize it exactly like this, because aria-label can
> affect the name of a node used in an accessible subtree computation (where
> aria-labelledby cannot, because it can't be recursive).

Main idea of the patch is to separate ARIA name from native name. It allows to to avoid code duplications and give us confidence accessible name for all elements takes into account ARIA name. It makes sense because I can see in code lot of places where ARIA doesn't play role in name computation.

So regarding your question. I will use GetNameInternal() only if I fall into recursion, otherwise I will use GetName(). So it means if I'm in recursion then I tried aria-label already and I can skip it. Sounds correct?

Comment 4

[Aaron Leventhal]

If you are in recursion aria-label still matters, but aria-labelledby doesn't

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 342557 [details] [diff] [review]
patch

All tests pass, and this change in nsIAccessible_name.js is fine! r=me.

Comment 6

[Marco Zehe (:MarcoZ)]

Comment on attachment 342557 [details] [diff] [review]
patch

Sorry, that was meant to be r+, not r-.

Comment 7

[Aaron Leventhal]

Comment on attachment 342557 [details] [diff] [review]
patch

rs=aaronlev, we really need #Name_Computation rewrite from the ARIA implementor's guide.

Comment 8

[Marco Zehe (:MarcoZ)]

Comment on attachment 342557 [details] [diff] [review]
patch

>+   * Retruns the accessible name specified by ARIA.

Nit: "returns"...

>+nsresult
>+nsHTMLButtonAccessible::GetNameInternal(nsAString& aName)
[...]
>+  GetHTMLName(name, PR_FALSE);
> 
>   if (name.IsEmpty()) {
>     // no label from HTML or ARIA
>+    nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>     if (!content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::value,
>                           name) &&
>         !content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::alt,
>                           name)) {

No null check here before using content?

>+NS_IMETHODIMP
>+nsHTMLImageAccessible::GetName(nsAString& aName)

As discussed on IRC; please file follow-up bug to clean this one up so it prefers ARIA over anything else.

>+nsresult
>+nsHTMLSelectOptionAccessible::GetNameInternal(nsAString& aName)
[...]
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+  content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::label, aName);

Same question about null check.

> class nsHTMLListBulletAccessible : public nsLeafAccessible
[...]
>   // nsIAccessible
>+  NS_IMETHOD GetRole(PRUint32 *aRole);
>   NS_IMETHOD GetName(nsAString& aName);
>-  NS_IMETHOD GetRole(PRUint32 *aRole);

Nit: Why this change? I'd prefer they stay in alphabetical order.

>+  // Override nsXFromsAccessible::GetName() to prevent name calculation by
>+  // XForms rules.

Nit: "Override nsXFormsAccessible ..."

>+nsresult
>+nsXULMenuitemAccessible::GetNameInternal(nsAString& aName)
[...]
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>+  content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::label, aName);

Another null check question...

Same for the following:
>+nsresult
>+nsXULTextAccessible::GetNameInternal(nsAString& aName)

and

>+nsresult
>+nsXULLinkAccessible::GetNameInternal(nsAString& aName)

> NS_IMETHODIMP
> nsXULTreeitemAccessible::GetName(nsAString& aName)
> {
>+  // XXX: we should take into account ARIA usage for content tree. 

Please don't forget to file follow-up bug.

Comment 9

[alexander :surkov (:asurkov)]

checked in with MarcoZ comments http://hg.mozilla.org/mozilla-central/rev/5eb98035a8a0

I'll run through the code and will file following up bugs