Closed Bug 1774043 Opened 2 years ago Closed 2 years ago

Cache LABELLED_BY relation

Categories

(Core :: Disability Access APIs, task)

task

Tracking

()

RESOLVED FIXED
105 Branch
Tracking Status
firefox105 --- fixed

People

(Reporter: morgan, Assigned: morgan)

References

Details

(Whiteboard: [ctw-m2])

Attachments

(4 files, 2 obsolete files)

This is the relation Windows depends on the most, so we should cache this first :)

Last CtW meeting we discussed caching one of the label for/labelled by pair and computing the reciprocal relation in the parent process.
I thought about this some more today and am confused about how we handle the "attribute removed" case.
Say, for example, we have the following:

<div id="d1">hello</div>
<button aria-labelledby="d1"></button>

Let's also assume we're caching the labelled by relation and computing the label for relation.
We'll update our cache with a DeleteEntry() for labelled by on the button, but how do we know (in parent) that we need to remove the div's label for relation? Outside of DOMAttributeChanged I don't think we have access to the old value of aria-labelledby...

:Jamie do you have thoughts on this? I don't really see a way forward here, apart from caching both relations independently (then, when we see aria-labelledby removed in DOMAttributeChanged, we'd use the old value to somehow fetch the acc(s) that used to act as labels, and push DeleteEntry()s for their label for relations too. I imagine there's a hash-map-y way around this, but I'm just not seeing it right now :(

Also: I think this problem also exists if we end up caching the label for relation instead, if that node gets destroyed, or its ID changes, we still need to be able to update the reciprocal relation.

Flags: needinfo?(jteh)

Hmm. Tricky.

My initial thinking was that we have the old value in mCachedFields, so we can just use that to compare against. Unfortunately, the cache update logic is isolated to AccAttributes::Update, which doesn't (and shouldn't) know anything about RemoteAccessibles, relations, etc.

Moving the update code into RemoteAccessibleBase::ApplyCache would make this easier, but it's also horrible and will probably require yucky messing with making other AccAttributes things public or friend classes or the like. On the flip side, it would avoid extra hash lookups.

Another alternative could be to have AccAttributes::Update take a lambda which takes the key being updated/removed, giving the caller the chance to do some pre-processing if that key is present. If this lambda sees labelledBy, it would remove the labelFor data for the target Accessibles for the pre-update labelledBy (i.e. the labelledBy still in mCachedFields). It would also set a flag so it knows to update labelFor on the new targets.

A final option could be to check for the presence of labelledBy in aFields in RemoteAccessibleBase::ApplyCache before we call mCachedFields->Update. If present (either an update or a delete), we remove the labelFor data for the target Accessibles for the pre-update labelledBy (i.e. the labelledBy still in mCachedFields). We then call mCachedFields->Update. After that, we update the labelFor on the new labelledBy targets. This does involve additional hash lookups for every cache update, though. I'm not sure if that is premature optimisation or not.

