1774043 - Cache LABELLED_BY relation

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task)

Description

[Morgan Reschenberg [:morgan]]

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

Comment 1

[Morgan Reschenberg [:morgan]]

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.

Comment 2

[James Teh [:Jamie]]

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.

Comment 3

[Morgan Reschenberg [:morgan]]

Attachment: WIP: Bug 1774043: Create base methods for ::Relations and ::RelationByType caching r?Jamie

Comment 4

[Morgan Reschenberg [:morgan]]

Attachment: WIP: Bug 1774043: Add bundling logic for labelled_by relation r?Jamie

Depends on D151881

Comment 5

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1774043: [Part 4] Add tests for relations caching r?Jamie

Comment 6

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1774043: [Part 1] Add cache domains, caching logic for LABEL_FOR and LABELLED_BY relations r?Jamie

Comment 7

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1774043: [Part 2] Add implicit relations map, pre- and post-update map processing in ApplyCache r?Jamie

Depends on D152100

Comment 8

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1774043: [Part 3] Make remote ::Relations and ::RelationByType functions to use cache r?Jamie

Depends on D152101

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Atila Butkovits]

Backed out for causing build bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/4eeee5a6274709c9aad68cc34c03631da9c57d2e

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=34a9e10a9f87047b4c7e6e33c43d0852204b569f&selectedTaskRun=edvH2LllTWmAiiQ9UN1TSA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=385502594&repo=autoland&lineNumber=9976

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Norisz Fay [:noriszfay]]

Backed out for causing gv-junit failures

Backout link

Push with failures

Failure log 1 // Failure log 2

Comment 13

[Morgan Reschenberg [:morgan]]

clean try: https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=WZhEYKPGSJOfmZQLBoQvcA.0&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel%2Crunning%2Cpending%2Crunnable&revision=d552ef44da1dc65906e5d440d7743e2d0c6d13a9
(one failure is unrelated, known intermittent)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6de77a6f3d23
https://hg.mozilla.org/mozilla-central/rev/570aa988c8c8
https://hg.mozilla.org/mozilla-central/rev/dd760c44c8ba
https://hg.mozilla.org/mozilla-central/rev/063973343a0e