diff --git a/content/browser/accessibility/dump_accessibility_tree_browsertest.cc b/content/browser/accessibility/dump_accessibility_tree_browsertest.cc index 036ebd59355a2..1f07747678b8d 100644 --- a/content/browser/accessibility/dump_accessibility_tree_browsertest.cc +++ b/content/browser/accessibility/dump_accessibility_tree_browsertest.cc @@ -947,6 +947,16 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityAriaOwns) { RunAriaTest(FILE_PATH_LITERAL("aria-owns.html")); } +IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, + AccessibilityAriaOwnsIgnored) { + RunAriaTest(FILE_PATH_LITERAL("aria-owns-ignored.html")); +} + +IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, + AccessibilityAriaOwnsIncludedInTree) { + RunAriaTest(FILE_PATH_LITERAL("aria-owns-included-in-tree.html")); +} + IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityAriaOwnsList) { RunAriaTest(FILE_PATH_LITERAL("aria-owns-list.html")); } @@ -1478,6 +1488,10 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, RunHtmlTest(FILE_PATH_LITERAL("contenteditable-with-no-descendants.html")); } +IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityCustomElement) { + RunHtmlTest(FILE_PATH_LITERAL("custom-element.html")); +} + IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityEm) { RunHtmlTest(FILE_PATH_LITERAL("em.html")); } diff --git a/content/test/data/accessibility/aria/aria-owns-ignored-expected-auralinux.txt b/content/test/data/accessibility/aria/aria-owns-ignored-expected-auralinux.txt new file mode 100644 index 0000000000000..8068ae388c064 --- /dev/null +++ b/content/test/data/accessibility/aria/aria-owns-ignored-expected-auralinux.txt @@ -0,0 +1,14 @@ +[document web] +++[separator] name='Ordinary presentation element with id' horizontal +++[separator] name='An aria-owned element is never ignored' horizontal +++[panel] +++++[push button] name='button-in-owned-tree' +++[separator] name='Element with aria-owns is never ignored' horizontal +++[section] +++++[entry] selectable-text +++[separator] name='Owning an element with an ignored parent serializes cleanly' horizontal +++[panel] +++++[check box] checkable checkable:true +++[separator] name='All the above in one' horizontal +++[section] +++++[slider] horizontal current=50.000000 minimum=0.000000 maximum=100.000000 diff --git a/content/test/data/accessibility/aria/aria-owns-ignored-expected-blink.txt b/content/test/data/accessibility/aria/aria-owns-ignored-expected-blink.txt new file mode 100644 index 0000000000000..1ce7785e4de7d --- /dev/null +++ b/content/test/data/accessibility/aria/aria-owns-ignored-expected-blink.txt @@ -0,0 +1,26 @@ +rootWebArea +++genericContainer ignored +++++genericContainer ignored +++++++splitter horizontal name='Ordinary presentation element with id' +++++++presentational ignored +++++++splitter horizontal name='An aria-owned element is never ignored' +++++++group +++++++++presentational ignored +++++++++++button name='button-in-owned-tree' +++++++++++++staticText name='button-in-owned-tree' +++++++++++++++inlineTextBox name='button-in-owned-tree' +++++++splitter horizontal name='Element with aria-owns is never ignored' +++++++genericContainer +++++++++textField +++++++++++genericContainer +++++++splitter horizontal name='Owning an element with an ignored parent serializes cleanly' +++++++presentational ignored +++++++++group +++++++++++checkBox checkedState=false +++++++++presentational ignored +++++++splitter horizontal name='All the above in one' +++++++presentational ignored +++++++++genericContainer +++++++++++presentational ignored +++++++++++++slider horizontal value='50' valueForRange=50.00 minValueForRange=0.00 maxValueForRange=100.00 +++++++++++++++sliderThumb diff --git a/content/test/data/accessibility/aria/aria-owns-ignored.html b/content/test/data/accessibility/aria/aria-owns-ignored.html new file mode 100644 index 0000000000000..217a66da5ba1d --- /dev/null +++ b/content/test/data/accessibility/aria/aria-owns-ignored.html @@ -0,0 +1,30 @@ +<!-- Make sure that the serialization/deserialization process works without + crashing when aria-owns is used with ignored nodes --> +<hr aria-label="Ordinary presentation element with id"> +<div role="presentation" id="presentational0"></div> + +<hr aria-label="An aria-owned element is never ignored"> +<div role="group" aria-owns="presentational1"></div> +<div role="presentation" id="presentational1"> + <input type="button" value="button-in-owned-tree"> +</div> + +<hr aria-label="Element with aria-owns is never ignored"> +<input type="text" id="textbox"> +<div role="presentation" aria-owns="textbox"></div> + +<hr aria-label="Owning an element with an ignored parent serializes cleanly"> +<div role="presentation"> + <div role="group" aria-owns="checkbox"></div> + <div role="presentation"> + <input type="checkbox" id="checkbox"> + </div> +</div> + +<hr aria-label="All the above in one"> +<div role="presentation" id="presentational3"> + <input type="range" id="range"> +</div> +<div role="presentation"> + <div role="presentation" aria-owns="presentational3"></div> +</div> diff --git a/content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-auralinux.txt b/content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-auralinux.txt new file mode 100644 index 0000000000000..cb22e5f98fcf2 --- /dev/null +++ b/content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-auralinux.txt @@ -0,0 +1,21 @@ +[document web] +++[separator] name='Control: elements that are not in tree' horizontal +++[section] +++++[section] +++++++[section] +++++++++[entry] selectable-text +++[separator] name='An aria-owned element is always in tree' horizontal +++[panel] +++++[push button] name='button-in-owned-tree' +++[separator] name='Element with aria-owns is always in tree' horizontal +++[section] +++++[entry] selectable-text +++[separator] name='Owning an element with unincluded ancestors serializes cleanly' horizontal +++[panel] +++++[check box] checkable checkable:true +++[separator] name='All the above in one' horizontal +++[section] +++++[section] +++++++[static] name='xyz' +++++[section] +++++++[slider] horizontal current=50.000000 minimum=0.000000 maximum=100.000000 diff --git a/content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-blink.txt b/content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-blink.txt new file mode 100644 index 0000000000000..4176f605f93cb --- /dev/null +++ b/content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-blink.txt @@ -0,0 +1,33 @@ +rootWebArea +++genericContainer ignored +++++genericContainer ignored +++++++splitter horizontal name='Control: elements that are not in tree' +++++++layoutTable +++++++++layoutTableRow +++++++++++layoutTableCell +++++++++++++textField +++++++++++++++genericContainer +++++++splitter horizontal name='An aria-owned element is always in tree' +++++++group +++++++++genericContainer ignored +++++++++++button name='button-in-owned-tree' +++++++++++++staticText name='button-in-owned-tree' +++++++++++++++inlineTextBox name='button-in-owned-tree' +++++++splitter horizontal name='Element with aria-owns is always in tree' +++++++genericContainer +++++++++textField +++++++++++genericContainer +++++++splitter horizontal name='Owning an element with unincluded ancestors serializes cleanly' +++++++group +++++++++checkBox checkedState=false +++++++splitter horizontal name='All the above in one' +++++++none ignored +++++++++genericContainer +++++++++++genericContainer +++++++++++++staticText name='xyz' +++++++++++++++inlineTextBox name='xyz' +++++++++++genericContainer +++++++++++++slider horizontal value='50' valueForRange=50.00 minValueForRange=0.00 maxValueForRange=100.00 +++++++++++++++sliderThumb +++++++none ignored +++++++genericContainer ignored invisible diff --git a/content/test/data/accessibility/aria/aria-owns-included-in-tree.html b/content/test/data/accessibility/aria/aria-owns-included-in-tree.html new file mode 100644 index 0000000000000..c2d1c18ecd10e --- /dev/null +++ b/content/test/data/accessibility/aria/aria-owns-included-in-tree.html @@ -0,0 +1,61 @@ +<!-- Make sure that the serialization/deserialization process works without + crashing when aria-owns is used with nodes that normally wouldn't be in the + tree, and that any target of an aria-owns is included in the tree --> +<test-element></test-element> +<template id="test-contents"> + <hr aria-label="Control: elements that are not in tree"> + <span id="presentational0"></span> <!-- Should not be in tree --> + <table> + <tbody> <!-- Should not be in tree --> + <td><input></td> + </tbody> + </table> + + <hr aria-label="An aria-owned element is always in tree"> + <span role="group" aria-owns="presentational1"></span> + <span id="presentational1"> <!-- Should be in tree --> + <input type="button" value="button-in-owned-tree"> + </span> + + <hr aria-label="Element with aria-owns is always in tree"> + <input type="text" id="textbox"> + <span aria-owns="textbox"></span> <!-- Should be in tree --> + + <hr aria-label="Owning an element with unincluded ancestors serializes cleanly"> + <span> + <span role="group" aria-owns="checkbox"></span> + <span> + <span> + <input type="checkbox" id="checkbox"> + </span> + </span> + </span> + + <hr aria-label="All the above in one"> + <table role="none"> + <tbody aria-owns="td"> + <td>xyz</td> + </tbody> + </table> + <table role="none"> + <tbody> <!-- Not in tree --> + <td id="td"> + <span> + <input type="range"> + </span> + </td> + </tbody> + </table> +</template> +<script> + class TestElement extends HTMLElement { + constructor() { + super(); + + const template = document.getElementById('test-contents'); + const instance = template.content.cloneNode(true); + this.attachShadow({mode: 'open'}).appendChild(instance); + } + } + customElements.define('test-element', TestElement); +</script> diff --git a/content/test/data/accessibility/html/custom-element-expected-blink.txt b/content/test/data/accessibility/html/custom-element-expected-blink.txt new file mode 100644 index 0000000000000..849b12aa37135 --- /dev/null +++ b/content/test/data/accessibility/html/custom-element-expected-blink.txt @@ -0,0 +1,13 @@ +rootWebArea +++genericContainer ignored +++++genericContainer ignored +++++++splitter horizontal name='Outside custom element' +++++++genericContainer +++++++++presentational ignored +++++++++++staticText name='a' +++++++++++++inlineTextBox name='a' +++++++splitter horizontal name='Inside custom element' +++++++genericContainer ignored +++++++++presentational ignored +++++++++++staticText name='a' +++++++++++++inlineTextBox name='a' diff --git a/content/test/data/accessibility/html/custom-element.html b/content/test/data/accessibility/html/custom-element.html new file mode 100644 index 0000000000000..4526dc6f79cda --- /dev/null +++ b/content/test/data/accessibility/html/custom-element.html @@ -0,0 +1,21 @@ +<hr aria-label="Outside custom element"> +<div id="test-contents"> + <div role="presentation"> + <span>a</span> + <span style="display:none">b</span> + <span style="visibility:hidden">c</span> + </div> +</div> +<hr aria-label="Inside custom element"> +<test-element></test-element> +<script> + class TestElement extends HTMLElement { + constructor() { + super(); + + const testContents = document.getElementById('test-contents'); + this.attachShadow({mode: 'open'}).innerHTML = testContents.outerHTML; + } + } + customElements.define('test-element', TestElement); +</script> diff --git a/third_party/blink/renderer/modules/accessibility/ax_node_object.cc b/third_party/blink/renderer/modules/accessibility/ax_node_object.cc index cc644926e0b0b..17fdb1690d9ce 100644 --- a/third_party/blink/renderer/modules/accessibility/ax_node_object.cc +++ b/third_party/blink/renderer/modules/accessibility/ax_node_object.cc @@ -3396,14 +3396,11 @@ void AXNodeObject::AddChildren() { child = LayoutTreeBuilderTraversal::NextSibling(*child)) { if (child->IsMarkerPseudoElement() && AccessibilityIsIgnored()) continue; - AXObject* child_obj = AXObjectCache().GetOrCreate(child); - if (child_obj && !AXObjectCache().IsAriaOwned(child_obj)) - AddChild(child_obj); + AddChild(AXObjectCache().GetOrCreate(child)); } } else { for (AXObject* obj = RawFirstChild(); obj; obj = obj->RawNextSibling()) { - if (!AXObjectCache().IsAriaOwned(obj)) - AddChild(obj); + AddChild(obj); } } @@ -3417,7 +3414,7 @@ void AXNodeObject::AddChildren() { AddAccessibleNodeChildren(); for (const auto& owned_child : owned_children) - AddChild(owned_child); + AddChild(owned_child, true); for (const auto& child : children_) { if (!child->CachedParentObject()) @@ -3425,16 +3422,29 @@ void AXNodeObject::AddChildren() { } } -void AXNodeObject::AddChild(AXObject* child) { +void AXNodeObject::AddChild(AXObject* child, bool is_from_aria_owns) { unsigned int index = children_.size(); - InsertChild(child, index); + if (child) + InsertChild(child, index, is_from_aria_owns); } -void AXNodeObject::InsertChild(AXObject* child, unsigned index) { +void AXNodeObject::InsertChild(AXObject* child, + unsigned index, + bool is_from_aria_owns) { if (!child || !CanHaveChildren()) return; + if (is_from_aria_owns) { + DCHECK(AXObjectCache().IsAriaOwned(child)); + } else { + // Don't add an aria-owned child to its natural parent, because it will + // already be the child of the element with aria-owns. + if (AXObjectCache().IsAriaOwned(child)) + return; + } + if (!child->AccessibilityIsIncludedInTree()) { + DCHECK(!is_from_aria_owns) << "Owned elements smust be in tree"; // Child is ignored and not in the tree. // Recompute the child's children now as we skip over the ignored object. child->SetNeedsToUpdateChildren(); @@ -3443,8 +3453,13 @@ void AXNodeObject::InsertChild(AXObject* child, unsigned index) { // unignored descendants as it goes. const auto& children = child->ChildrenIncludingIgnored(); wtf_size_t length = children.size(); - for (wtf_size_t i = 0; i < length; ++i) - children_.insert(index + i, children[i]); + for (wtf_size_t i = 0; i < length; ++i) { + // If the child was owned, it will be added elsewhere as a direct + // child of the object owning it, and not as an indirect child under + // an object not included in the tree. + if (!AXObjectCache().IsAriaOwned(children[i])) + children_.insert(index + i, children[i]); + } } else if (!child->IsMenuListOption()) { // MenuListOptions must only be added in AXMenuListPopup::AddChildren. children_.insert(index, child); diff --git a/third_party/blink/renderer/modules/accessibility/ax_node_object.h b/third_party/blink/renderer/modules/accessibility/ax_node_object.h index 0538e45266caa..3220246b4f47f 100644 --- a/third_party/blink/renderer/modules/accessibility/ax_node_object.h +++ b/third_party/blink/renderer/modules/accessibility/ax_node_object.h @@ -221,8 +221,12 @@ class MODULES_EXPORT AXNodeObject : public AXObject { void AddChildren() override; bool CanHaveChildren() const override; - void AddChild(AXObject*); - void InsertChild(AXObject*, unsigned index); + // Set is_from_aria_owns to true if the child is being added because it was + // pointed to from aria-owns. + void AddChild(AXObject*, bool is_from_aria_owns = false); + // Set is_from_aria_owns to true if the child is being insert because it was + // pointed to from aria-owns. + void InsertChild(AXObject*, unsigned index, bool is_from_aria_owns = false); void ClearChildren() override; bool NeedsToUpdateChildren() const override { return children_dirty_; } void SetNeedsToUpdateChildren() override { children_dirty_ = true; } diff --git a/third_party/blink/renderer/modules/accessibility/ax_object.cc b/third_party/blink/renderer/modules/accessibility/ax_object.cc index daae2412f4d2c..1e6f995ddc193 100644 --- a/third_party/blink/renderer/modules/accessibility/ax_object.cc +++ b/third_party/blink/renderer/modules/accessibility/ax_object.cc @@ -1643,6 +1643,12 @@ const AXObject* AXObject::DisabledAncestor() const { } bool AXObject::ComputeAccessibilityIsIgnoredButIncludedInTree() const { + if (AXObjectCache().IsAriaOwned(this)) { + // Always include an aria-owned object. It must be a child of the + // element with aria-owns. + return true; + } + if (!GetNode()) return false; diff --git a/third_party/blink/renderer/modules/accessibility/ax_relation_cache.cc b/third_party/blink/renderer/modules/accessibility/ax_relation_cache.cc index cb5f72fe40fbd..36480b95ef2eb 100644 --- a/third_party/blink/renderer/modules/accessibility/ax_relation_cache.cc +++ b/third_party/blink/renderer/modules/accessibility/ax_relation_cache.cc @@ -152,6 +152,7 @@ void AXRelationCache::UpdateAriaOwnsFromAttrAssociatedElements( // attr-associated elements have already had their scope validated, but they // need to be further validated to determine if they introduce a cycle or are // already owned by another element. + Vector<String> owned_id_vector; for (const auto& element : attr_associated_elements) { AXObject* child = GetOrCreate(element); @@ -225,6 +226,13 @@ void AXRelationCache::UpdateAriaOwnerToChildrenMapping( // Finally, update the mapping from the owner to the list of child IDs. aria_owner_to_children_mapping_.Set(owner->AXObjectID(), validated_owned_child_axids); + +#if DCHECK_IS_ON() + // Owned children must be in tree to avoid serialization issues. + for (AXObject* child : validated_owned_children_result) { + DCHECK(child->AccessibilityIsIncludedInTree()); + } +#endif } bool AXRelationCache::MayHaveHTMLLabelViaForAttribute( diff --git a/third_party/blink/web_tests/accessibility/aria-owns-presentational-node-edge-cases.html b/third_party/blink/web_tests/accessibility/aria-owns-presentational-node-edge-cases.html new file mode 100644 index 0000000000000..d521f7aeaa79c --- /dev/null +++ b/third_party/blink/web_tests/accessibility/aria-owns-presentational-node-edge-cases.html @@ -0,0 +1,30 @@ +<!DOCTYPE HTML> +<script src="../resources/testharness.js"></script> +<script src="../resources/testharnessreport.js"></script> + +<div role="presentation"> + <div id="parent"> <!-- Will get aria-owns="2" --> + </div> + <div id="2" role="presentation"> + <div id="3" role="button"></div> + <div id="4" role="button"></div> + </div> +</div> + +<script> +test(function(t) +{ + const axParent = accessibilityController.accessibleElementById("parent"); + const axRoot = axParent.parentElement(); + assert_equals(axRoot.role, "AXRole: AXPresentational"); + assert_equals(axRoot.childrenCount, 2); + + document.getElementById('parent').setAttribute('aria-owns', '2'); + assert_equals(axParent.childrenCount, 1); + assert_equals(axRoot.childrenCount, 1); // Root no longer owns #2. + + const axChild = axParent.childAtIndex(0); + assert_equals(axChild.role, "AXRole: AXPresentational"); +}, "Aria-owns cannot be fooled by pointing it at a presentational node."); +</script> + diff --git a/ui/accessibility/ax_tree_serializer.h b/ui/accessibility/ax_tree_serializer.h index a8ea1e8dcff4c..f05530b49724f 100644 --- a/ui/accessibility/ax_tree_serializer.h +++ b/ui/accessibility/ax_tree_serializer.h @@ -178,9 +178,6 @@ class AXTreeSerializer { AXSourceNode node, AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update); - // Visit all of the descendants of |node| once. - void WalkAllDescendants(AXSourceNode node); - // Delete the entire client subtree but don't set the did_reset_ flag // like when Reset() is called. void InternalReset(); @@ -459,12 +456,6 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeChanges( if (!tree_->IsValid(lca)) lca = tree_->GetRoot(); - // Work around flaky source trees where nodes don't figure out their - // correct parent/child relationships until you walk the whole tree once. - // Covered by this test in the content_browsertests suite: - // DumpAccessibilityTreeTest.AccessibilityAriaOwns. - WalkAllDescendants(lca); - if (!SerializeChangedNodes(lca, out_update)) return false; @@ -588,12 +579,23 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>:: ClientTreeNode* client_child = ClientTreeNodeById(new_child_id); if (client_child && GetClientTreeNodeParent(client_child) != client_node) { - DVLOG(1) << "Illegal reparenting detected"; -#if defined(ADDRESS_SANITIZER) +#if defined(ADDRESS_SANITIZER) || !defined(NDEBUG) // Wrapping this in ADDRESS_SANITIZER will cause it to run on - // clusterfuzz, which should help us narrow down the issue. - NOTREACHED() << "Illegal reparenting detected"; + // clusterfuzz, which should help us narrow down remaining issues. + // TODO(accessibility) Remove all cases where this occurs and re-add + // NOTREACHED(), or add a histogram. + // This condition leads to performance problems. It will + // also reset virtual buffers, causing users to lose their place. + NOTREACHED() +#else + LOG(ERROR) #endif + << "Illegal reparenting detected: " + << "\nPassed-in parent: " + << tree_->GetDebugString(tree_->GetFromId(client_node->id)) + << "\nChild: " << tree_->GetDebugString(child) << "\nChild's parent: " + << tree_->GetDebugString(tree_->GetFromId(client_child->parent->id)) + << "\n-----------------------------------------\n\n\n"; Reset(); return false; } @@ -672,6 +674,28 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>:: new_child->ignored = tree_->IsIgnored(child); new_child->invalid = false; client_node->children.push_back(new_child); + if (ClientTreeNodeById(child_id)) { +#if defined(ADDRESS_SANITIZER) || !defined(NDEBUG) + // Wrapping this in ADDRESS_SANITIZER will cause it to run on + // clusterfuzz, which should help us narrow down remaining issues. + // TODO(accessibility) Remove all cases where this occurs and re-add + // NOTREACHED(), or add a histogram. + // This condition leads to performance problems. It will + // also reset virtual buffers, causing users to lose their place. + NOTREACHED() +#else + LOG(ERROR) +#endif + << "Child id " << child_id << " already exists in map." + << "\nChild is " + << tree_->GetDebugString(tree_->GetFromId(child_id)) + << "\nWanted as child of parent " << tree_->GetDebugString(node) + << "\nAlready had parent " + << tree_->GetDebugString( + tree_->GetFromId(ClientTreeNodeById(child_id)->parent->id)); + Reset(); + return false; + } client_id_map_[child_id] = new_child; if (!SerializeChangedNodes(child, out_update)) return false; @@ -686,15 +710,6 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>:: return true; } -template <typename AXSourceNode, typename AXNodeData, typename AXTreeData> -void AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::WalkAllDescendants( - AXSourceNode node) { - std::vector<AXSourceNode> children; - tree_->GetChildren(node, &children); - for (size_t i = 0; i < children.size(); ++i) - WalkAllDescendants(children[i]); -} - } // namespace ui #endif // UI_ACCESSIBILITY_AX_TREE_SERIALIZER_H_