Skip to content

Commit

Permalink
Ensure BrowserAccessibility::Platform* methods have the same children
Browse files Browse the repository at this point in the history
BrowserAccessibility::PlatformChildCount
considers BrowserAccessibility::IsLeaf() as having 0 children

while

BrowserAccessibility::PlatformGetFirstChild (via BrowserAccessibility::PlatformGetChild)
BrowserAccessibility::PlatformGetLastChild

do not.

Make this consistent by having Platform[First|Last]Child check that
PlatformChildCount is 0 and return nullptr accordingly.

Change-Id: I81d1535bf658642ed9f6c8dc9e7f00a5ba38bdd4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3574854
Reviewed-by: Aaron Leventhal <aleventhal@chromium.org>
Commit-Queue: David Tseng <dtseng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#991087}
  • Loading branch information
dtsengchromium authored and Chromium LUCI CQ committed Apr 11, 2022
1 parent 1e6cd10 commit 15db761
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 12 deletions.
15 changes: 12 additions & 3 deletions content/browser/accessibility/ax_platform_node_win_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -726,23 +726,32 @@ IN_PROC_BROWSER_TEST_F(AXPlatformNodeWinBrowserTest, IFrameTraversal) {
FindNodeAfter(root_node, "Before iframe");
ASSERT_NE(nullptr, before_iframe_node);
ASSERT_EQ(ax::mojom::Role::kStaticText, before_iframe_node->GetRole());
before_iframe_node = before_iframe_node->PlatformGetFirstChild();

ASSERT_EQ(0U, before_iframe_node->PlatformChildCount());
ASSERT_EQ(1U, before_iframe_node->InternalChildCount());
before_iframe_node = before_iframe_node->InternalGetFirstChild();
ASSERT_NE(nullptr, before_iframe_node);
ASSERT_EQ(ax::mojom::Role::kInlineTextBox, before_iframe_node->GetRole());

BrowserAccessibility* inside_iframe_node =
FindNodeAfter(before_iframe_node, "Text in iframe");
ASSERT_NE(nullptr, inside_iframe_node);
ASSERT_EQ(ax::mojom::Role::kStaticText, inside_iframe_node->GetRole());
inside_iframe_node = inside_iframe_node->PlatformGetFirstChild();

ASSERT_EQ(0U, inside_iframe_node->PlatformChildCount());
ASSERT_EQ(1U, inside_iframe_node->InternalChildCount());
inside_iframe_node = inside_iframe_node->InternalGetFirstChild();
ASSERT_NE(nullptr, inside_iframe_node);
ASSERT_EQ(ax::mojom::Role::kInlineTextBox, inside_iframe_node->GetRole());

BrowserAccessibility* after_iframe_node =
FindNodeAfter(inside_iframe_node, "After iframe");
ASSERT_NE(nullptr, after_iframe_node);
ASSERT_EQ(ax::mojom::Role::kStaticText, after_iframe_node->GetRole());
after_iframe_node = after_iframe_node->PlatformGetFirstChild();

ASSERT_EQ(0U, after_iframe_node->PlatformChildCount());
ASSERT_EQ(1U, after_iframe_node->InternalChildCount());
after_iframe_node = after_iframe_node->InternalGetFirstChild();
ASSERT_NE(nullptr, after_iframe_node);
ASSERT_EQ(ax::mojom::Role::kInlineTextBox, after_iframe_node->GetRole());

Expand Down
14 changes: 9 additions & 5 deletions content/browser/accessibility/browser_accessibility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,16 @@ BrowserAccessibility* BrowserAccessibility::PlatformGetParent() const {
}

BrowserAccessibility* BrowserAccessibility::PlatformGetFirstChild() const {
if (PlatformChildCount() == 0)
return nullptr;

return PlatformGetChild(0);
}

BrowserAccessibility* BrowserAccessibility::PlatformGetLastChild() const {
if (PlatformChildCount() == 0)
return nullptr;

BrowserAccessibility* child_tree_root = PlatformGetRootOfChildTree();
return child_tree_root ? child_tree_root : InternalGetLastChild();
}
Expand Down Expand Up @@ -211,6 +217,9 @@ bool BrowserAccessibility::IsLineBreakObject() const {

BrowserAccessibility* BrowserAccessibility::PlatformGetChild(
uint32_t child_index) const {
if (PlatformChildCount() == 0)
return nullptr;

BrowserAccessibility* child_tree_root = PlatformGetRootOfChildTree();
if (child_tree_root) {
// A node with a child tree has only one child.
Expand Down Expand Up @@ -776,11 +785,6 @@ gfx::RectF BrowserAccessibility::GetInlineTextRect(const int start_offset,

BrowserAccessibility* BrowserAccessibility::ApproximateHitTest(
const gfx::Point& blink_screen_point) {
// TODO(accessibility): propagate this to all tree traversal methods and
// remove here. This keeps things equivalent for now.
if (IsLeaf())
return this;

// The best result found that's a child of this object.
BrowserAccessibility* child_result = nullptr;
// The best result that's an indirect descendant like grandchild, etc.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,17 @@ TEST_F(BrowserAccessibilityManagerAuraLinuxTest, TestEmitChildrenChanged) {
}),
nullptr);

// The static text is a platform leaf.
ASSERT_EQ(0U, static_text_accessible->PlatformChildCount());
ASSERT_EQ(1U, static_text_accessible->InternalChildCount());

BrowserAccessibility* inline_text_accessible =
static_text_accessible->PlatformGetChild(0);
static_text_accessible->InternalGetChild(0);
// PlatformLeaf node such as InlineText should not trigger
// 'children-changed' event to the parent when subtree is changed.
aura_linux_manager->FireSubtreeCreatedEvent(inline_text_accessible);
aura_linux_manager->OnSubtreeWillBeDeleted(manager->ax_tree(),
inline_text_accessible->node());
}

} // namespace content
} // namespace content
Original file line number Diff line number Diff line change
Expand Up @@ -937,8 +937,11 @@ TEST_F(BrowserAccessibilityManagerTest, TestFindIndicesInCommonParent) {
ASSERT_EQ(2U, div_accessible->PlatformChildCount());
BrowserAccessibility* button_accessible = div_accessible->PlatformGetChild(0);
ASSERT_NE(nullptr, button_accessible);
ASSERT_EQ(0U, button_accessible->PlatformChildCount());
ASSERT_EQ(1U, button_accessible->InternalChildCount());

BrowserAccessibility* button_text_accessible =
button_accessible->PlatformGetChild(0);
button_accessible->InternalGetChild(0);
ASSERT_NE(nullptr, button_text_accessible);
BrowserAccessibility* line_break_accessible =
div_accessible->PlatformGetChild(1);
Expand Down Expand Up @@ -1096,8 +1099,11 @@ TEST_F(BrowserAccessibilityManagerTest, TestGetTextForRange) {
ASSERT_EQ(3U, div_accessible->PlatformChildCount());
BrowserAccessibility* button_accessible = div_accessible->PlatformGetChild(0);
ASSERT_NE(nullptr, button_accessible);
ASSERT_EQ(0U, button_accessible->PlatformChildCount());
ASSERT_EQ(1U, button_accessible->InternalChildCount());

BrowserAccessibility* button_text_accessible =
button_accessible->PlatformGetChild(0);
button_accessible->InternalGetChild(0);
ASSERT_NE(nullptr, button_text_accessible);
BrowserAccessibility* container_accessible =
div_accessible->PlatformGetChild(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -960,11 +960,18 @@ TEST_F(BrowserAccessibilityTest, CreatePositionAt) {
BrowserAccessibility::AXPosition pos = gc_accessible->CreatePositionAt(0);
EXPECT_TRUE(pos->IsTreePosition());

ASSERT_EQ(1U, gc_accessible->InternalChildCount());
#if BUILDFLAG(IS_ANDROID)
// On Android, nodes with only static text can drop their children.
ASSERT_EQ(0U, gc_accessible->PlatformChildCount());
#else
ASSERT_EQ(1U, gc_accessible->PlatformChildCount());
BrowserAccessibility* text_accessible = gc_accessible->PlatformGetChild(0);
ASSERT_NE(nullptr, text_accessible);

pos = text_accessible->CreatePositionAt(0);
EXPECT_TRUE(pos->IsTextPosition());
#endif // BUILDFLAG(IS_ANDROID)
}

} // namespace content

0 comments on commit 15db761

Please sign in to comment.