Reland "[A11y] Avoid queuing up a deferred update for every attached node."
This is a reland of commit de68001bf80113d11ff879405c41070b01602902
Reverted in CL:531819. Reason for revert: bug 326386227, failing All/DumpAccessibilityTreeTest.AccessibilityTableHeadersRowRoleDynamic/blink
Fix for test: restore the code that updates roles of <th> cells when sibling cells change.
Original change's description:
> [A11y] Avoid queuing up a deferred update for every attached node.
>
> This is a performance improvement that does not affect behavior.
>
> In many cases when a node is attached, there is no need to queue up
> work for later. Only queue deferred processing when needed.
> Note that ChildrenChanged() is called in many cases where there
> was already an AXObject, but these are deduped via
> nodes_with_pending_children_changed_.
>
> Bug: 324445636
> Change-Id: I93ca7da709bfcee37545edb395c209fcc594e443
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5311098
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1263666}
Fixed: 324445636,326386227
Change-Id: I010c8abe71e8e103e762b15188c7262695047e93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5317851
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1264243}
diff --git a/third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc b/third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
index bc569b91..17008f8 100644
--- a/third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
+++ b/third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
@@ -1109,7 +1109,7 @@
AXObject* AXObjectCacheImpl::GetAXImageForMap(HTMLMapElement& map) {
// Find first child node of <map> that has an AXObject and return it's
// parent, which should be a native image.
- Node* child = LayoutTreeBuilderTraversal::FirstChild(map);
+ Node* child = ElementTraversal::FirstChild(map);
while (child) {
if (AXObject* ax_child = Get(child)) {
if (AXObject* ax_image = ax_child->CachedParentObject()) {
@@ -1123,7 +1123,7 @@
return ax_image;
}
}
- child = LayoutTreeBuilderTraversal::NextSibling(*child);
+ child = ElementTraversal::NextSibling(*child);
}
return nullptr;
}
@@ -2633,73 +2633,67 @@
RemoveSubtreeWithFlatTraversal(node);
return;
}
+ // Rare edge case: if an image is added, it could have changed the order of
+ // images with the same usemap in the document. Only the first image for a
+ // given <map> should have the <area> children. Therefore, get the current
+ // primary image before it's updated, and ensure its children are
+ // recalculated.
+ if (IsA<HTMLImageElement>(node)) {
+ if (HTMLMapElement* map = AXObject::GetMapForImage(node)) {
+ HTMLImageElement* primary_image_element = map->ImageElement();
+ if (node != primary_image_element) {
+ ChildrenChanged(primary_image_element);
+ } else if (AXObject* ax_previous_parent = GetAXImageForMap(*map)) {
+ if (ax_previous_parent->GetNode() != node) {
+ ChildrenChanged(ax_previous_parent);
+ ax_previous_parent->ClearChildren();
+ }
+ }
+ }
+ }
if (IsA<HTMLTableElement>(node) && !node->IsFinishedParsingChildren() &&
!node_to_parse_before_more_tree_updates_) {
// Tables must be fully parsed before building, because many of the
// computed properties require the entire table.
node_to_parse_before_more_tree_updates_ = node;
}
- }
-
- DeferTreeUpdate(TreeUpdateReason::kNodeIsAttached, node);
-}
-
-void AXObjectCacheImpl::NodeIsAttachedWithCleanLayout(Node* node) {
- if (!node || !node->isConnected()) {
- return;
+ // Check if a row or cell's table changed to or from a data table.
+ if (IsA<HTMLTableRowElement>(node) || IsA<HTMLTableCellElement>(node)) {
+ HTMLTableElement* table =
+ Traversal<HTMLTableElement>::FirstAncestor(*node);
+ if (table) {
+ // This will change the table's role if necessary. If that occurs, the
+ // subtree will be removed and recomputed as well, because the
+ // table row and cell roles depend on the table's role (layout vs data).
+ DeferTreeUpdate(
+ TreeUpdateReason::kRoleMaybeChangedFromAttachedTableDescendant,
+ table);
+ }
+ if (IsA<HTMLTableCellElement>(node)) {
+ DeferTreeUpdate(
+ TreeUpdateReason::kTableCellSiblingHeaderRolesMaybeChanged, node);
+ }
+ }
}
Element* element = DynamicTo<Element>(node);
-
-#if DCHECK_IS_ON()
- DCHECK(node->GetDocument().Lifecycle().GetState() >=
- DocumentLifecycle::kLayoutClean)
- << "Unclean document at lifecycle "
- << node->GetDocument().Lifecycle().ToString();
-#endif // DCHECK_IS_ON()
-
if (AccessibleNode::GetPropertyOrARIAAttributeValue(
element, AOMRelationProperty::kActiveDescendant)) {
- HandleActiveDescendantChangedWithCleanLayout(element);
+ DeferTreeUpdate(TreeUpdateReason::kActiveDescendantChanged, node);
}
AXObject* obj = Get(node);
- MaybeNewRelationTarget(*node, obj);
- // Rare edge case: if an image is added, it could have changed the order of
- // images with the same usemap in the document. Only the first image for a
- // given <map> should have the <area> children. Therefore, get the current
- // primary image before it's updated, and ensure its children are
- // recalculated.
- if (IsA<HTMLImageElement>(node)) {
- if (HTMLMapElement* map = AXObject::GetMapForImage(node)) {
- HTMLImageElement* primary_image_element = map->ImageElement();
- if (node != primary_image_element) {
- ChildrenChangedWithCleanLayout(Get(primary_image_element));
- } else if (AXObject* ax_previous_parent = GetAXImageForMap(*map)) {
- if (ax_previous_parent->GetNode() != node) {
- ChildrenChangedWithCleanLayout(ax_previous_parent->GetNode(),
- ax_previous_parent);
- ax_previous_parent->ClearChildren();
- }
- }
- }
+ if (!obj) {
+ // Nothing to do if there was not a previous object.
+ return;
}
- // Check if a row or cell's table changed to or from a data table.
- if (IsA<HTMLTableRowElement>(node) || IsA<HTMLTableCellElement>(node)) {
- Element* parent = node->parentElement();
- while (parent) {
- if (DynamicTo<HTMLTableElement>(parent)) {
- break;
- }
- parent = parent->parentElement();
- }
- if (parent) {
- UpdateTableRoleWithCleanLayout(parent);
- }
- TableCellRoleMaybeChanged(node);
- }
+ // Even if the node or parent are ignored, an ancestor may need to include
+ // descendants of the attached node, thus ChildrenChanged()
+ // must be called. It handles ignored logic, ensuring that the first ancestor
+ // that should have this as a child will be updated.
+ ChildrenChanged(obj->CachedParentObject());
}
// Note: do not call this when a child is becoming newly included, because
@@ -2885,7 +2879,9 @@
relation_cache_->UpdateRelatedTree(optional_node, obj);
}
- TableCellRoleMaybeChanged(optional_node);
+ if (IsA<HTMLTableCellElement>(optional_node)) {
+ TableCellSiblingHeaderRolesMaybeChangedWithCleanLayout(optional_node);
+ }
}
void AXObjectCacheImpl::UpdateTreeIfNeeded() {
@@ -3703,6 +3699,7 @@
break;
case TreeUpdateReason::kRoleMaybeChangedFromEventListener:
case TreeUpdateReason::kRoleMaybeChangedFromHref:
+ case TreeUpdateReason::kRoleMaybeChangedFromAttachedTableDescendant:
HandleRoleMaybeChangedWithCleanLayout(node);
break;
case TreeUpdateReason::kSectionOrRegionRoleMaybeChangedFromLabel:
@@ -3710,6 +3707,9 @@
case TreeUpdateReason::kSectionOrRegionRoleMaybeChangedFromTitle:
SectionOrRegionRoleMaybeChangedWithCleanLayout(node);
break;
+ case TreeUpdateReason::kTableCellSiblingHeaderRolesMaybeChanged:
+ TableCellSiblingHeaderRolesMaybeChangedWithCleanLayout(node);
+ break;
case TreeUpdateReason::kTextChangedOnNode:
case TreeUpdateReason::kTextChangedOnClosestNodeForLayoutObject:
TextChangedWithCleanLayout(node);
@@ -3720,9 +3720,6 @@
case TreeUpdateReason::kUpdateActiveMenuOption:
HandleUpdateActiveMenuOptionWithCleanLayout(node);
break;
- case TreeUpdateReason::kNodeIsAttached:
- NodeIsAttachedWithCleanLayout(node);
- break;
case TreeUpdateReason::kUpdateAriaOwns:
UpdateAriaOwnsWithCleanLayout(node);
break;
@@ -4054,29 +4051,23 @@
HandleRoleMaybeChangedWithCleanLayout(element);
}
-void AXObjectCacheImpl::TableCellRoleMaybeChanged(Node* node) {
- if (!node) {
- return;
- }
- // The role for a table cell depends in complex ways on multiple of its
- // siblings (see DecideRoleFromSiblings). Rather than attempt to reproduce
- // that logic here for invalidation, just recompute the role of all siblings
- // when new table cells are added.
- if (auto* cell = DynamicTo<HTMLTableCellElement>(node)) {
- for (auto* prev = LayoutTreeBuilderTraversal::PreviousSibling(*cell); prev;
- prev = LayoutTreeBuilderTraversal::PreviousSibling(*prev)) {
- HandleRoleMaybeChangedWithCleanLayout(prev);
- }
- HandleRoleMaybeChangedWithCleanLayout(cell);
- for (auto* next = LayoutTreeBuilderTraversal::NextSibling(*cell); next;
- next = LayoutTreeBuilderTraversal::PreviousSibling(*next)) {
- HandleRoleMaybeChangedWithCleanLayout(next);
+void AXObjectCacheImpl::TableCellSiblingHeaderRolesMaybeChangedWithCleanLayout(
+ Node* node) {
+ // The role for a table header (<th>) depends in complex ways on multiple of
+ // its siblings (see DecideRoleFromSiblings). Rather than attempt to reproduce
+ // that logic here for invalidation, just recompute the role of all <th>
+ // siblings when new table cells are added.
+ DCHECK(IsA<HTMLTableCellElement>(node));
+ for (HTMLTableCellElement& cell :
+ Traversal<HTMLTableCellElement>::ChildrenOf(*node->parentNode())) {
+ if (cell.HasTagName(html_names::kThTag)) {
+ HandleRoleMaybeChangedWithCleanLayout(&cell);
}
}
}
void AXObjectCacheImpl::HandleRoleMaybeChangedWithCleanLayout(Node* node) {
- if (AXObject* obj = GetOrCreate(node)) {
+ if (AXObject* obj = Get(node)) {
// If role would stay the same, do nothing.
if (obj->RoleValue() == obj->DetermineAccessibilityRole()) {
return;
diff --git a/third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h b/third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h
index b5b7f29..53531d5e 100644
--- a/third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h
+++ b/third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h
@@ -401,7 +401,7 @@
void HandleActiveDescendantChangedWithCleanLayout(Node*);
void SectionOrRegionRoleMaybeChangedWithCleanLayout(Node*);
- void TableCellRoleMaybeChanged(Node* node);
+ void TableCellSiblingHeaderRolesMaybeChangedWithCleanLayout(Node* node);
void HandleRoleMaybeChangedWithCleanLayout(Node*);
void HandleRoleChangeWithCleanLayout(Node*);
void HandleAriaExpandedChangeWithCleanLayout(Node*);
@@ -409,7 +409,6 @@
void HandleAriaPressedChangedWithCleanLayout(Node*);
void HandleNodeLostFocusWithCleanLayout(Node*);
void HandleNodeGainedFocusWithCleanLayout(Node*);
- void NodeIsAttachedWithCleanLayout(Node*);
void DidShowMenuListPopupWithCleanLayout(Node*);
void DidHideMenuListPopupWithCleanLayout(Node*);
void HandleScrollPositionChangedWithCleanLayout(Node*);
@@ -774,20 +773,21 @@
kRoleChangeFromAriaHasPopup = 20,
kRoleChangeFromImageMapName = 21,
kRoleChangeFromRoleOrType = 22,
- kRoleMaybeChangedFromEventListener = 23,
- kRoleMaybeChangedFromHref = 24,
- kSectionOrRegionRoleMaybeChangedFromLabel = 25,
- kSectionOrRegionRoleMaybeChangedFromLabelledBy = 26,
- kSectionOrRegionRoleMaybeChangedFromTitle = 27,
- kTextChangedOnNode = 28,
- kTextChangedOnClosestNodeForLayoutObject = 29,
- kTextMarkerDataAdded = 30,
- kUpdateActiveMenuOption = 31,
- kNodeIsAttached = 32,
- kUpdateAriaOwns = 33,
- kUpdateTableRole = 34,
- kUseMapAttributeChanged = 35,
- kValidationMessageVisibilityChanged = 36,
+ kRoleMaybeChangedFromAttachedTableDescendant = 23,
+ kRoleMaybeChangedFromEventListener = 24,
+ kRoleMaybeChangedFromHref = 25,
+ kSectionOrRegionRoleMaybeChangedFromLabel = 26,
+ kSectionOrRegionRoleMaybeChangedFromLabelledBy = 27,
+ kSectionOrRegionRoleMaybeChangedFromTitle = 28,
+ kTableCellSiblingHeaderRolesMaybeChanged = 29,
+ kTextChangedOnNode = 30,
+ kTextChangedOnClosestNodeForLayoutObject = 31,
+ kTextMarkerDataAdded = 32,
+ kUpdateActiveMenuOption = 33,
+ kUpdateAriaOwns = 34,
+ kUpdateTableRole = 35,
+ kUseMapAttributeChanged = 36,
+ kValidationMessageVisibilityChanged = 37,
// These updates are associated with an AXID:
kChildrenChanged = 100,
