commit 3b389fdd62e93b624a2a01b240e30556c059f389
parent 5cc2a12756a9de2d4352f28ce12744dad66b15f0
Author: Alice Boxhall <95208+alice@users.noreply.github.com>
Date: Wed, 24 Dec 2025 13:39:25 +0000
Bug 1984685 - Refactor: consistently return Maybe<nsTArray<RefPtr<Element>>> when getting attr-associated elements, and key AttrRelProviders on nsAtom rather than nsString. r=dom-core,morgan,Jamie,smaug
The existing `Element::GetAttrAssociatedElements()` and its internal lambda (which the subsequent change promotes into the `GetAttrAssociatedElementsInternal()` method, in order to use it as the internal "source of truth" method) already used arrays of `RefPtrs`, since the former interacts with scripting. In the subsequent change, since the `GetAttrAssociatedElementsInternal()` method which I adapted from the original lambda is used both by the bindings-exposed `GetAttrAssociatedElementsForBindings()` and in `AssociatedElementsIterator()`, I thought it was more straightforward to adapt `AssociatedElementsIterator` to use the array of `RefPtrs` rather than unpack the raw pointers each time, but I'm happy to modify it if this will be too slow or memory-intensive.
Differential Revision: https://phabricator.services.mozilla.com/D262170
Diffstat:
12 files changed, 78 insertions(+), 75 deletions(-)
diff --git a/accessible/base/AccIterator.cpp b/accessible/base/AccIterator.cpp
@@ -80,9 +80,8 @@ RelatedAccIterator::RelatedAccIterator(DocAccessible* aDocument,
mProviders(nullptr),
mIndex(0),
mIsWalkingDependentElements(false) {
- nsAutoString id;
- if (aDependentContent->IsElement() &&
- aDependentContent->AsElement()->GetAttr(nsGkAtoms::id, id)) {
+ if (!aDependentContent->IsElement()) return;
+ if (nsAtom* id = aDependentContent->GetID()) {
mProviders = mDocument->GetRelProviders(aDependentContent->AsElement(), id);
}
}
@@ -254,8 +253,10 @@ AssociatedElementsIterator::AssociatedElementsIterator(DocAccessible* aDoc,
mContent->AsElement()->GetAttr(aIDRefsAttr, mIDs);
if (mIDs.IsEmpty() &&
(aria::AttrCharacteristicsFor(aIDRefsAttr) & ATTR_REFLECT_ELEMENTS)) {
- nsAccUtils::GetARIAElementsAttr(mContent->AsElement(), aIDRefsAttr,
- mElements);
+ if (auto elements = nsAccUtils::GetARIAElementsAttr(mContent->AsElement(),
+ aIDRefsAttr)) {
+ mElements.SwapElements(*elements);
+ }
}
}
}
diff --git a/accessible/base/AccIterator.h b/accessible/base/AccIterator.h
@@ -245,7 +245,7 @@ class AssociatedElementsIterator : public AccIterable {
nsIContent* mContent;
DocAccessible* mDoc;
nsAString::index_type mCurrIdx;
- nsTArray<dom::Element*> mElements;
+ nsTArray<RefPtr<dom::Element>> mElements;
uint32_t mElemIdx;
};
diff --git a/accessible/base/nsAccUtils.cpp b/accessible/base/nsAccUtils.cpp
@@ -579,20 +579,19 @@ const nsAttrValue* nsAccUtils::GetARIAAttr(dom::Element* aElement,
return defaults->GetAttr(aName, kNameSpaceID_None);
}
-bool nsAccUtils::GetARIAElementsAttr(dom::Element* aElement, nsAtom* aName,
- nsTArray<dom::Element*>& aElements) {
+Maybe<nsTArray<RefPtr<dom::Element>>> nsAccUtils::GetARIAElementsAttr(
+ dom::Element* aElement, nsAtom* aName) {
if (aElement->HasAttr(aName)) {
- aElement->GetExplicitlySetAttrElements(aName, aElements);
- return true;
+ return aElement->GetExplicitlySetAttrElements(aName);
}
if (auto* element = nsGenericHTMLElement::FromNode(aElement)) {
if (auto* internals = element->GetInternals()) {
- return internals->GetAttrElements(aName, aElements);
+ return internals->GetAttrElements(aName);
}
}
- return false;
+ return Nothing();
}
bool nsAccUtils::ARIAAttrValueIs(dom::Element* aElement, const nsAtom* aName,
diff --git a/accessible/base/nsAccUtils.h b/accessible/base/nsAccUtils.h
@@ -289,8 +289,8 @@ class nsAccUtils {
nsAString& aResult);
static const nsAttrValue* GetARIAAttr(dom::Element* aElement,
const nsAtom* aName);
- static bool GetARIAElementsAttr(dom::Element* aElement, nsAtom* aName,
- nsTArray<dom::Element*>& aElements);
+ static Maybe<nsTArray<RefPtr<dom::Element>>> GetARIAElementsAttr(
+ dom::Element* aElement, nsAtom* aName);
static bool ARIAAttrValueIs(dom::Element* aElement, const nsAtom* aName,
const nsAString& aValue,
nsCaseTreatment aCaseSensitive);
diff --git a/accessible/base/nsAccessibilityService.cpp b/accessible/base/nsAccessibilityService.cpp
@@ -242,8 +242,7 @@ static bool MustBeAccessible(nsIContent* aContent, DocAccessible* aDocument) {
// If the given ID is referred by relation attribute then create an
// Accessible for it.
- nsAutoString id;
- if (nsCoreUtils::GetID(aContent, id) && !id.IsEmpty()) {
+ if (nsAtom* id = aContent->GetID()) {
return aDocument->IsDependentID(aContent->AsElement(), id);
}
}
diff --git a/accessible/generic/DocAccessible-inl.h b/accessible/generic/DocAccessible-inl.h
@@ -131,7 +131,7 @@ inline void DocAccessible::CreateSubtree(LocalAccessible* aChild) {
}
inline DocAccessible::AttrRelProviders* DocAccessible::GetRelProviders(
- dom::Element* aElement, const nsAString& aID) const {
+ dom::Element* aElement, nsAtom* aID) const {
DependentIDsHashtable* hash = mDependentIDsHashes.Get(
aElement->GetUncomposedDocOrConnectedShadowRoot());
if (hash) {
@@ -141,7 +141,7 @@ inline DocAccessible::AttrRelProviders* DocAccessible::GetRelProviders(
}
inline DocAccessible::AttrRelProviders* DocAccessible::GetOrCreateRelProviders(
- dom::Element* aElement, const nsAString& aID) {
+ dom::Element* aElement, nsAtom* aID) {
dom::DocumentOrShadowRoot* docOrShadowRoot =
aElement->GetUncomposedDocOrConnectedShadowRoot();
DependentIDsHashtable* hash =
@@ -151,7 +151,7 @@ inline DocAccessible::AttrRelProviders* DocAccessible::GetOrCreateRelProviders(
}
inline void DocAccessible::RemoveRelProvidersIfEmpty(dom::Element* aElement,
- const nsAString& aID) {
+ nsAtom* aID) {
dom::DocumentOrShadowRoot* docOrShadowRoot =
aElement->GetUncomposedDocOrConnectedShadowRoot();
DependentIDsHashtable* hash = mDependentIDsHashes.Get(docOrShadowRoot);
diff --git a/accessible/generic/DocAccessible.cpp b/accessible/generic/DocAccessible.cpp
@@ -468,19 +468,17 @@ void DocAccessible::QueueCacheUpdateForDependentRelations(
QueueCacheUpdate(relatedAcc, CacheDomain::Relations);
}
- if (aOldId) {
+ if (aOldId && !aOldId->IsEmptyString()) {
// If we have an old ID, we need to update any accessibles that depended on
// that ID as well.
- nsAutoString id;
- aOldId->ToString(id);
- if (!id.IsEmpty()) {
- auto* providers = GetRelProviders(el, id);
- if (providers) {
- for (auto& provider : *providers) {
- if (LocalAccessible* oldRelatedAcc =
- GetAccessible(provider->mContent)) {
- QueueCacheUpdate(oldRelatedAcc, CacheDomain::Relations);
- }
+ MOZ_ASSERT(aOldId->Type() == nsAttrValue::eAtom);
+ RefPtr<nsAtom> id = aOldId->GetAtomValue();
+ auto* providers = GetRelProviders(el, id);
+ if (providers) {
+ for (auto& provider : *providers) {
+ if (LocalAccessible* oldRelatedAcc =
+ GetAccessible(provider->mContent)) {
+ QueueCacheUpdate(oldRelatedAcc, CacheDomain::Relations);
}
}
}
@@ -1670,8 +1668,8 @@ void DocAccessible::ProcessInvalidationList() {
if (container) {
// Check if the node is a target of aria-owns, and if so, don't process
// it here and let DoARIAOwnsRelocation process it.
- AttrRelProviders* list = GetRelProviders(
- content->AsElement(), nsDependentAtomString(content->GetID()));
+ AttrRelProviders* list =
+ GetRelProviders(content->AsElement(), content->GetID());
bool shouldProcess = !!list;
if (shouldProcess) {
for (uint32_t idx = 0; idx < list->Length(); idx++) {
@@ -1931,7 +1929,9 @@ void DocAccessible::AddDependentIDsFor(LocalAccessible* aRelProvider,
const nsDependentSubstring id = iter.NextID();
if (id.IsEmpty()) break;
- AttrRelProviders* providers = GetOrCreateRelProviders(relProviderEl, id);
+ RefPtr<nsAtom> idAtom = NS_Atomize(id);
+ AttrRelProviders* providers =
+ GetOrCreateRelProviders(relProviderEl, idAtom);
if (providers) {
AttrRelProvider* provider = new AttrRelProvider(relAttr, relProviderEl);
if (provider) {
@@ -1974,7 +1974,9 @@ void DocAccessible::RemoveDependentIDsFor(LocalAccessible* aRelProvider,
const nsDependentSubstring id = iter.NextID();
if (id.IsEmpty()) break;
- AttrRelProviders* providers = GetRelProviders(relProviderElm, id);
+ RefPtr<nsAtom> idAtom = NS_Atomize(id);
+ AttrRelProviders* providers =
+ GetOrCreateRelProviders(relProviderElm, idAtom);
if (providers) {
providers->RemoveElementsBy(
[relAttr, relProviderElm](const auto& provider) {
@@ -1982,7 +1984,7 @@ void DocAccessible::RemoveDependentIDsFor(LocalAccessible* aRelProvider,
provider->mContent == relProviderElm;
});
- RemoveRelProvidersIfEmpty(relProviderElm, id);
+ RemoveRelProvidersIfEmpty(relProviderElm, idAtom);
}
}
@@ -2023,13 +2025,13 @@ void DocAccessible::AddDependentElementsFor(LocalAccessible* aRelProvider,
if (aRelAttr && aRelAttr != attr) {
continue;
}
- nsTArray<dom::Element*> elements;
- nsAccUtils::GetARIAElementsAttr(providerEl, attr, elements);
- for (dom::Element* targetEl : elements) {
- AttrRelProviders& providers =
- mDependentElementsMap.LookupOrInsert(targetEl);
- AttrRelProvider* provider = new AttrRelProvider(attr, providerEl);
- providers.AppendElement(provider);
+ if (auto elements = nsAccUtils::GetARIAElementsAttr(providerEl, attr)) {
+ for (auto targetEl : *elements) {
+ AttrRelProviders& providers =
+ mDependentElementsMap.LookupOrInsert(targetEl);
+ AttrRelProvider* provider = new AttrRelProvider(attr, providerEl);
+ providers.AppendElement(provider);
+ }
}
// If the relation attribute was given, we've already handled it. We don't
// have anything else to check.
@@ -2075,16 +2077,17 @@ void DocAccessible::RemoveDependentElementsFor(LocalAccessible* aRelProvider,
if (aRelAttr && aRelAttr != attr) {
continue;
}
- nsTArray<dom::Element*> elements;
- nsAccUtils::GetARIAElementsAttr(providerEl, attr, elements);
- for (dom::Element* targetEl : elements) {
- if (auto providers = mDependentElementsMap.Lookup(targetEl)) {
- providers.Data().RemoveElementsBy([attr,
- providerEl](const auto& provider) {
- return provider->mRelAttr == attr && provider->mContent == providerEl;
- });
- if (providers.Data().IsEmpty()) {
- providers.Remove();
+ if (auto elements = nsAccUtils::GetARIAElementsAttr(providerEl, attr)) {
+ for (auto targetEl : *elements) {
+ if (auto providers = mDependentElementsMap.Lookup(targetEl)) {
+ providers.Data().RemoveElementsBy(
+ [attr, providerEl](const auto& provider) {
+ return provider->mRelAttr == attr &&
+ provider->mContent == providerEl;
+ });
+ if (providers.Data().IsEmpty()) {
+ providers.Remove();
+ }
}
}
}
@@ -3177,8 +3180,7 @@ void DocAccessible::MaybeHandleChangeToHiddenNameOrDescription(
if (!id) {
continue;
}
- auto* providers =
- GetRelProviders(content->AsElement(), nsDependentAtomString(id));
+ auto* providers = GetRelProviders(content->AsElement(), id);
if (!providers) {
continue;
}
diff --git a/accessible/generic/DocAccessible.h b/accessible/generic/DocAccessible.h
@@ -329,7 +329,7 @@ class DocAccessible : public HyperTextAccessible,
/**
* Return true if the given ID is referred by relation attribute.
*/
- bool IsDependentID(dom::Element* aElement, const nsAString& aID) const {
+ bool IsDependentID(dom::Element* aElement, nsAtom* aID) const {
return GetRelProviders(aElement, aID);
}
@@ -756,7 +756,7 @@ class DocAccessible : public HyperTextAccessible,
};
typedef nsTArray<mozilla::UniquePtr<AttrRelProvider>> AttrRelProviders;
- typedef nsClassHashtable<nsStringHashKey, AttrRelProviders>
+ typedef nsClassHashtable<nsAtomHashKey, AttrRelProviders>
DependentIDsHashtable;
/**
@@ -764,11 +764,10 @@ class DocAccessible : public HyperTextAccessible,
* a DOM document if the element is in uncomposed document or associated
* with shadow DOM the element is in.
*/
- AttrRelProviders* GetRelProviders(dom::Element* aElement,
- const nsAString& aID) const;
+ AttrRelProviders* GetRelProviders(dom::Element* aElement, nsAtom* aID) const;
AttrRelProviders* GetOrCreateRelProviders(dom::Element* aElement,
- const nsAString& aID);
- void RemoveRelProvidersIfEmpty(dom::Element* aElement, const nsAString& aID);
+ nsAtom* aID);
+ void RemoveRelProvidersIfEmpty(dom::Element* aElement, nsAtom* aID);
/**
* A map used to look up the target node for an implicit reverse relation
diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
@@ -2185,20 +2185,23 @@ Element* Element::GetExplicitlySetAttrElement(nsAtom* aAttr) const {
return nullptr;
}
-void Element::GetExplicitlySetAttrElements(
- nsAtom* aAttr, nsTArray<Element*>& aElements) const {
+Maybe<nsTArray<RefPtr<dom::Element>>> Element::GetExplicitlySetAttrElements(
+ nsAtom* aAttr) const {
if (const nsExtendedDOMSlots* slots = GetExistingExtendedDOMSlots()) {
if (auto attrElementsMaybeEntry = slots->mAttrElementsMap.Lookup(aAttr)) {
auto& [attrElements, cachedAttrElements] = attrElementsMaybeEntry.Data();
if (attrElements) {
+ nsTArray<RefPtr<dom::Element>> elements;
for (const nsWeakPtr& weakEl : *attrElements) {
if (nsCOMPtr<Element> attrEl = do_QueryReferent(weakEl)) {
- aElements.AppendElement(attrEl);
+ elements.AppendElement(attrEl);
}
}
+ return Some(std::move(elements));
}
}
}
+ return Nothing();
}
void Element::GetElementsWithGrid(nsTArray<RefPtr<Element>>& aElements) {
diff --git a/dom/base/Element.h b/dom/base/Element.h
@@ -1406,8 +1406,8 @@ class Element : public FragmentOrElement {
* shadow-including ancestors. It also does not attempt to retrieve elements
* using the ids set in the content attribute.
*/
- void GetExplicitlySetAttrElements(nsAtom* aAttr,
- nsTArray<Element*>& aElements) const;
+ Maybe<nsTArray<RefPtr<dom::Element>>> GetExplicitlySetAttrElements(
+ nsAtom* aAttr) const;
PseudoStyleType GetPseudoElementType() const {
nsresult rv = NS_OK;
diff --git a/dom/html/ElementInternals.cpp b/dom/html/ElementInternals.cpp
@@ -618,9 +618,9 @@ void ElementInternals::GetAttrElements(
for (const nsWeakPtr& weakEl : attrElements) {
// For each attrElement in reflectedTarget's explicitly set attr-elements:
- if (nsCOMPtr<Element> attrEl = do_QueryReferent(weakEl)) {
+ if (RefPtr<Element> attrEl = do_QueryReferent(weakEl)) {
// Append attrElement to elements.
- elements.AppendElement(attrEl);
+ elements.AppendElement(std::move(attrEl));
}
}
@@ -654,22 +654,22 @@ void ElementInternals::GetAttrElements(
cachedAttrElements = std::move(elements);
}
-bool ElementInternals::GetAttrElements(nsAtom* aAttr,
- nsTArray<Element*>& aElements) {
- aElements.Clear();
+Maybe<nsTArray<RefPtr<Element>>> ElementInternals::GetAttrElements(
+ nsAtom* aAttr) {
auto attrElementsMaybeEntry = mAttrElementsMap.Lookup(aAttr);
if (!attrElementsMaybeEntry) {
- return false;
+ return Nothing();
}
+ nsTArray<RefPtr<Element>> elements;
auto& [attrElements, cachedAttrElements] = attrElementsMaybeEntry.Data();
for (const nsWeakPtr& weakEl : attrElements) {
- if (nsCOMPtr<Element> attrEl = do_QueryReferent(weakEl)) {
- aElements.AppendElement(attrEl);
+ if (RefPtr<Element> attrEl = do_QueryReferent(weakEl)) {
+ elements.AppendElement(std::move(attrEl));
}
}
- return true;
+ return Some(std::move(elements));
}
} // namespace mozilla::dom
diff --git a/dom/html/ElementInternals.h b/dom/html/ElementInternals.h
@@ -193,7 +193,7 @@ class ElementInternals final : public nsIFormControl,
nsresult SetAttr(nsAtom* aName, const nsAString& aValue);
- bool GetAttrElements(nsAtom* aAttr, nsTArray<Element*>& aElements);
+ Maybe<nsTArray<RefPtr<Element>>> GetAttrElements(nsAtom* aAttr);
const AttrArray& GetAttrs() const { return mAttrs; }
