Closed Bug 1801234 Opened 1 year ago Closed 1 year ago

[CTW] Forward relation not updated when the target is removed/changed without changing the relation attribute

Categories

(Core :: Disability Access APIs, defect)

defect

Tracking

()

RESOLVED FIXED
110 Branch
Tracking Status
firefox110 --- fixed

People

(Reporter: Jamie, Assigned: morgan)

References

Details

(Whiteboard: [ctw-m4])

Attachments

(1 file, 1 obsolete file)

STR:

  1. 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>
  2. Get the labelledBy relation of the input RemoteAccessible.
    • Observe: The relation correctly points at the div.
  3. Press the Change button.
  4. 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:

  1. 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.
  2. 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.

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

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

  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.

Assignee: nobody → mreschenberg

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

  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?

Flags: needinfo?(jteh)

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

Flags: needinfo?(jteh)

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.

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

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

ahhh okay yes that makes sense :) thank you!

Attachment #9304981 - Attachment is obsolete: true

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

Whiteboard: [ctw-m4]
Depends on: 1803814
Attachment #9304983 - Attachment description: WIP: Bug 1801234: Queue a relations cache update on dependent accs when DOM ID mutations are observed r?Jamie → Bug 1801234: Queue a relations cache update on dependent accs when DOM ID mutations are observed r?Jamie
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

Backed out for causing assertion failures on DocAccessibleParent.cpp

Backout link

Push with failures

Failure log

Flags: needinfo?(mreschenberg)

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:

  1. We queue a cache update during BindToDocument, which happens during insertion.
  2. 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.

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

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

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

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

Flags: needinfo?(mreschenberg) → needinfo?(jteh)

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

Flags: needinfo?(jteh)

All of that said, you can equally argue that the foundation should be solid and prevent footguns, so... 🤷‍♂️

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
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 110 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: