1760739 - Expose ARIA object attributes for cached RemoteAccessibles

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task)

Description

[James Teh [:Jamie]]

Some ARIA attributes (e.g. aria-current, aria-dropeffect, aria-grabbed, aria-haspopup) need to be exposed as object attributes via Accessible::Attributes. This is either because they aren't exposed via other means at all or are exposed via other means (such as State) but not all the info is provided (e.g. there is a HASPOPUP state but this doesn't specify dialog, menu, etc.). We need to cache and expose these.

This should be any attribute in base/ARIAMap.cpp's gWAIUnivAttrMap list without ATTR_BYPASSOBJ or ATTR_BYPASSOBJ_IF_FALSE. ATTR_BYPASSOBJ_IF_FALSE needs to be handled differently, though.

Comment 1

[James Teh [:Jamie]]

This also applies to aria-rowcount and aria-colcount. Those will also need code added in AccGroupInfo::TotalItemCount and RemoteAccessibleBase::GroupPosition.

Comment 2

[James Teh [:Jamie]]

We also need xml-roles.

Comment 3

[James Teh [:Jamie]]

Eitan, I've been looking into this and have a bunch of things I'd love your thoughts on.

1. Do you think we should send cache updates for ARIA attributes individually or just update them all when one or more of them changes during a tick? The latter is how we handle other cache domains, but it's also a bit wasteful when there are more than a couple of attributes per domain. The question is whether that really matters.
2. If we do update them as an entire domain, one challenge is how to handle DeleteEntry, since we won't know whether an attribute was removed.
3. We could get around 2) by giving ARIA its own nested AccAttributes inside mCachedFields, similar to what we do for text attributes. That's a tiny bit of extra overhead and is perhaps a bit ugly, but the advantage is that we can just replace the whole thing in one shot. If it's empty, we just send DeleteEntry for the ARIA key.
4. For LocalAccessible, we currently expose aria-foo attributes (where foo is arbitrary) to the client. The idea is to expose raw values for new ARIA attributes which aren't in the spec yet or aren't yet supported by Firefox. Do you think we should keep this behaviour for RemoteAccessible? Chrome doesn't.
5. On the remote side, if we store them all in mCachedFields, we either have to walk all the standard attributes and check mCachedFields for each, or walk mCachedFields and check each to see if it's a standard attribute. If we want to support 4), we have to walk mCachedFields and look for aria- prefixes. On the other hand, if we do a nested AccAttributes as per 3), we can just walk that without any checks.

Comment 4

[James Teh [:Jamie]]

I'm moving xml-roles into bug 1773930, as it needs to be handled quite differently.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to James Teh [:Jamie] from comment #3)

Eitan, I've been looking into this and have a bunch of things I'd love your thoughts on.

1. Do you think we should send cache updates for ARIA attributes individually or just update them all when one or more of them changes during a tick? The latter is how we handle other cache domains, but it's also a bit wasteful when there are more than a couple of attributes per domain. The question is whether that really matters.

I don't think it is too wasteful, maybe. I think your second point really makes this one moot.

2. If we do update them as an entire domain, one challenge is how to handle DeleteEntry, since we won't know whether an attribute was removed.

We can detect a removal, but we currently can't know of that removal in the SendCache method. We would need to add a lot of overhead to be able to do that. It would add a bunch of complexity and probably wouldn't be worth it because of your first point.

3. We could get around 2) by giving ARIA its own nested AccAttributes inside mCachedFields, similar to what we do for text attributes. That's a tiny bit of extra overhead and is perhaps a bit ugly, but the advantage is that we can just replace the whole thing in one shot. If it's empty, we just send DeleteEntry for the ARIA key.

That's not a bad idea.. so you would iterate over the element's attributes, anything with an aria- prefix (standard or not) you put in the nested AccAttributes. If there is nothing, you do DeleteEntry. On the remote end, don't update the nested AccAttributes recursively, just delete or replace it. I like that.

