Skip to content

Commit

Permalink
[A11y] Better identification of parents for pseudo element descendants
Browse files Browse the repository at this point in the history 
This CL changes how parents are identified for descendants of pseudo elements, such that they are never computed. Instead, they only set when the pseudo element subtree is built from the top down, by passing the object adding children as the parent. Because having a parent is a requirement for creating an AXObject, s a result of this, AXObjects for pseudo element descendants will no longer be created when requested randomly in the middle of the tree -- they will only be created as the tree is explored from an element with a node.

These rules help ensure that in certain edge cases involving pseudo elements, objects will not be created with the wrong parent or no parent. Examples of such edge cases are ::first-letter inside of a ::before. In addition, this enables removal 50 lines of special case code in this CL, while enabling more cleanups in downstream CLs.

Otherwise, the changes mostly move things around (one method had to
become static, another had to move to another class).

This helps simplify for CL:4027071, stable ids, by removing some
messy workarounds in AXObjectCacheImpl::CreateAndInit().

Bug: none
Change-Id: Iac87e490e96cdd2fa2bd021355e5c5565389d6b1
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4107969
Commit-Queue: Benjamin Beaudry <benjamin.beaudry@microsoft.com>
Reviewed-by: David Tseng <dtseng@chromium.org>
Reviewed-by: Benjamin Beaudry <benjamin.beaudry@microsoft.com>
Auto-Submit: Aaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1084524}
  • Loading branch information
@aleventhal
aleventhal authored and Chromium LUCI CQ committed Dec 16, 2022
1 parent 4a8f95e commit 6c017a1
Show file tree
Hide file tree
Showing 6 changed files with 240 additions and 238 deletions.
108 changes: 11 additions & 97 deletions third_party/blink/renderer/modules/accessibility/ax_node_object.cc
View file
Expand Up @@ -4006,64 +4006,23 @@ void AXNodeObject::AddPopupChildren() {
}
}

bool AXNodeObject::CanAddLayoutChild(LayoutObject& child) {
if (child.IsAnonymous())
return true;

// An non-anonymous layout object (has a DOM node) is only reached when a
// pseudo element is inside another pseudo element.
// This is because layout object traversal only occurs for pseudo element
// subtrees -- see AXObject::ShouldUseLayoutObjectTraversalForChildren().
// The only way a node will occur inside of that subtree is if it's another
// pseudo element.
DCHECK(child.GetNode()->IsPseudoElement());

// ---------------------------------------------------------------------------
// Under certain circumstances the LayoutTreeBuilderTraversal and LayoutObject
// trees do not match, e.g. for the combination of ::before/::after and
// ::marker pseudo elements in legacy layout.
// In this case, there is a danger that the AXObject created for |child| will
// be added in two places.Unfortunately, this requires a slow check.
// For more info, see discussion here:
// https://crrev.com/c/chromium/src/+/3591572/9/third_party/blink/renderer/modules/accessibility/ax_node_object.cc#3973
// TODO(accessibility) Remove this once legacy layout is completely removed,
// as this problem will go away.
AXObject* ax_dom_parent = AXObjectCache().SafeGet(
LayoutTreeBuilderTraversal::Parent(*child.GetNode()));
if (ax_dom_parent &&
!ax_dom_parent->ShouldUseLayoutObjectTraversalForChildren()) {
DCHECK_NE(ax_dom_parent, this);
for (Node* child_node =
LayoutTreeBuilderTraversal::FirstChild(*ax_dom_parent->GetNode());
child_node;
child_node = LayoutTreeBuilderTraversal::NextSibling(*child_node)) {
if (child_node == child.GetNode()) {
// Different AX parent would have the same AX child (via
// LayoutTreeBuilderTraversal).
return false;
}
}
}
// ---------------------------------------------------------------------------

return true;
}

#if DCHECK_IS_ON()
#define CHECK_NO_OTHER_PARENT_FOR(child) \
AXObject* ax_preexisting = AXObjectCache().Get(child); \
DCHECK(!ax_preexisting || !ax_preexisting->CachedParentObject() || \
ax_preexisting->CachedParentObject() == this) \
<< "Newly added child can't have a different preexisting parent:" \
<< "\nChild = " << ax_preexisting->ToString(true, true) \
<< "\nChild DOM node: " << ax_preexisting->GetNode() \
<< "\nChild layout object: " << ax_preexisting->GetLayoutObject() \
<< "\nNew parent = " << ToString(true, true) \
<< "\nPreexisting parent = " \
<< ax_preexisting->CachedParentObject()->ToString(true, true);
#else
#define CHECK_NO_OTHER_PARENT_FOR(child) (void(0))
#endif

