[A11y] Retain anchor in AXPlatformNodeBase::FindTextBoundary

AsUnignoredPosition has a "last resort" of finding the next/previous
unignored position, but in cases where we are required to stop at Anchor
Boundaries, this behavior can cause bugs, as callers expect the node the
position is anchored in to be retained when called with
AXBoundaryBehavior::kStopAtAnchorBoundary

Bug: 6480215
Change-Id: I89bf02f9f06677750b325cb677ef572e6c3b1b51
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6500379
Commit-Queue: Jacques Newman <janewman@microsoft.com>
Reviewed-by: Benjamin Beaudry <benjamin.beaudry@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1454484}

ui/accessibility/ax_node_position_unittest.cc

@@ -12354,6 +12354,143 @@ TEST_F(AXPositionTest, TextNavigationWithCollapsedCombobox) {
   EXPECT_EQ(2, position->text_offset());
 }
 
+TEST_F(AXPositionTest, CreatePositionAtTextBoundaryWithHiddenLineBreak) {
+  // This test simulates a contenteditable div with three lines, where the
+  // middle line has a hidden <br> element. Structure: rootWebArea
+  // ++genericContainer editable
+  // ++++div "first"
+  // ++++div id="parent"
+  // ++++++span (contenteditable=false)
+  // ++++++span "second"
+  // ++++++br (style="display: none")
+  // ++++div "third"
+
+  AXNodeData root_data;
+  root_data.id = 1;
+  root_data.role = ax::mojom::Role::kRootWebArea;
+
+  AXNodeData editable_container;
+  editable_container.id = 12;
+  editable_container.role = ax::mojom::Role::kGenericContainer;
+  editable_container.AddState(ax::mojom::State::kEditable);
+  editable_container.AddState(ax::mojom::State::kFocusable);
+  editable_container.AddState(ax::mojom::State::kMultiline);
+  editable_container.AddState(ax::mojom::State::kRichlyEditable);
+  editable_container.AddBoolAttribute(
+      ax::mojom::BoolAttribute::kIsLineBreakingObject, true);
+
+  AXNodeData first_div;
+  first_div.id = 3;
+  first_div.role = ax::mojom::Role::kGenericContainer;
+  first_div.AddState(ax::mojom::State::kEditable);
+  first_div.AddState(ax::mojom::State::kRichlyEditable);
+  first_div.AddBoolAttribute(ax::mojom::BoolAttribute::kIsLineBreakingObject,
+                             true);
+
+  AXNodeData first_static_text;
+  first_static_text.id = 8;
+  first_static_text.role = ax::mojom::Role::kStaticText;
+  first_static_text.AddState(ax::mojom::State::kEditable);
+  first_static_text.AddState(ax::mojom::State::kRichlyEditable);
+  first_static_text.SetName("first");
+
+  AXNodeData parent_div;
+  parent_div.id = 4;
+  parent_div.role = ax::mojom::Role::kGenericContainer;
+  parent_div.AddState(ax::mojom::State::kEditable);
+  parent_div.AddState(ax::mojom::State::kRichlyEditable);
+  parent_div.AddBoolAttribute(ax::mojom::BoolAttribute::kIsLineBreakingObject,
+                              true);
+
+  AXNodeData span_noneditable;
+  span_noneditable.id = 13;
+  span_noneditable.role = ax::mojom::Role::kGenericContainer;
+
+  AXNodeData second_static_text;
+  second_static_text.id = 9;
+  second_static_text.role = ax::mojom::Role::kStaticText;
+  second_static_text.AddState(ax::mojom::State::kEditable);
+  second_static_text.AddState(ax::mojom::State::kRichlyEditable);
+  second_static_text.SetName("second");
+
+  AXNodeData hidden_br;
+  hidden_br.id = 15;
+  hidden_br.role = ax::mojom::Role::kGenericContainer;
+  hidden_br.AddState(ax::mojom::State::kIgnored);
+  hidden_br.AddState(ax::mojom::State::kInvisible);
+  hidden_br.AddBoolAttribute(ax::mojom::BoolAttribute::kIsLineBreakingObject,
+                             true);
+
+  AXNodeData third_div;
+  third_div.id = 5;
+  third_div.role = ax::mojom::Role::kGenericContainer;
+  third_div.AddState(ax::mojom::State::kEditable);
+  third_div.AddState(ax::mojom::State::kRichlyEditable);
+  third_div.AddBoolAttribute(ax::mojom::BoolAttribute::kIsLineBreakingObject,
+                             true);
+
+  AXNodeData third_static_text;
+  third_static_text.id = 10;
+  third_static_text.role = ax::mojom::Role::kStaticText;
+  third_static_text.AddState(ax::mojom::State::kEditable);
+  third_static_text.AddState(ax::mojom::State::kRichlyEditable);
+  third_static_text.SetName("third");
+
+  root_data.child_ids = {editable_container.id};
+  editable_container.child_ids = {first_div.id, parent_div.id, third_div.id};
+  first_div.child_ids = {first_static_text.id};
+  parent_div.child_ids = {span_noneditable.id, second_static_text.id,
+                          hidden_br.id};
+  third_div.child_ids = {third_static_text.id};
+
+  SetTree(
+      CreateAXTree({root_data, editable_container, first_div, first_static_text,
+                    parent_div, span_noneditable, second_static_text, hidden_br,
+                    third_div, third_static_text}));
+
+  TestPositionType text_position =
+      CreateTextPosition(parent_div, 0, ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+
+  // Navigate to the next line start boundary.
+  TestPositionType line_position = text_position->CreatePositionAtTextBoundary(
+      ax::mojom::TextBoundary::kLineStart, ax::mojom::MoveDirection::kForward,
+      {AXBoundaryBehavior::kStopAtAnchorBoundary,
+       AXBoundaryDetection::kDontCheckInitialPosition});
+
+  ASSERT_NE(nullptr, line_position);
+  EXPECT_TRUE(line_position->IsTextPosition());
+  EXPECT_EQ(parent_div.id, line_position->anchor_id());
+  EXPECT_EQ(6, line_position->text_offset());
+
+  line_position = text_position->CreatePositionAtTextBoundary(
+      ax::mojom::TextBoundary::kLineStart, ax::mojom::MoveDirection::kBackward,
+      {AXBoundaryBehavior::kStopAtAnchorBoundary,
+       AXBoundaryDetection::kDontCheckInitialPosition});
+
+  ASSERT_NE(nullptr, line_position);
+  EXPECT_TRUE(line_position->IsTextPosition());
+  EXPECT_EQ(parent_div.id, line_position->anchor_id());
+  EXPECT_EQ(0, line_position->text_offset());
+
+  text_position =
+      CreateTextPosition(third_div, 4, ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+
+  // Navigate to the next line start boundary.
+  line_position = text_position->CreatePositionAtTextBoundary(
+      ax::mojom::TextBoundary::kLineStart, ax::mojom::MoveDirection::kBackward,
+      {AXBoundaryBehavior::kStopAtAnchorBoundary,
+       AXBoundaryDetection::kDontCheckInitialPosition});
+
+  ASSERT_NE(nullptr, line_position);
+  EXPECT_TRUE(line_position->IsTextPosition());
+  EXPECT_EQ(third_div.id, line_position->anchor_id());
+  EXPECT_EQ(0, line_position->text_offset());
+}
+
 TEST_F(AXPositionTest, GetUnignoredSelectionWithLeafNodes) {
   ScopedAXEmbeddedObjectBehaviorSetter ax_embedded_object_behavior(
       AXEmbeddedObjectBehavior::kExposeCharacterForHypertext);

ui/accessibility/ax_position.h

@@ -1961,6 +1961,62 @@ class AXPosition {
     return leaf_tree_position;
   }
 
+  // This method is similar to `AsUnignoredPosition`, but it will never cross
+  // an anchor boundary. This means that if the position is at the start or end
+  // of the anchor, it will return a position at the start or end of the anchor,
+  // respectively. This is useful when we want to ensure that the resulting
+  // position is still within the same anchor. If no unignored position can be
+  // found, it will return a null position.
+  AXPositionInstance TryAsUnignoredPositionPreservingAnchor(
+      AXPositionAdjustmentBehavior behavior) const {
+    if (IsNullPosition()) {
+      return Clone();
+    }
+
+    AXPositionInstance new_position = AsUnignoredPosition(behavior);
+    if (GetAnchor() == new_position->GetAnchor()) {
+      return new_position;
+    }
+
+    // As a last resort, AsUnignoredPosition() may return a position that is
+    // anchored at a different node. In such case, we need to create a new
+    // position that is anchored at the same node as this position. To do this,
+    // Try Calling AsUnignoredPosition in the other direction, starting from
+    // one of the ends, as there may not have been any unignored positions in
+    // the original direction.
+    switch (behavior) {
+      case AXPositionAdjustmentBehavior::kMoveBackward:
+        new_position = CreatePositionAtStartOfAnchor()->AsUnignoredPosition(
+            AXPositionAdjustmentBehavior::kMoveForward);
+        break;
+      case AXPositionAdjustmentBehavior::kMoveForward:
+        new_position = CreatePositionAtEndOfAnchor()->AsUnignoredPosition(
+            AXPositionAdjustmentBehavior::kMoveBackward);
+        break;
+    }
+    if (GetAnchor() != new_position->GetAnchor()) {
+      // Check to see if the new position can be expressed in terms of the
+      // current anchor.
+      new_position = new_position->CreateAncestorPosition(
+          GetAnchor(), behavior == AXPositionAdjustmentBehavior::kMoveForward
+                           ? ax::mojom::MoveDirection::kForward
+                           : ax::mojom::MoveDirection::kBackward);
+    }
+    // It could be that there are no unignored positions that can be rooted
+    // at the current anchor. In such case, we return a null position.
+    if (new_position->IsIgnored()) {
+      return CreateNullPosition();
+    }
+
+    // Retain original position type.
+    if (IsTextPosition()) {
+      new_position = new_position->AsTextPosition();
+    } else {
+      new_position = new_position->AsTreePosition();
+    }
+    return new_position;
+  }
+
   // Searches backward and forward from this position until it finds the given
   // text boundary, and creates an AXRange that spans from the former to the
   // latter. The resulting AXRange is always a forward range: its anchor always
@@ -3522,11 +3578,13 @@ class AXPosition {
               case ax::mojom::MoveDirection::kNone:
                 NOTREACHED();
               case ax::mojom::MoveDirection::kBackward:
-                return CreatePositionAtStartOfAnchor()->AsUnignoredPosition(
-                    AXPositionAdjustmentBehavior::kMoveBackward);
+                return CreatePositionAtStartOfAnchor()
+                    ->TryAsUnignoredPositionPreservingAnchor(
+                        AXPositionAdjustmentBehavior::kMoveBackward);
               case ax::mojom::MoveDirection::kForward:
-                return CreatePositionAtEndOfAnchor()->AsUnignoredPosition(
-                    AXPositionAdjustmentBehavior::kMoveForward);
+                return CreatePositionAtEndOfAnchor()
+                    ->TryAsUnignoredPositionPreservingAnchor(
+                        AXPositionAdjustmentBehavior::kMoveForward);
             }
           }
 
@@ -3572,12 +3630,14 @@ class AXPosition {
         case ax::mojom::MoveDirection::kNone:
           NOTREACHED();
         case ax::mojom::MoveDirection::kBackward:
-          text_position = CreatePositionAtStartOfAnchor()->AsUnignoredPosition(
-              AXPositionAdjustmentBehavior::kMoveBackward);
+          text_position = CreatePositionAtStartOfAnchor()
+                              ->TryAsUnignoredPositionPreservingAnchor(
+                                  AXPositionAdjustmentBehavior::kMoveBackward);
           break;
         case ax::mojom::MoveDirection::kForward:
-          text_position = CreatePositionAtEndOfAnchor()->AsUnignoredPosition(
-              AXPositionAdjustmentBehavior::kMoveForward);
+          text_position = CreatePositionAtEndOfAnchor()
+                              ->TryAsUnignoredPositionPreservingAnchor(
+                                  AXPositionAdjustmentBehavior::kMoveForward);
       }
 
       // Preserve affinity for forward upstream positions.
@@ -3685,11 +3745,13 @@ class AXPosition {
               case ax::mojom::MoveDirection::kNone:
                 NOTREACHED();
               case ax::mojom::MoveDirection::kBackward:
-                return CreatePositionAtStartOfAnchor()->AsUnignoredPosition(
-                    AXPositionAdjustmentBehavior::kMoveBackward);
+                return CreatePositionAtStartOfAnchor()
+                    ->TryAsUnignoredPositionPreservingAnchor(
+                        AXPositionAdjustmentBehavior::kMoveBackward);
               case ax::mojom::MoveDirection::kForward:
-                return CreatePositionAtEndOfAnchor()->AsUnignoredPosition(
-                    AXPositionAdjustmentBehavior::kMoveForward);
+                return CreatePositionAtEndOfAnchor()
+                    ->TryAsUnignoredPositionPreservingAnchor(
+                        AXPositionAdjustmentBehavior::kMoveForward);
             }
           }
 
@@ -3735,11 +3797,13 @@ class AXPosition {
         case ax::mojom::MoveDirection::kNone:
           NOTREACHED();
         case ax::mojom::MoveDirection::kBackward:
-          return CreatePositionAtStartOfAnchor()->AsUnignoredPosition(
-              AXPositionAdjustmentBehavior::kMoveBackward);
+          return CreatePositionAtStartOfAnchor()
+              ->TryAsUnignoredPositionPreservingAnchor(
+                  AXPositionAdjustmentBehavior::kMoveBackward);
         case ax::mojom::MoveDirection::kForward:
-          return CreatePositionAtEndOfAnchor()->AsUnignoredPosition(
-              AXPositionAdjustmentBehavior::kMoveForward);
+          return CreatePositionAtEndOfAnchor()
+              ->TryAsUnignoredPositionPreservingAnchor(
+                  AXPositionAdjustmentBehavior::kMoveForward);
       }
     }
 

ui/accessibility/platform/ax_platform_node_base.cc

@@ -2201,6 +2201,7 @@ int AXPlatformNodeBase::FindTextBoundary(
       position->CreatePositionAtTextBoundary(boundary, direction, options);
   if (boundary_position->IsNullPosition())
     return -1;
+  DCHECK_EQ(boundary_position->GetAnchor(), position->GetAnchor());
   DCHECK_GE(boundary_position->text_offset(), 0);
   return boundary_position->text_offset();
 }