Fix AXRange::GetAnchors from stopping at empty anchors

1. In order to collect the anchors between the start and end positions
   of an `AXRange`, an incorrect condition is evaluating if the start of
   the current anchor is equal to its end position, expecting such case
   to only be present when we reach the end of the `AXRange`.

   Such assumption does not hold for anchors of objects invisible to
   the text representation (such as checkboxes, images, etc.), since the
   anchor is empty and its start and end positions are the same.

   Refactoring `AXRange::GetAnchors` to cover such case.

2. Introducing unit tests for `AXRange::GetAnchors`.

3. Adding unit test coverage in `AXPlatformNodeTextRangeProviderTest`
   for the issue in `GetBoundingRectangles` which led to this bug.

Bug: 928948
Change-Id: I73ef429b7fb5043cfb535ce05909ac243b31aef1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1595545
Commit-Queue: Ethan Jimenez <ethavar@microsoft.com>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659701}

ui/accessibility/ax_range.h

@@ -22,12 +22,13 @@ namespace ui {
 template <class AXPositionType>
 class AXRange {
  public:
+  using AXPositionInstance = std::unique_ptr<AXPositionType>;
+
   AXRange()
       : anchor_(AXPositionType::CreateNullPosition()),
         focus_(AXPositionType::CreateNullPosition()) {}
 
-  AXRange(std::unique_ptr<AXPositionType> anchor,
-          std::unique_ptr<AXPositionType> focus) {
+  AXRange(AXPositionInstance anchor, AXPositionInstance focus) {
     if (anchor) {
       anchor_ = std::move(anchor);
     } else {
@@ -59,6 +60,15 @@ class AXRange {
     return *this;
   }
 
+  bool operator==(const AXRange& other) const {
+    if (IsNull())
+      return other.IsNull();
+    return !other.IsNull() && *anchor_ == *other.anchor() &&
+           *focus_ == *other.focus();
+  }
+
+  bool operator!=(const AXRange& other) const { return !(*this == other); }
+
   virtual ~AXRange() {}
 
   bool IsNull() const {
@@ -76,18 +86,20 @@ class AXRange {
     return focus_.get();
   }
 
+  AXRange GetForwardRange() const {
+    DCHECK(!IsNull());
+    if (*focus_ < *anchor_)
+      return AXRange(focus_->Clone(), anchor_->Clone());
+    return AXRange(anchor_->Clone(), focus_->Clone());
+  }
+
   base::string16 GetText() const {
     if (IsNull() || *anchor_ == *focus_)
       return base::string16();
 
-    std::unique_ptr<AXPositionType> start, end;
-    if (*anchor_ < *focus_) {
-      start = anchor_->AsLeafTextPosition();
-      end = focus_->AsLeafTextPosition();
-    } else {
-      start = focus_->AsLeafTextPosition();
-      end = anchor_->AsLeafTextPosition();
-    }
+    AXRange forward_range = GetForwardRange();
+    AXPositionInstance start = forward_range.anchor()->AsLeafTextPosition();
+    AXPositionInstance end = forward_range.focus()->AsLeafTextPosition();
 
     int start_offset = start->text_offset();
     DCHECK_GE(start_offset, 0);
@@ -114,72 +126,46 @@ class AXRange {
     return text.substr(0, text_length);
   }
 
-  // Returns a vector of AXRanges that span from the start of an anchor
-  // to the end of an anchor, all of which are in between anchor_ and focus_
-  // endpoints of this range.
-  std::vector<AXRange<AXPositionType>> GetAnchors() {
-    DCHECK(*anchor_ <= *focus_);
-
+  // Returns a vector of all ranges (each of them spanning a single anchor)
+  // between the anchor_ and focus_ endpoints of this instance.
+  std::vector<AXRange<AXPositionType>> GetAnchors() const {
     std::vector<AXRange<AXPositionType>> anchors;
-    auto range_start = anchor_->Clone();
-    auto range_end = focus_->Clone();
-
-    // Non-null degenerate ranges span no content, but they do have a single
-    // anchor.
-    auto current_anchor_start = range_start->AsLeafTextPosition();
-    if (!current_anchor_start->IsNullPosition() && *range_start == *range_end) {
-      anchors.emplace_back(AXRange(current_anchor_start->Clone(),
-                                   current_anchor_start->Clone()));
+    if (IsNull())
       return anchors;
-    }
 
-    // If start and end are in the same anchor, use end instead of
-    // CreatePositionAtEndOfAnchor to ensure this doesn't return a range that
-    // spans past end.
-    auto current_anchor_end =
-        current_anchor_start->CreatePositionAtEndOfAnchor();
-    const auto end = range_end->AsLeafTextPosition();
-    if (*current_anchor_end > *end &&
-        end->GetAnchor() == current_anchor_start->GetAnchor())
-      current_anchor_end = end->Clone();
+    AXRange forward_range = GetForwardRange();
+    AXPositionInstance current_anchor_start =
+        forward_range.anchor()->AsLeafTextPosition();
+    AXPositionInstance range_end = forward_range.focus()->AsLeafTextPosition();
 
     while (!current_anchor_start->IsNullPosition() &&
-           !current_anchor_end->IsNullPosition() &&
-           *current_anchor_start < *current_anchor_end) {
-      if (current_anchor_start->GetAnchor() ==
-          current_anchor_end->GetAnchor()) {
-        anchors.emplace_back(AXRange(current_anchor_start->Clone(),
-                                     current_anchor_end->Clone()));
-      }
-
-      if (*current_anchor_end >= *end)
-        break;
-
-      if (current_anchor_end->CreateNextTextAnchorPosition()
-              ->IsNullPosition()) {
-        current_anchor_start = current_anchor_start->CreateNextAnchorPosition()
-                                   ->AsLeafTextPosition();
-      } else {
-        current_anchor_start =
-            current_anchor_end->CreateNextTextAnchorPosition();
+           *current_anchor_start <= *range_end) {
+      // When the current start reaches the same anchor as this AXRange's end,
+      // simply append this last anchor (trimmed at range_end) and exit.
+      if (current_anchor_start->GetAnchor() == range_end->GetAnchor()) {
+        anchors.emplace_back(current_anchor_start->Clone(), range_end->Clone());
+        return anchors;
       }
 
-      current_anchor_end = current_anchor_start->CreatePositionAtEndOfAnchor();
-
-      // If CreatePositionAtEndOfAnchor goes past the end anchor, use the end
-      // anchor instead.
-      if (*current_anchor_end > *end &&
-          end->GetAnchor() == current_anchor_start->GetAnchor())
-        current_anchor_end = end->Clone();
+      AXPositionInstance current_anchor_end =
+          current_anchor_start->CreatePositionAtEndOfAnchor();
+      DCHECK_EQ(current_anchor_start->GetAnchor(),
+                current_anchor_end->GetAnchor());
+      DCHECK_LE(*current_anchor_start, *current_anchor_end);
+      DCHECK_LE(*current_anchor_end, *range_end);
+
+      anchors.emplace_back(current_anchor_start->Clone(),
+                           current_anchor_end->Clone());
+      current_anchor_start =
+          current_anchor_end->CreateNextAnchorPosition()->AsLeafTextPosition();
     }
-
     return anchors;
   }
 
   // Appends rects in screen coordinates of all text nodes that span between
   // anchor_ and focus_. Rects outside of the viewport are skipped.
   std::vector<gfx::Rect> GetScreenRects() const {
-    DCHECK(*anchor_ <= *focus_);
+    DCHECK_LE(*anchor_, *focus_);
     std::vector<gfx::Rect> rectangles;
     auto current_line_start = anchor_->Clone();
     auto range_end = focus_->Clone();
@@ -226,8 +212,8 @@ class AXRange {
   }
 
  private:
-  std::unique_ptr<AXPositionType> anchor_;
-  std::unique_ptr<AXPositionType> focus_;
+  AXPositionInstance anchor_;
+  AXPositionInstance focus_;
 };
 
 }  // namespace ui

ui/accessibility/platform/ax_platform_node_textrangeprovider_win_unittest.cc

@@ -2143,18 +2143,28 @@ TEST_F(AXPlatformNodeTextRangeProviderTest,
   base::win::ScopedSafearray rectangles;
   EXPECT_HRESULT_SUCCEEDED(
       text_range_provider->GetBoundingRectangles(rectangles.Receive()));
-
   std::vector<double> expected_values = {100, 150, 200, 200};
   EXPECT_UIA_DOUBLE_SAFEARRAY_EQ(rectangles.Get(), expected_values);
+  rectangles.Reset();
 
   ComPtr<ITextRangeProvider> document_textrange;
   GetTextRangeProviderFromTextNode(document_textrange, root_node);
 
-  base::win::ScopedSafearray body_rectangles;
   EXPECT_HRESULT_SUCCEEDED(
-      document_textrange->GetBoundingRectangles(body_rectangles.Receive()));
+      document_textrange->GetBoundingRectangles(rectangles.Receive()));
   expected_values = {100, 150, 200, 200, 200, 250, 100, 100};
-  EXPECT_UIA_DOUBLE_SAFEARRAY_EQ(body_rectangles.Get(), expected_values);
+  EXPECT_UIA_DOUBLE_SAFEARRAY_EQ(rectangles.Get(), expected_values);
+  rectangles.Reset();
+
+  EXPECT_UIA_MOVE(document_textrange, TextUnit_Character,
+                  /*count*/ 9,
+                  /*expected_text*/ L"m",
+                  /*expected_count*/ 9);
+
+  EXPECT_HRESULT_SUCCEEDED(
+      document_textrange->GetBoundingRectangles(rectangles.Receive()));
+  expected_values = {200, 250, 100, 100};
+  EXPECT_UIA_DOUBLE_SAFEARRAY_EQ(rectangles.Get(), expected_values);
 
   AXTreeManagerMap::GetInstance().RemoveTreeManager(tree_data.tree_id);
 }