Flags: needinfo?(jteh)
Attachment #9285569 - Attachment is obsolete: true
Attachment #9285568 - Attachment is obsolete: true
Whiteboard: [ctw-m2]
Attachment #9285570 - Attachment description: WIP: Bug 1774043: An entire mess of relations stuff that needs to be split up r?Jamie → WIP: Bug 1774043: Add tests for relations caching r?Jamie
Attachment #9285924 - Attachment description: WIP: Bug 1774043: Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r?Jamie → Bug 1774043: Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r?Jamie
Attachment #9285925 - Attachment description: WIP: Bug 1774043: Add implicit relations map, pre- and post-update map processing in ApplyCache r?Jamie → Bug 1774043: Add implicit relations map, pre- and post-update map processing in ApplyCache r?Jamie
Attachment #9285926 - Attachment description: WIP: Bug 1774043: Make remote ::Relations and ::RelationByType functions to use cache r?Jamie → Bug 1774043: Make remote ::Relations and ::RelationByType functions to use cache r?Jamie
Attachment #9285570 - Attachment description: WIP: Bug 1774043: Add tests for relations caching r?Jamie → Bug 1774043: Add tests for relations caching r?Jamie
Attachment #9285924 - Attachment description: Bug 1774043: Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r?Jamie → Bug 1774043: [Part 1] Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r?Jamie
Attachment #9285924 - Attachment description: Bug 1774043: [Part 1] Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r?Jamie → WIP: Bug 1774043: [Part 1] Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r?Jamie
Attachment #9285925 - Attachment description: Bug 1774043: Add implicit relations map, pre- and post-update map processing in ApplyCache r?Jamie → WIP: Bug 1774043: [Part 2] Add implicit relations map, pre- and post-update map processing in ApplyCache r?Jamie
Attachment #9285926 - Attachment description: Bug 1774043: Make remote ::Relations and ::RelationByType functions to use cache r?Jamie → WIP: Bug 1774043: [Part 3] Make remote ::Relations and ::RelationByType functions to use cache r?Jamie
Attachment #9285570 - Attachment description: Bug 1774043: Add tests for relations caching r?Jamie → WIP: Bug 1774043: [Part 4] Add tests for relations caching r?Jamie
Attachment #9285924 - Attachment description: WIP: Bug 1774043: [Part 1] Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r?Jamie → Bug 1774043: [Part 1] Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r?Jamie
Attachment #9285925 - Attachment description: WIP: Bug 1774043: [Part 2] Add implicit relations map, pre- and post-update map processing in ApplyCache r?Jamie → Bug 1774043: [Part 2] Add implicit relations map, pre- and post-update map processing in ApplyCache r?Jamie
Attachment #9285926 - Attachment description: WIP: Bug 1774043: [Part 3] Make remote ::Relations and ::RelationByType functions to use cache r?Jamie → Bug 1774043: [Part 3] Make remote ::Relations and ::RelationByType functions to use cache r?Jamie
Attachment #9285570 - Attachment description: WIP: Bug 1774043: [Part 4] Add tests for relations caching r?Jamie → Bug 1774043: [Part 4] Add tests for relations caching r?Jamie
Blocks: 1780878
Pushed by mreschenberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9309568a622d
[Part 1] Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r=Jamie
https://hg.mozilla.org/integration/autoland/rev/8a7e2374e0b2
[Part 2] Add implicit relations map, pre- and post-update map processing in ApplyCache r=Jamie
https://hg.mozilla.org/integration/autoland/rev/14e4b5310a9a
[Part 3]  Make remote ::Relations and ::RelationByType functions to use cache r=Jamie
https://hg.mozilla.org/integration/autoland/rev/c0cdd3e2efa2
[Part 4] Add tests for relations caching r=Jamie
Blocks: 1781536
Attachment #9285570 - Attachment description: Bug 1774043: [Part 4] Add tests for relations caching r?Jamie → WIP: Bug 1774043: [Part 4] Add tests for relations caching r?Jamie
Attachment #9285570 - Attachment description: WIP: Bug 1774043: [Part 4] Add tests for relations caching r?Jamie → Bug 1774043: [Part 4] Add tests for relations caching r?Jamie
Pushed by mreschenberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffbb26d2a3c1
[Part 1] Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r=Jamie
https://hg.mozilla.org/integration/autoland/rev/5fff79016cb6
[Part 2] Add implicit relations map, pre- and post-update map processing in ApplyCache r=Jamie
https://hg.mozilla.org/integration/autoland/rev/f44a9d0f46ab
[Part 3]  Make remote ::Relations and ::RelationByType functions to use cache r=Jamie
https://hg.mozilla.org/integration/autoland/rev/63a4b92b5453
[Part 4] Add tests for relations caching r=Jamie
Regressions: 1782652

Backed out for causing gv-junit failures

Backout link

Push with failures

Failure log 1 // Failure log 2

Flags: needinfo?(mreschenberg)
Pushed by mreschenberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6de77a6f3d23
[Part 1] Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r=Jamie
https://hg.mozilla.org/integration/autoland/rev/570aa988c8c8
[Part 2] Add implicit relations map, pre- and post-update map processing in ApplyCache r=Jamie
https://hg.mozilla.org/integration/autoland/rev/dd760c44c8ba
[Part 3]  Make remote ::Relations and ::RelationByType functions to use cache r=Jamie
https://hg.mozilla.org/integration/autoland/rev/063973343a0e
[Part 4] Add tests for relations caching r=Jamie
Regressions: 1783549
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: