Fix ExpandToEnclosingUnit issue with positions between text anchors

1. Previously, our implementation of `ExpandToEnclosingUnit` didn't take
   into account that, since we changed the behavior of text positions to
   be able to reach the maximum text offset of its anchor, there are
   multiple text positions that are equal when compared, but are located
   in different anchors.

   For example, the end position of an anchor and the start position of
   the next text anchor. This could get even trickier with empty text
   anchors between characters in the text representation.

   Such scenarios are relevant to some assistive technologies since the
   anchors that the enclosing range spans affect `GetEnclosingElement`,
   ultimately breaking functions of these ATs.

   Implementing two new methods `AsPositionBeforeCharacter` and
   `AsPositionAfterCharacter` in `AXPosition`, which we will use to
   normalize positions between characters of the text representation.

2. Adding unit tests for both new methods in `AXPositionTest`.

3. Improving `TestITextRangeProviderExpandToEnclosingCharacter` coverage
   in `AXPlatformNodeTextRangeProviderTest` to check that the enclosing
   element resolved from the expanded range matches AT expectations.

4. Addressing a bug in `ExpandToEnclosingUnit` where the position at the
   end of the last anchor of the tree does not expand to the last
   character of its text representation.

Bug: 961893
Change-Id: Ib168d680578a3b460b57b7ca9808987568743a95
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1614940Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Commit-Queue: Ethan Jimenez <ethavar@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#660881}

ui/accessibility/ax_node_position_unittest.cc

@@ -1672,6 +1672,128 @@ TEST_F(AXPositionTest, CreatePreviousTextAnchorPosition) {
   EXPECT_EQ(0, test_position->text_offset());
 }
 
+TEST_F(AXPositionTest, AsPositionBeforeAndAfterCharacterWithNullPosition) {
+  TestPositionType null_position = AXNodePosition::CreateNullPosition();
+  ASSERT_NE(nullptr, null_position);
+  ASSERT_TRUE(null_position->IsNullPosition());
+  TestPositionType test_position = null_position->AsPositionBeforeCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsNullPosition());
+  test_position = null_position->AsPositionAfterCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsNullPosition());
+}
+
+TEST_F(AXPositionTest, AsPositionBeforeCharacter) {
+  TestPositionType text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, root_.id, 0 /* text_offset */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  TestPositionType test_position = text_position->AsPositionBeforeCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsTextPosition());
+  EXPECT_EQ(inline_box1_.id, test_position->anchor_id());
+  EXPECT_EQ(0, test_position->text_offset());
+
+  text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, inline_box1_.id, 3 /* text_offset */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  test_position = text_position->AsPositionBeforeCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsTextPosition());
+  EXPECT_EQ(inline_box1_.id, test_position->anchor_id());
+  EXPECT_EQ(3, test_position->text_offset());
+
+  text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, line_break_.id, 1 /* text_offset */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  test_position = text_position->AsPositionBeforeCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsTextPosition());
+  EXPECT_EQ(inline_box2_.id, test_position->anchor_id());
+  EXPECT_EQ(0, test_position->text_offset());
+
+  text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, inline_box2_.id, 0 /* text_offset */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  test_position = text_position->AsPositionBeforeCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsTextPosition());
+  EXPECT_EQ(inline_box2_.id, test_position->anchor_id());
+  EXPECT_EQ(0, test_position->text_offset());
+
+  text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, inline_box2_.id, 6 /* text_offset */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  test_position = text_position->AsPositionBeforeCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsNullPosition());
+}
+
+TEST_F(AXPositionTest, AsPositionAfterCharacter) {
+  TestPositionType text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, inline_box1_.id, 0 /* text_offset */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  TestPositionType test_position = text_position->AsPositionAfterCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsNullPosition());
+
+  text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, inline_box1_.id, 5 /* text_offset */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  test_position = text_position->AsPositionAfterCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsTextPosition());
+  EXPECT_EQ(inline_box1_.id, test_position->anchor_id());
+  EXPECT_EQ(5, test_position->text_offset());
+
+  text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, line_break_.id, 1 /* text_offset */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  test_position = text_position->AsPositionAfterCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsTextPosition());
+  EXPECT_EQ(line_break_.id, test_position->anchor_id());
+  EXPECT_EQ(1, test_position->text_offset());
+
+  text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, inline_box2_.id, 0 /* text_offset */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  test_position = text_position->AsPositionAfterCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsTextPosition());
+  EXPECT_EQ(line_break_.id, test_position->anchor_id());
+  EXPECT_EQ(1, test_position->text_offset());
+
+  text_position = AXNodePosition::CreateTextPosition(
+      tree_.data().tree_id, root_.id, 13 /* text_offset */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_NE(nullptr, text_position);
+  ASSERT_TRUE(text_position->IsTextPosition());
+  test_position = text_position->AsPositionAfterCharacter();
+  EXPECT_NE(nullptr, test_position);
+  EXPECT_TRUE(test_position->IsTextPosition());
+  EXPECT_EQ(inline_box2_.id, test_position->anchor_id());
+  EXPECT_EQ(6, test_position->text_offset());
+}
+
 TEST_F(AXPositionTest, CreateNextAndPreviousCharacterPositionWithNullPosition) {
   TestPositionType null_position = AXNodePosition::CreateNullPosition();
   ASSERT_NE(nullptr, null_position);
@@ -1763,7 +1885,7 @@ TEST_F(AXPositionTest, CreateNextCharacterPosition) {
       AXBoundaryBehavior::CrossBoundary);
   EXPECT_NE(nullptr, test_position);
   EXPECT_TRUE(test_position->IsTextPosition());
-  EXPECT_EQ(text_field_.id, test_position->anchor_id());
+  EXPECT_EQ(inline_box1_.id, test_position->anchor_id());
   EXPECT_EQ(1, test_position->text_offset());
   // Affinity should have been reset to downstream.
   EXPECT_EQ(ax::mojom::TextAffinity::kDownstream, test_position->affinity());
@@ -1777,8 +1899,8 @@ TEST_F(AXPositionTest, CreateNextCharacterPosition) {
       AXBoundaryBehavior::CrossBoundary);
   EXPECT_NE(nullptr, test_position);
   EXPECT_TRUE(test_position->IsTextPosition());
-  EXPECT_EQ(text_field_.id, test_position->anchor_id());
-  EXPECT_EQ(13, test_position->text_offset());
+  EXPECT_EQ(inline_box2_.id, test_position->anchor_id());
+  EXPECT_EQ(6, test_position->text_offset());
   // Affinity should have been reset to downstream.
   EXPECT_EQ(ax::mojom::TextAffinity::kDownstream, test_position->affinity());
 }
@@ -1860,7 +1982,7 @@ TEST_F(AXPositionTest, CreatePreviousCharacterPosition) {
       AXBoundaryBehavior::CrossBoundary);
   EXPECT_NE(nullptr, test_position);
   EXPECT_TRUE(test_position->IsTextPosition());
-  EXPECT_EQ(text_field_.id, test_position->anchor_id());
+  EXPECT_EQ(inline_box1_.id, test_position->anchor_id());
   EXPECT_EQ(0, test_position->text_offset());
   // Affinity should have been reset to downstream.
   EXPECT_EQ(ax::mojom::TextAffinity::kDownstream, test_position->affinity());

ui/accessibility/ax_position.h

@@ -796,31 +796,47 @@ class AXPosition {
     return previous_leaf->AsTextPosition();
   }
 
+  // Returns a text position located right before the next character (from this
+  // position) on the tree's text representation, following these conditions:
+  //   - If this position is at the end of its anchor, normalize it to the start
+  //   of the next text anchor. Both positions are equal when compared, but we
+  //   consider the start of an anchor to be a position BEFORE its first
+  //   character and the end to be AFTER its last character.
+  //   - Skip any empty text anchors; they're "invisible" to the text
+  //   representation and the next character could be ahead.
+  //   - Return a null position if there is no next character forward.
+  AXPositionInstance AsPositionBeforeCharacter() const {
+    AXPositionInstance text_position = AsLeafTextPosition();
+
+    // This loop satisfies all the conditions described above.
+    while (text_position->AtEndOfAnchor())
+      text_position = text_position->CreateNextTextAnchorPosition();
+    return text_position;
+  }
+
+  // Returns a text position located right after the previous character (from
+  // this position) on the tree's text representation.
+  // See `AsPositionBeforeCharacter`, as this is its "reversed" version.
+  AXPositionInstance AsPositionAfterCharacter() const {
+    AXPositionInstance text_position = AsLeafTextPosition();
+    while (text_position->AtStartOfAnchor()) {
+      text_position = text_position->CreatePreviousTextAnchorPosition();
+      text_position = text_position->CreatePositionAtEndOfAnchor();
+    }
+    return text_position;
+  }
+
   AXPositionInstance CreateNextCharacterPosition(
       AXBoundaryBehavior boundary_behavior) const {
     DCHECK_NE(boundary_behavior, AXBoundaryBehavior::StopIfAlreadyAtBoundary)
         << "StopIfAlreadyAtBoundary is unreasonable for character boundaries.";
-
     if (boundary_behavior == AXBoundaryBehavior::StopAtAnchorBoundary &&
         AtEndOfAnchor())
       return Clone();
 
     const bool was_tree_position = IsTreePosition();
-    AXPositionInstance text_position = AsTextPosition();
-
-    // Being at the end of a text anchor could mean:
-    //   - We're in the position between this anchor's end and the next text
-    //   anchor's start. Normalize it to the next anchor's start so we can reach
-    //   the next character by simply increasing its offset.
-    //   - We're at an empty anchor. Skip it, such anchor is "invisible" to the
-    //   text representation and the next character could be ahead.
-    //   - We're at the end of the last anchor and there is no next character,
-    //   let CreateNextTextAnchorPosition return a null position.
-    while (text_position->AtEndOfAnchor())
-      text_position = text_position->CreateNextTextAnchorPosition();
-
-    // Either we couldn't get this position as a text position or we reached the
-    // last anchor and there is no next character position to create.
+    AXPositionInstance text_position = AsPositionBeforeCharacter();
+    // There is no next character position.
     if (text_position->IsNullPosition())
       return text_position;
 
@@ -839,29 +855,13 @@ class AXPosition {
       AXBoundaryBehavior boundary_behavior) const {
     DCHECK_NE(boundary_behavior, AXBoundaryBehavior::StopIfAlreadyAtBoundary)
         << "StopIfAlreadyAtBoundary is unreasonable for character boundaries.";
-
     if (boundary_behavior == AXBoundaryBehavior::StopAtAnchorBoundary &&
         AtStartOfAnchor())
       return Clone();
 
     const bool was_tree_position = IsTreePosition();
-    AXPositionInstance text_position = AsTextPosition();
-
-    // Being at the start of a text anchor could mean:
-    //   - We're in the position between this anchor's start and the previous
-    //   text anchor's end. Normalize it to the previous anchor's end so we can
-    //   reach the previous character by simply decreasing its offset.
-    //   - We're at an empty anchor. Skip it, such anchor is "invisible" to the
-    //   text representation and the previous character could be behind.
-    //   - We're at the start of the first anchor and there is no previous
-    //   character, let CreatePreviousTextAnchorPosition return a null position.
-    while (text_position->AtStartOfAnchor()) {
-      text_position = text_position->CreatePreviousTextAnchorPosition();
-      text_position = text_position->CreatePositionAtEndOfAnchor();
-    }
-
-    // Either we couldn't get this position as a text position or we reached the
-    // first anchor and there is no previous character position to create.
+    AXPositionInstance text_position = AsPositionAfterCharacter();
+    // There is no previous character position.
     if (text_position->IsNullPosition())
       return text_position;
 

ui/accessibility/platform/ax_platform_node_textrangeprovider_win.cc

@@ -124,33 +124,49 @@ STDMETHODIMP AXPlatformNodeTextRangeProviderWin::ExpandToEnclosingUnit(
   // is on the next TextUnit boundary, if one exists.
   switch (unit) {
     case TextUnit_Character: {
-      // For characters, start_ will always be on a TextUnit boundary.
-      // Thus, end_ is the only position that needs to be moved.
-      AXNodePosition::AXPositionInstance next =
-          start_->CreateNextCharacterPosition(
+      // For characters, the start endpoint will always be on a TextUnit
+      // boundary, thus we only need to move the end position.
+      AXPositionInstance end_backup = end_->Clone();
+      end_ = start_->CreateNextCharacterPosition(
           ui::AXBoundaryBehavior::CrossBoundary);
-      if (!next->IsNullPosition())
-        end_ = next->Clone();
+
+      if (end_->IsNullPosition()) {
+        // The previous could fail if the start is at the end of the last anchor
+        // of the tree, try expanding to the previous character instead.
+        AXPositionInstance start_backup = start_->Clone();
+        start_ = start_->CreatePreviousCharacterPosition(
+            ui::AXBoundaryBehavior::CrossBoundary);
+
+        if (start_->IsNullPosition()) {
+          // Text representation is empty, undo everything and exit.
+          start_ = std::move(start_backup);
+          end_ = std::move(end_backup);
           return S_OK;
         }
+        end_ = start_->CreateNextCharacterPosition(
+            ui::AXBoundaryBehavior::CrossBoundary);
+        DCHECK(!end_->IsNullPosition());
+      }
+      break;
+    }
     case TextUnit_Format:
       start_ = start_->CreatePreviousFormatStartPosition(
           ui::AXBoundaryBehavior::StopIfAlreadyAtBoundary);
       end_ = start_->CreateNextFormatEndPosition(
           ui::AXBoundaryBehavior::StopIfAlreadyAtBoundary);
-      return S_OK;
+      break;
     case TextUnit_Word:
       start_ = start_->CreatePreviousWordStartPosition(
           ui::AXBoundaryBehavior::StopIfAlreadyAtBoundary);
       end_ = start_->CreateNextWordEndPosition(
           ui::AXBoundaryBehavior::StopIfAlreadyAtBoundary);
-      return S_OK;
+      break;
     case TextUnit_Line:
       start_ = start_->CreatePreviousLineStartPosition(
           ui::AXBoundaryBehavior::StopIfAlreadyAtBoundary);
       end_ = start_->CreateNextLineEndPosition(
           ui::AXBoundaryBehavior::StopIfAlreadyAtBoundary);
-      return S_OK;
+      break;
     case TextUnit_Paragraph:
       return E_NOTIMPL;
     // Since web content is not paginated, TextUnit_Page is not supported.
@@ -159,10 +175,26 @@ STDMETHODIMP AXPlatformNodeTextRangeProviderWin::ExpandToEnclosingUnit(
     case TextUnit_Document:
       start_ = start_->CreatePositionAtStartOfDocument()->AsLeafTextPosition();
       end_ = start_->CreatePositionAtEndOfDocument();
-      return S_OK;
+      break;
     default:
       return UIA_E_NOTSUPPORTED;
   }
+
+  // Some text positions are equal when compared, but they could be located at
+  // different anchors, affecting how `GetEnclosingElement` works. Normalize the
+  // endpoints to correctly enclose characters of the text representation.
+  AXPositionInstance normalized_start = start_->AsPositionBeforeCharacter();
+  AXPositionInstance normalized_end = end_->AsPositionBeforeCharacter();
+
+  if (!normalized_start->IsNullPosition()) {
+    DCHECK_EQ(*start_, *normalized_start);
+    start_ = std::move(normalized_start);
+  }
+  if (!normalized_end->IsNullPosition()) {
+    DCHECK_EQ(*end_, *normalized_end);
+    end_ = std::move(normalized_end);
+  }
+  return S_OK;
 }
 
 STDMETHODIMP AXPlatformNodeTextRangeProviderWin::FindAttribute(

ui/accessibility/platform/ax_platform_node_textrangeprovider_win_unittest.cc

@@ -696,9 +696,10 @@ TEST_F(AXPlatformNodeTextRangeProviderTest,
        TestITextRangeProviderExpandToEnclosingCharacter) {
   Init(BuildTextDocument({"some text", "more text"}));
   AXNodePosition::SetTreeForTesting(tree_.get());
+  AXNode* root_node = GetRootNode();
 
   ComPtr<ITextRangeProvider> text_range_provider;
-  GetTextRangeProviderFromTextNode(text_range_provider, GetRootNode());
+  GetTextRangeProviderFromTextNode(text_range_provider, root_node);
   ASSERT_HRESULT_SUCCEEDED(
       text_range_provider->ExpandToEnclosingUnit(TextUnit_Character));
   EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"s");
@@ -739,16 +740,33 @@ TEST_F(AXPlatformNodeTextRangeProviderTest,
 
   ASSERT_HRESULT_SUCCEEDED(
       text_range_provider->ExpandToEnclosingUnit(TextUnit_Character));
-  EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"");
+  EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"t");
 
-  // Move both endpoints to the position between the end of the first text
-  // anchor and before the start of the next anchor.
+  // Move both endpoints to the position before the start of the "more text"
+  // anchor. Then, force the start to be on the position after the end of
+  // "some text" by moving one character backward and one forward.
   ASSERT_HRESULT_SUCCEEDED(text_range_provider->MoveEndpointByUnit(
       TextPatternRangeEndpoint_End, TextUnit_Character, /*count*/ -9, &count));
   ASSERT_EQ(-9, count);
+  ASSERT_HRESULT_SUCCEEDED(text_range_provider->MoveEndpointByUnit(
+      TextPatternRangeEndpoint_Start, TextUnit_Character, /*count*/ -1,
+      &count));
+  ASSERT_EQ(-1, count);
+  ASSERT_HRESULT_SUCCEEDED(text_range_provider->MoveEndpointByUnit(
+      TextPatternRangeEndpoint_Start, TextUnit_Character, /*count*/ 1, &count));
+  ASSERT_EQ(1, count);
   ASSERT_HRESULT_SUCCEEDED(
       text_range_provider->ExpandToEnclosingUnit(TextUnit_Character));
   EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"m");
+
+  // Check that the enclosing element of the range matches ATs expectations.
+  ComPtr<IRawElementProviderSimple> more_text_provider =
+      QueryInterfaceFromNode<IRawElementProviderSimple>(
+          root_node->children()[1]);
+  ComPtr<IRawElementProviderSimple> enclosing_element;
+  ASSERT_HRESULT_SUCCEEDED(
+      text_range_provider->GetEnclosingElement(&enclosing_element));
+  EXPECT_EQ(more_text_provider.Get(), enclosing_element.Get());
 }
 
 TEST_F(AXPlatformNodeTextRangeProviderTest,