[CTW] Forward relation not updated when the target is removed/changed without changing the relation attribute
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox110 | --- | fixed |
People
(Reporter: Jamie, Assigned: morgan)
References
Details
(Whiteboard: [ctw-m4])
Attachments
(1 file, 1 obsolete file)
STR:
- Open this test case:
data:text/html,<div id="label">before</div><input id="input" aria-labelledby="label"><button onclick="label.remove(); label = document.createElement('div'); label.id = 'label'; label.textContent = 'after'; document.body.insertBefore(label, input);">Change</button>
- Get the labelledBy relation of the input RemoteAccessible.
- Observe: The relation correctly points at the div.
- Press the Change button.
- Get the labelledBy relation of the input RemoteAccessible.
- Expected: The relation should point at the new div.
- Actual: The labelledBy relation has no targets.
There are two problems here:
- In the cache, I'm fairly sure there will still be a labelledBy relation, but it will be pointing at the old (now non-existent) Accessible. This is okay for a short while because we'll just ignore it. However, if that Accessible id gets reused (which is entirely possible, albeit hard to reproduce reliably), the relation will now return an incorrect target.
- When the DOM id of a target gets associated with a new node without updating the relation attribute (e.g. aria-labelledby), we aren't updating the relation.
Reporter | ||
Comment 1•1 year ago
|
||
(In reply to James Teh [:Jamie] from comment #0)
- In the cache, I'm fairly sure there will still be a labelledBy relation, but it will be pointing at the old (now non-existent) Accessible.
I guess we'd need to solve this by updating the forward relations when the target RemoteAccessible shuts down, contrary to this comment. Alternatively, I guess we could make that comment true by pushing a cache update for a relation provider when a LocalAccessible shuts down, similar to the solution below. I think reducing redundant cache updates is a good thing to do if we can, though, so I'd prefer the former solution here if it's feasible.
- When the DOM id of a target gets associated with a new node without updating the relation attribute (e.g. aria-labelledby), we aren't updating the relation.
We could do this both when we bind a new LocalAccessible to its document and when the id attribute changes. We have a data structure which is keyed by id and has data for each attribute and element that depends on that id; see DocAccessible::GetRelProviders. We could use that to queue relation cache updates for any associated relation provider.
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 2•1 year ago
•
|
||
(In reply to James Teh [:Jamie] from comment #1)
- When the DOM id of a target gets associated with a new node without updating the relation attribute (e.g. aria-labelledby), we aren't updating the relation.
We could do this both when we bind a new LocalAccessible to its document and when the id attribute changes. We have a data structure which is keyed by id and has data for each attribute and element that depends on that id; see DocAccessible::GetRelProviders. We could use that to queue relation cache updates for any associated relation provider.
I understand the update we'd need to do in BindToDocument
but, separately, I'm a little confused at what we do when we observe an ID change in DOMAttributeChanged. Why don't we update mDependentIDsHashes
for things other than aria-owns?
Like, if we have:
data:text/html,<label id="label" for="d">hi</label><button id="d"></button>
and then do something like:
document.getElementById("label").htmlFor = "x";
document.getElementById("d").setAttribute("id", "x");
when do we update label's dependent ID has from "d" to "x"? or do we not need to?
Reporter | ||
Comment 3•1 year ago
|
||
(In reply to Morgan Reschenberg [:morgan] from comment #2)
I'm a little confused at what we do when we observe an ID change in DOMAttributeChanged. Why don't we update
mDependentIDsHashes
for things other than aria-owns?
Which aria-owns code path are you referring to?
We update mDependentIDsHashes for every attribute change, adding mappings here and removing mappings here. Add/RemoveDependentIDsFor determine whether the attribute is relevant or not by looking in kRelationAttrs.
Reporter | ||
Comment 4•1 year ago
•
|
||
I originally misinterpreted your question and started unintentionally mansplaining how these maps work, which I now realise you've already figured out. 😳 Nevertheless, since I already wrote it out (and since it took me a while to understand when I was looking into this), I thought I'd include it here for anyone's future reference or in case it clarifies anything.
The key is to understand that mDependentIDsHashes is a reverse map, not a forward map. For forward mapping, we can just query the attribute on the DOM element directly; e.g. we can ask the <label>
what it's for
attribute is.
For example:
data:text/html,<label id="label" for="d">hi</label><button id="d"></button>
mDependentIDsHashes will have something like this:
{
theDocumentNode: { // This part is because ids are scoped to shadow roots.
"d": [
["for", theLabelElement]
]
}
}
To get the label's LABEL_FOR relation, we just directly query the for
DOM attribute on the label. To get the LABELLED_BY relation on the button, we get the button's DOM id, then look it up in mDependentIDsHashes to get the label.
Assignee | ||
Comment 5•1 year ago
|
||
(In reply to James Teh [:Jamie] from comment #4)
I originally misinterpreted your question and started unintentionally mansplaining how these maps work, which I now realise you've already figured out. 😳 Nevertheless, since I already wrote it out (and since it took me a while to understand when I was looking into this), I thought I'd include it here for anyone's future reference or in case it clarifies anything.
The key is to understand that mDependentIDsHashes is a reverse map, not a forward map. For forward mapping, we can just query the attribute on the DOM element directly; e.g. we can ask the
<label>
what it'sfor
attribute is.For example:
data:text/html,<label id="label" for="d">hi</label><button id="d"></button>
mDependentIDsHashes will have something like this:
{ theDocumentNode: { // This part is because ids are scoped to shadow roots. "label": [ ["for", theLabelElement] ] } }
To get the label's LABEL_FOR relation, we just directly query the
for
DOM attribute on the label. To get the LABELLED_BY relation on the button, we get the button's DOM id, then look it up in mDependentIDsHashes to get the label.
hmm, does that mean in your example above mDependentIDsHashes would have an entry like this to make looking up the "by" relationship quick? Also, is "theLabelElement" "d" or something else?
"d" : [
["by", label]
]
I don't quite get how it's a reverse map, it seems like it's maintaining info about the "for" attribute that, like you said, could be queried from the DOM element (label) itself.
Reporter | ||
Comment 6•1 year ago
|
||
Ug. I messed up. I've edited the comment, but the id should have been "d", not "label". theLabelElement refers to the <label>
node. So, it's a reverse map in that we're saying "the id d
has a for
attribute pointing at it from label
".
Assignee | ||
Comment 7•1 year ago
|
||
ahhh okay yes that makes sense :) thank you!
Assignee | ||
Comment 8•1 year ago
|
||
Assignee | ||
Comment 9•1 year ago
|
||
Updated•1 year ago
|
Reporter | ||
Comment 10•1 year ago
|
||
Ug. I messed up. I've edited the comment, but the id should have been "d", not "label". theLabelElement refers to the <label>
node. So, it's a reverse map in that we're saying "the id d
has a for attribute pointing at it from label".
Updated•1 year ago
|
Comment 11•1 year ago
|
||
Pushed by mreschenberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/51527201739c Queue a relations cache update on dependent accs when DOM ID mutations are observed r=Jamie
Comment 12•1 year ago
|
||
Backed out for causing assertion failures on DocAccessibleParent.cpp
Reporter | ||
Comment 13•1 year ago
|
||
Morgan noted in the team meeting:
Ran into a crash in DocAccessibleParent, where we’re trying to process a cache update for an acc that isn’t yet in the document. I don’t think this is due to my relations patch stack, it’s probably a timing issue ? FWIW we push two cache updates for this accessible, one that fails because the remote can’t be found, and one immediately after that succeeds. Probably need to verify the acc is in the doc somewhere in ProcessQueuedCacheUpdates
This means that we queued a cache update for an Accessible before we sent it to the parent process. That really shouldn't happen. There also isn't really a way to check whether we've sent an Accessible to parent yet.
Normally, this can't happen because a cache update shouldn't be queued for an Accessible between the time it is inserted into the tree and the time we call ProcessQueuedCacheUpdates, since both of these things happen synchronously in WillRefresh. However, in the case of QueueCacheUpdateForDependentRelations, there are two violations of this invariant that didn't occur to me during code review:
- We queue a cache update during BindToDocument, which happens during insertion.
- It queues cache updates for other Accessibles. We have no idea whether those Accessibles have been sent to the parent process yet. They might have been, but they might not.
Reporter | ||
Comment 14•1 year ago
|
||
(In reply to James Teh [:Jamie] from comment #13)
- We queue a cache update during BindToDocument, which happens during insertion.
While that is normally a violation of the invariant, it probably doesn't matter here because we aren't queuing an update for the Accessible itself.
- It queues cache updates for other Accessibles. We have no idea whether those Accessibles have been sent to the parent process yet. They might have been, but they might not.
This is the real problem here.
There also isn't really a way to check whether we've sent an Accessible to parent yet.
Well, actually, there kinda is, even if it wasn't intended to be used this way. In order to track moved Accessibles, we have mInsertedAccessibles and mMovedAccessibles. We can use mInsertedAccessibles to check whether we've fully finished handling an insertion yet. If we haven't, we shouldn't queue a cache update for that Accessible; it will get handled as part of its initial cache update when we send it.
As an optimisation, if the tree is still being constructed (DoInitialUpdate, HasLoadState(eTreeConstructed)), no Accessibles will have been sent yet, so we don't even need to check mInsertedAccessibles and we should not queue a cache update.
Assignee | ||
Comment 15•1 year ago
|
||
(In reply to James Teh [:Jamie] from comment #14)
(In reply to James Teh [:Jamie] from comment #13)
There also isn't really a way to check whether we've sent an Accessible to parent yet.
Well, actually, there kinda is, even if it wasn't intended to be used this way. In order to track moved Accessibles, we have mInsertedAccessibles and mMovedAccessibles. We can use mInsertedAccessibles to check whether we've fully finished handling an insertion yet. If we haven't, we shouldn't queue a cache update for that Accessible; it will get handled as part of its initial cache update when we send it.
As an optimisation, if the tree is still being constructed (DoInitialUpdate, HasLoadState(eTreeConstructed)), no Accessibles will have been sent yet, so we don't even need to check mInsertedAccessibles and we should not queue a cache update.
Do you think it makes more sense to check this before calling QueueCacheUpdate, or when we're in ProcessQueuedCacheUpdates? I was thinking the latter, but your message seems like you're thinking of the former.
Reporter | ||
Comment 16•1 year ago
|
||
(In reply to Morgan Reschenberg [:morgan] from comment #15)
Do you think it makes more sense to check this before calling QueueCacheUpdate, or when we're in ProcessQueuedCacheUpdates? I was thinking the latter, but your message seems like you're thinking of the former.
Checking it in ProcessQueuedCacheUpdates will solve this problem more broadly. On the other hand, in most cases, we really shouldn't ever queue a cache update between an insertion and ProcessQueuedCacheUpdates. It isn't exactly harmful, but I (perhaps over-cautiously) am concerned that in cases we don't know about, it might indicate a deeper problem with ordering, and we'd never know about that problem if we did it in ProcessQueuedCacheUpdates. Relations are an exception (which we now understand) because we queue for a related Accessible instead of the Accessible itself.
Really, either is fine, but I'm leaning very slightly towards doing it specifically for relations so as not to hide a potential future problem. It's not unreasonable to argue that I'm being paranoid, though, so I'd accept either change.
Reporter | ||
Comment 17•1 year ago
|
||
All of that said, you can equally argue that the foundation should be solid and prevent footguns, so... 🤷♂️
Assignee | ||
Comment 18•1 year ago
|
||
Comment 19•1 year ago
|
||
Pushed by mreschenberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e4b01a07a85d Queue a relations cache update on dependent accs when DOM ID mutations are observed r=Jamie
Comment 20•1 year ago
|
||
bugherder |
Description
•