commit c67896dd6201b7bed70a412d2146929c2ddc9d49
parent 9cb7f8116308f12554ff18d16ed49c510f863aff
Author: Eitan Isaacson <eitan@monotonous.org>
Date: Tue, 30 Sep 2025 17:19:31 +0000
Bug 1987945 - P2: Cache relations using multiple attributes for same relation type. r=Jamie
This allows us to calculate relations remotely when more than one
relation source is at play. For example, a details relationship will be
determined in order of priority: aria-details, commandfor, popover.
Knowing the relation source remotely allows further filtering. For
example, a popover relation should not be valid if the target is a
sibling of the button. We should determine that remotely instead of
recalculating and sending a new relation cache on each tree mutation
that might change the relation.
It also allows us to provide something like `details-from` in the
accessible attribute to let ATs know what the source of the relationship
is and for it to determine presentation.
The one sticking point in this change is that determining reverse
relations is less straightforward and requires us to interrogate each
target candidate to see if the relationship is mutual. This is because
relationship sets can be fully or partially "eclipsed" by a higher
priority source on the potential target.
For example:
<button id="toggle" commandfor="popover" command="toggle-popover" aria-details="details">toggle1</button>
<button id="show" commandfor="popover" command="show-popover">show</button>
<dialog id="popover" popover>popover</dialog>
<div id="details">details</div>
The cached stored reverse relationship for "commandfor" on "popover" is
both "toggle" and "show", but the direct details relationship of "toggle" is "details".
So the calculated DETAILS_FOR of "popover" should just be "show".
Differential Revision: https://phabricator.services.mozilla.com/D264636
Diffstat:
4 files changed, 83 insertions(+), 27 deletions(-)
diff --git a/accessible/base/CacheConstants.h b/accessible/base/CacheConstants.h
@@ -87,6 +87,10 @@ static constexpr RelationData kRelationTypeAtoms[] = {
RelationType::FLOWS_FROM},
{nsGkAtoms::aria_details, nullptr, RelationType::DETAILS,
RelationType::DETAILS_FOR},
+ {nsGkAtoms::commandfor, nullptr, RelationType::DETAILS,
+ RelationType::DETAILS_FOR},
+ {nsGkAtoms::popovertarget, nullptr, RelationType::DETAILS,
+ RelationType::DETAILS_FOR},
{nsGkAtoms::aria_errormessage, nullptr, RelationType::ERRORMSG,
RelationType::ERRORMSG_FOR},
};
diff --git a/accessible/generic/LocalAccessible.cpp b/accessible/generic/LocalAccessible.cpp
@@ -4239,12 +4239,22 @@ already_AddRefed<AccAttributes> LocalAccessible::BundleFieldsForCache(
dom::HTMLLabelElement::FromNode(mContent)) {
rel.AppendTarget(mDoc, labelEl->GetControl());
}
- } else if (data.mType == RelationType::DETAILS ||
- data.mType == RelationType::CONTROLLER_FOR) {
- // We need to use RelationByType for details because it might include
- // popovertarget. Nothing exposes an implicit reverse details
- // relation, so using RelationByType here is fine.
- //
+ } else if (data.mType == RelationType::DETAILS) {
+ if (relAtom == nsGkAtoms::aria_details) {
+ rel.AppendIter(
+ new AssociatedElementsIterator(mDoc, mContent, relAtom));
+ } else if (relAtom == nsGkAtoms::commandfor) {
+ if (LocalAccessible* target = GetCommandForDetailsRelation()) {
+ rel.AppendTarget(target);
+ }
+ } else if (relAtom == nsGkAtoms::popovertarget) {
+ if (LocalAccessible* target = GetPopoverTargetDetailsRelation()) {
+ rel.AppendTarget(target);
+ }
+ } else {
+ MOZ_ASSERT_UNREACHABLE("Unknown details relAtom");
+ }
+ } else if (data.mType == RelationType::CONTROLLER_FOR) {
// We need to use RelationByType for controls because it might include
// failed aria-owned relocations or it may be an output element.
// Nothing exposes an implicit reverse controls relation, so using
diff --git a/accessible/ipc/DocAccessibleParent.h b/accessible/ipc/DocAccessibleParent.h
@@ -287,8 +287,8 @@ class DocAccessibleParent : public RemoteAccessible,
// Tracks cached reverse relations (ie. those not set explicitly by an
// attribute like aria-labelledby) for accessibles in this doc. This map is of
- // the form: {accID, {relationType, [targetAccID, targetAccID, ...]}}
- nsTHashMap<uint64_t, nsTHashMap<RelationType, nsTArray<uint64_t>>>
+ // the form: {accID, {pointerToRelationDataAddress, [targetAccID, ...]}}
+ nsTHashMap<uint64_t, nsTHashMap<const RelationData*, nsTArray<uint64_t>>>
mReverseRelations;
// Computed from the viewport cache, the accs referenced by these ids
diff --git a/accessible/ipc/RemoteAccessible.cpp b/accessible/ipc/RemoteAccessible.cpp
@@ -1193,25 +1193,68 @@ Relation RemoteAccessible::RelationByType(RelationType aType) const {
return rel;
}
- for (const auto& data : kRelationTypeAtoms) {
- if (data.mType != aType ||
- (data.mValidTag && TagName() != data.mValidTag)) {
- continue;
- }
+ auto GetDirectRelationFromCache =
+ [](const RemoteAccessible* aAcc,
+ RelationType aRelType) -> Maybe<const nsTArray<uint64_t>&> {
+ for (const auto& data : kRelationTypeAtoms) {
+ if (data.mType != aRelType ||
+ (data.mValidTag && aAcc->TagName() != data.mValidTag)) {
+ continue;
+ }
- if (auto maybeIds =
- mCachedFields->GetAttribute<nsTArray<uint64_t>>(data.mAtom)) {
- rel.AppendIter(new RemoteAccIterator(*maybeIds, Document()));
+ if (auto maybeIds = aAcc->mCachedFields->GetAttribute<nsTArray<uint64_t>>(
+ data.mAtom)) {
+ // Relations can have several cached attributes in order of precedence,
+ // if one is found we use it.
+ return maybeIds;
+ }
}
- // Each relation type has only one relevant cached attribute,
- // so break after we've handled the attr for this type,
- // even if we didn't find any targets.
- break;
+
+ return Nothing();
+ };
+
+ if (auto maybeIds = GetDirectRelationFromCache(this, aType)) {
+ rel.AppendIter(new RemoteAccIterator(*maybeIds, Document()));
}
if (auto accRelMapEntry = mDoc->mReverseRelations.Lookup(ID())) {
- if (auto reverseIdsEntry = accRelMapEntry.Data().Lookup(aType)) {
- rel.AppendIter(new RemoteAccIterator(reverseIdsEntry.Data(), Document()));
+ // A list of candidate IDs that might have a relation of type aType
+ // pointing to us. We keep a list so we don't evaluate or add the same
+ // target more than once.
+ nsTArray<uint64_t> relationCandidateIds;
+ for (const auto& data : kRelationTypeAtoms) {
+ if (data.mReverseType != aType) {
+ continue;
+ }
+
+ auto reverseIdsEntry = accRelMapEntry.Data().Lookup(&data);
+ if (!reverseIdsEntry) {
+ continue;
+ }
+
+ for (auto id : *reverseIdsEntry) {
+ if (relationCandidateIds.Contains(id)) {
+ // If multiple attributes point to the same target, only
+ // include it once. Our assumption is that there is a 1:1
+ // relationship between relation and reverse relation *types*.
+ continue;
+ }
+ relationCandidateIds.AppendElement(id);
+
+ RemoteAccessible* relatedAcc = mDoc->GetAccessible(id);
+ if (!relatedAcc) {
+ continue;
+ }
+
+ if (auto maybeIds =
+ GetDirectRelationFromCache(relatedAcc, data.mType)) {
+ if (maybeIds->Contains(ID())) {
+ // The candidate target has the forward relation type pointing
+ // to us, so we can add it as a target.
+ rel.AppendTarget(relatedAcc);
+ }
+ }
+ }
}
}
@@ -1346,7 +1389,7 @@ nsTArray<bool> RemoteAccessible::PreProcessRelations(AccAttributes* aFields) {
// the following assert, we don't have parity on implicit/explicit
// rels and something is wrong.
nsTArray<uint64_t>& reverseRelIDs =
- reverseRels->LookupOrInsert(data.mReverseType);
+ reverseRels->LookupOrInsert(&data);
// There might be other reverse relations stored for this acc, so
// remove our ID instead of deleting the array entirely.
DebugOnly<bool> removed = reverseRelIDs.RemoveElement(ID());
@@ -1379,9 +1422,8 @@ void RemoteAccessible::PostProcessRelations(const nsTArray<bool>& aToUpdate) {
const nsTArray<uint64_t>& newIDs =
*mCachedFields->GetAttribute<nsTArray<uint64_t>>(data.mAtom);
for (uint64_t id : newIDs) {
- nsTHashMap<RelationType, nsTArray<uint64_t>>& relations =
- Document()->mReverseRelations.LookupOrInsert(id);
- nsTArray<uint64_t>& ids = relations.LookupOrInsert(data.mReverseType);
+ auto& relations = Document()->mReverseRelations.LookupOrInsert(id);
+ nsTArray<uint64_t>& ids = relations.LookupOrInsert(&data);
ids.AppendElement(ID());
}
}
@@ -1395,7 +1437,7 @@ void RemoteAccessible::PruneRelationsOnShutdown() {
}
for (auto const& data : kRelationTypeAtoms) {
// Fetch the list of targets for this reverse relation
- auto reverseTargetList = reverseRels->Lookup(data.mReverseType);
+ auto reverseTargetList = reverseRels->Lookup(&data);
if (!reverseTargetList) {
continue;
}