void AXNodeObject::AddLayoutChildren() {
void AXNodeObject::AddPseudoElementChildrenFromLayoutTree() {
// Children are added this way only for pseudo-element subtrees.
// See AXObject::ShouldUseLayoutObjectTraversalForChildren().
if (!GetLayoutObject()) {
Expand All @@ -4073,13 +4032,11 @@ void AXNodeObject::AddLayoutChildren() {
}
LayoutObject* child = GetLayoutObject()->SlowFirstChild();
while (child) {
if (CanAddLayoutChild(*child)) {
CHECK_NO_OTHER_PARENT_FOR(child);
// All added pseudo element desecendants are included in the tree.
if (AXObject* ax_child = AXObjectCache().GetOrCreate(child, this)) {
DCHECK(AXObjectCacheImpl::IsRelevantPseudoElementDescendant(*child));
AddChildAndCheckIncluded(ax_child);
}
CHECK_NO_OTHER_PARENT_FOR(child);
// All added pseudo element descendants are included in the tree.
if (AXObject* ax_child = AXObjectCache().GetOrCreate(child, this)) {
DCHECK(AXObjectCacheImpl::IsRelevantPseudoElementDescendant(*child));
AddChildAndCheckIncluded(ax_child);
}
child = child->NextSibling();
}
Expand Down Expand Up @@ -4169,7 +4126,7 @@ void AXNodeObject::AddChildrenImpl() {
if (HasValidHTMLTableStructureAndLayout())
AddTableChildren();
else if (ShouldUseLayoutObjectTraversalForChildren())
AddLayoutChildren();
AddPseudoElementChildrenFromLayoutTree();
else
AddNodeChildren();
CHECK_ATTACHED();
Expand Down Expand Up @@ -4320,9 +4277,7 @@ void AXNodeObject::AddChildAndCheckIncluded(AXObject* child,
bool is_from_aria_owns) {
if (!child)
return;
DCHECK(child->AccessibilityIsIncludedInTree())
<< "A parent " << ToString(true, true) << "\n ... tried to add child "
<< child->ToString(true, true);
DCHECK(child->LastKnownIsIncludedInTreeValue());
AddChild(child, is_from_aria_owns);
}

Expand Down Expand Up @@ -4399,7 +4354,7 @@ bool AXNodeObject::CanHaveChildren() const {
// their contents to the browser side, allowing platforms to decide whether
// to make them a leaf, ensuring that focusable content cannot be hidden,
// and improving stability in Blink.
bool result = !GetElement() || CanHaveChildren(*GetElement());
bool result = !GetElement() || AXObject::CanHaveChildren(*GetElement());
switch (native_role_) {
case ax::mojom::blink::Role::kCheckBox:
case ax::mojom::blink::Role::kListBoxOption:
Expand Down Expand Up @@ -4430,47 +4385,6 @@ bool AXNodeObject::CanHaveChildren() const {
return result;
}

// static
bool AXNodeObject::CanHaveChildren(Element& element) {
DCHECK(!IsA<HTMLMapElement>(element));
// Placeholder gets exposed as an attribute on the input accessibility node,
// so there's no need to add its text children. Placeholder text is a separate
// node that gets removed when it disappears, so this will only be present if
// the placeholder is visible.
if (element.ShadowPseudoId() ==
shadow_element_names::kPseudoInputPlaceholder) {
return false;
}

if (IsA<HTMLBRElement>(element) &&
(!element.GetLayoutObject() || !element.GetLayoutObject()->IsBR())) {
// A <br> element that is not treated as a line break could occur when the
// <br> element has DOM children. A <br> does not usually have DOM children,
// but there is nothing preventing a script from creating this situation.
// This anomalous child content is not rendered, and therefore AXObjects
// should not be created for the children. Enforcing that <br>s to only have
// children when they are line breaks also helps create consistency: any AX
// child of a <br> will always be an AXInlineTextBox.
return false;
}

if (IsA<HTMLHRElement>(element))
return false;

if (auto* input = DynamicTo<HTMLInputElement>(&element)) {
// False for checkbox, radio and range.
return !input->IsCheckable() && input->type() != input_type_names::kRange;
}

if (IsA<HTMLOptionElement>(element))
return false;

if (IsA<HTMLProgressElement>(element))
return false;

return true;
}

//
// Properties of the object's owning document or page.
//
Expand Down
4 changes: 1 addition & 3 deletions third_party/blink/renderer/modules/accessibility/ax_node_object.h
View file
Expand Up @@ -56,8 +56,6 @@ class MODULES_EXPORT AXNodeObject : public AXObject {

void Trace(Visitor*) const override;

static bool CanHaveChildren(Element& element);

protected:
#if DCHECK_IS_ON()
bool initialized_ = false;
Expand Down Expand Up @@ -315,7 +313,7 @@ class MODULES_EXPORT AXNodeObject : public AXObject {

void AddChildrenImpl();
void AddNodeChildren();
void AddLayoutChildren();
void AddPseudoElementChildrenFromLayoutTree();
bool CanAddLayoutChild(LayoutObject& child);
// Add inline textbox children, if either force == true or
// AXObjectCache().InlineTextBoxAccessibilityEnabled().
Expand Down

0 comments on commit 6c017a1

Please sign in to comment.