473164 - Need more cleaning of object attributes list

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

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

Spin-off from bug 452388#c72.

This bug is about the code in nsAccessible::GetAttributes where we expose all straggler ARIA properties as object attributes.

Token based aria attributes with value "undefined" should not be exposed in this list and we might have other similar special casing down the road. We'll need to deal with these token based attributes one by one unfortunately. Role extensibility, as Aaron points out will impact this later.

Comment 1

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

I'm going to hold off on this fix as I think it is has a low value to performance ratio.

Comment 2

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

Thinking out loud:

It might be nice capture the static properties of aria attributes (such as what value they can take). I wonder if we could make use of a map or hash based on the aria atoms, that would return us an object of default properties.

So we could do something like:

if (ourHash(nsAccessibilityAtoms::aria_foo).token) {
 // treat it like a token based aria attribute
 ...
}

Might be overkill now... but I could see this growing in usage. If you guys agree I'll explore it as part of this fix.

Comment 3

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

Chatted with surkov over IRC, and have his endorsement. Filed bug 475006 which I might tackle first after some investigation.

Comment 4

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

Attachment: WIP - for discussion

Here's the straightforward approach. We could go with this for now and refactor with the bug 475006 work.

Ideally it would be nice if we could easily perf test.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 358417 [details] [diff] [review]
WIP - for discussion


>       if (!nsAccUtils::IsARIAPropForObjectAttr(attrAtom))
>         continue; // No need to expose obj attribute -- will be exposed some other way
>       nsAutoString value;
>+      if (nsAccUtils::IsTokenBasedARIAAttribute(content, attrAtom) &&

I don't like the idea to check attributes twice (IsARIAPropForObjectAttr and IsTokenBasedARIAAttribute). It's better to combine IsARIAPropForObjectAttr, IsTokenBasedARIAAttribute, GetAttr into one method or follow general solution you suggested.

Comment 6

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

(In reply to comment #5)
I do think they are distinct characteristics and separating the two checks make some sense. I think my vote would be to go with this patch for correctness now, and then proceed with the general approach, or just go straight to the general approach.

Self-nit: I don't need the nsIContent param. Will update if I post another patch here.

Comment 7

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

Attachment: Removed a param.

Comment 8

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

OK I've done some research.

As I confirmed with Marco, our current plan is to commit this patch. I'll proceed to the more general refactor on bug 475006 as a more general fix. I imagine I'll want to take more time with that one, while working on other bugs.

Comment 9

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

Fixed via work on bug 475006