4. For LocalAccessible, we currently expose aria-foo attributes (where foo is arbitrary) to the client. The idea is to expose raw values for new ARIA attributes which aren't in the spec yet or aren't yet supported by Firefox. Do you think we should keep this behaviour for RemoteAccessible? Chrome doesn't.

With the solution above we can easily preserve that behavior.

5. On the remote side, if we store them all in mCachedFields, we either have to walk all the standard attributes and check mCachedFields for each, or walk mCachedFields and check each to see if it's a standard attribute. If we want to support 4), we have to walk mCachedFields and look for aria- prefixes. On the other hand, if we do a nested AccAttributes as per 3), we can just walk that without any checks.

Yup.

So to recap, you convinced me that an atomic nested AccAttributes is the way to go.

Comment 6

[James Teh [:Jamie]]

(In reply to Eitan Isaacson [:eeejay] from comment #5)

We can detect a removal, but we currently can't know of that removal in the SendCache method.

Right. If we were going to do this, I think we'd need to manually build the cache data for this case and not use LocalAccessible::SendCache at all. This would be completely outside the normal usage of CacheDomain, which is all kinds of yuck.

Comment 7

[James Teh [:Jamie]]

Attachment: Bug 1760739 part 1: Remove propagation of ARIA object attributes from iframes to documents.

This behaviour doesn't currently work for OOP iframes.
While we could make it work for OOP iframes using the cache, it isn't covered by the spec, nor does any other browser support it.

Comment 8

[James Teh [:Jamie]]

Attachment: Bug 1760739 part 2: Provide a way for ARIAMap's AttrIterator to expose values other than strings.

Previously, AttrIterator always exposed string values.
Soon, we'll be using this for caching, so we don't want to convert token values to strings.
We'll eventually need to expose numeric values as well.
To achieve this, an ExposeAttr method was added which takes an AccAttributes and sets the attribute on it using an appropriately typed value.

In addition, a small optimisation was made so we don't search the ARIA attributes map twice for each attribute we query.
The string version of AttrName was also removed, since it wasn't used.
Unfortunately, AttrValue can't be removed yet because our old UIA implementation depends on it, but that will have to be rewritten at some point in future.

Comment 9

[James Teh [:Jamie]]

Attachment: Bug 1760739 part 3: Expose ARIA object attributes for cached RemoteAccessibles.

Comment 10

[James Teh [:Jamie]]

Attachment: Bug 1760739 part 4: Normalize aria-row/colcount/index as integers.

Comment 11

[James Teh [:Jamie]]

Attachment: Bug 1760739 part 5: Add Accessible method to retrieve an integer ARIA attribute.

Comment 12

[James Teh [:Jamie]]

Attachment: Bug 1760739 part 6: Add unified GroupPosition and TotalItemCount support for aria-row/colcount/index.

While this removes most of the local-specific code, we can't remove ARIARowAccessible::GroupPosition yet because it also calculates row index/count compensating for intervening Accessibles between tables and rows.
Eventually, we should fix that properly in AccGroupInfo, at which point we can remove this.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/325140a26691
part 1: Remove propagation of ARIA object attributes from iframes to documents. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/f2726aaaac37
part 2: Provide a way for ARIAMap's AttrIterator to expose values other than strings. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/4685e0b04645
part 3: Expose ARIA object attributes for cached RemoteAccessibles. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/c3c223df49c4
part 4: Normalize aria-row/colcount/index as integers. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/f47725414205
part 5: Add Accessible method to retrieve an integer ARIA attribute. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/579b06a778ba
part 6: Add unified GroupPosition and TotalItemCount support for aria-row/colcount/index. r=eeejay

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/325140a26691
https://hg.mozilla.org/mozilla-central/rev/f2726aaaac37
https://hg.mozilla.org/mozilla-central/rev/4685e0b04645
https://hg.mozilla.org/mozilla-central/rev/c3c223df49c4
https://hg.mozilla.org/mozilla-central/rev/f47725414205
https://hg.mozilla.org/mozilla-central/rev/579b06a778ba