Open Bug 472143 Opened 12 years ago Updated 2 years ago

invalidate accessible when aria state changes affect exposed role

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

People

(Reporter: davidb, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(1 file, 2 obsolete files)

This is a spin-off of bug 452388. See related comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=452388#c11 and surrounding
discussion for background.
Blocks: 452388
We need to do an InvalidateCacheSubtree for certain aria state changes but I think we are going to need to know the old state so I'm adding bug 467144 as a dependancy.
Depends on: 467144
Blocks: aria
I think this is what I think we can fix this without a fix to bug 467144. It wouldn't be as nice of a fix, but was can just do an invalidation when there might be a role change. I don't think that's a big problem if we do it that way.

Also, this is probably the fix for bug 473355.
I'll give it a shot.
Assignee: nobody → david.bolter
Status: NEW → ASSIGNED
Assignee: david.bolter → nobody
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
Flags: wanted1.9.2?
Just coming back to this. I'm looking at nsDocAccessible::ARIAAttributeChanged where we check for aria_checked and aria_pressed, which seems a good place to consider possible role changes.

Just to be clear Aaron are you thinking we would always invalidate the subtree here?
Attached patch fix (obsolete) — Splinter Review
I tested it on the uiuc example mentioned in bug 473355 and it works like a charm.
Assignee: nobody → david.bolter
Attachment #357412 - Flags: review?(aaronleventhal)
Comment on attachment 357412 [details] [diff] [review]
fix

We need to do it for these combinations and no others:

BUTTON + HASPOPUP
BUTTON + PRESSED
Attachment #357412 - Flags: review?(aaronleventhal) → review-
OK, I was thinking of something like role='menuitem' aria-checked='true' mapping to atk role ROLE_RADIO_MENU_ITEM...  do you think that is too edge casey? In any event, I agree we should check role.
Assignee: david.bolter → nobody
Assignee: nobody → david.bolter
Attached patch WIP - for discussion (obsolete) — Splinter Review
Attachment #357412 - Attachment is obsolete: true
Uploaded the old patch again last time by mistake.
Attachment #357462 - Attachment is obsolete: true
Attachment #357463 - Flags: review-
Comment on attachment 357463 [details] [diff] [review]
WIP - for discussion

+    nsAutoString role;
+    aContent->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::role, role);

You would actually want Role(accessible), because aria-pressed and aria-haspopup will affect an HTML button's role as well. (BTW, if you did want the ARIA role you have to be cognizant that the role attribute can have multiple roles. You'd need to use GetRoleMapEntry() if you were only interested in testing ARIA roles).

Worse, I made a mistake. If we invalidate the accessible every time pressed changed from true to false or vice-versa, we would mess up toggle buttons. So, it does appear that for toggle buttons we actually need to store whether the accessible for it already was a toggle button or not.

There are 2 ways we can deal with that. One is to only worry about the case for the currently focused button, because that is the real when the button will actually change its pressed state, and we're worried about invalidating too much and confusing the screen reader.

Another way of dealing with it probably needs to be explained on IRC. But here goes: every nsAccessible has an mRoleMapEntry. If something has a toggle button role we could modify mRoleMapEntry to indicate that, and then if aria-pressed changes to something not undefined, then we just need to check if the role map entry for that accessible says it was already a toggle button or not.
(In reply to comment #10)
> (From update of attachment 357463 [details] [diff] [review])

Let's talk on IRC :)
I spoke with Aaron on IRC, and updating the nsAccessible's cached mRoleMapEntry to include some smarts about attributes sounds promising (and substantive). Potentially moving logic from GetFinalRole into where we get the current role map entry.
Alexander, what are your thoughts here? (no rush)
The current consensus is that roles should not be changed once computed.
(In reply to comment #13)
> Alexander, what are your thoughts here? (no rush)

change role and recreate an accessible when necessary, follow comment #10. Not urgent though.
Do we still want to do this? (I think we should just close it)
Flags: needinfo?(surkov.alexander)
I think Windows AT wants show/hide events for an accessible if its role is changed. So if ARIA state change affects on role then the bug looks reasonable.
Flags: needinfo?(surkov.alexander)
The worry is stuff like comment 10.
Assignee: dbolter → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.