Closed Bug 453591 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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.
Blocks: cleana11y
Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #342557 - Flags: review?(aaronleventhal)
Attachment #342557 - Flags: review?(marco.zehe)
Blocks: namea11y
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).
(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?
If you are in recursion aria-label still matters, but aria-labelledby doesn't
Comment on attachment 342557 [details] [diff] [review]
patch

All tests pass, and this change in nsIAccessible_name.js is fine! r=me.
Attachment #342557 - Flags: review?(marco.zehe) → review-
Comment on attachment 342557 [details] [diff] [review]
patch

Sorry, that was meant to be r+, not r-.
Attachment #342557 - Flags: review- → review+
Comment on attachment 342557 [details] [diff] [review]
patch

rs=aaronlev, we really need #Name_Computation rewrite from the ARIA implementor's guide.
Attachment #342557 - Flags: review?(aaronleventhal) → review+
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.
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 489944
You need to log in before you can comment on or make changes to this bug.