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

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

71.04 KB, patch
Aaron Leventhal
: review+
MarcoZ
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

10 years ago
Blocks: 389800
(Assignee)

Comment 1

10 years ago
Created attachment 342557 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #342557 - Flags: review?(aaronleventhal)
(Assignee)

Updated

10 years ago
Attachment #342557 - Flags: review?(marco.zehe)
(Assignee)

Updated

10 years ago
Blocks: 459353

Comment 2

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

Comment 3

10 years ago
(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

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

Comment 5

10 years ago
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 6

10 years ago
Comment on attachment 342557 [details] [diff] [review]
patch

Sorry, that was meant to be r+, not r-.
Attachment #342557 - Flags: review- → review+

Comment 7

10 years ago
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 8

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

Comment 9

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

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

9 years ago
Depends on: 489944
You need to log in before you can comment on or make changes to this bug.