472143 - invalidate accessible when aria state changes affect exposed role

Status: NEW

(Core :: Disability Access APIs, defect)

Description

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

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.

Comment 1

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

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.

Comment 2

[Aaron Leventhal]

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.

Comment 3

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

I'll give it a shot.

Comment 4

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

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?

Comment 5

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

Attachment: fix

I tested it on the uiuc example mentioned in bug 473355 and it works like a charm.

Comment 6

[Aaron Leventhal]

Comment on attachment 357412 [details] [diff] [review]
fix

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

BUTTON + HASPOPUP
BUTTON + PRESSED

Comment 7

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

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.

Comment 8

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

Attachment: WIP - for discussion

Comment 9

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

Attachment: WIP - for discussion

Uploaded the old patch again last time by mistake.

Comment 10

[Aaron Leventhal]

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.

Comment 11

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

(In reply to comment #10)
> (From update of attachment 357463 [details] [diff] [review])

Let's talk on IRC :)

Comment 12

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

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.

Comment 13

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

Alexander, what are your thoughts here? (no rush)

Comment 14

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

The current consensus is that roles should not be changed once computed.

Comment 15

[alexander :surkov (:asurkov)]

(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.

Comment 16

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

Do we still want to do this? (I think we should just close it)

Comment 17

[alexander :surkov (:asurkov)]

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.

Comment 18

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

The worry is stuff like comment 10.