From c25e3e319a71aabf2be0c0157ebf6486522c326e Mon Sep 17 00:00:00 2001
From: Eitan Isaacson <eitan@monotonous.org>
Date: Thu, 16 Mar 2023 15:54:20 +0000
Subject: [PATCH] Bug 1822170 - Handle any kind of NSRanges provided to
 AXAttributedStringForRange. r=Jamie

This patch remedies 4 things:
1. Fix conversion from NSRange to GeckoTextMarkerRange. We were adding the start offset twice. Oops.
2. Clamp given range to actual text size. Since we are messing with the offset fields directly we need to do our own checks here.
3. Don't allow an infinite loop in CachedTextMarkerRange::AttributedText
4. Fix legacy Crop method to be able to take a text leaf.

Differential Revision: https://phabricator.services.mozilla.com/D172594
---
 accessible/mac/CachedTextMarker.mm             |  4 ++++
 accessible/mac/LegacyTextMarker.mm             | 13 ++++++-------
 accessible/mac/mozTextAccessible.mm            |  8 ++++++--
 .../browser/mac/browser_attributed_text.js     | 18 ++++++++++++++++++
 .../tests/browser/mac/browser_text_leaf.js     |  2 +-
 5 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/accessible/mac/CachedTextMarker.mm b/accessible/mac/CachedTextMarker.mm
index ba8a6ea32e5b..aa543a707a91 100644
--- a/accessible/mac/CachedTextMarker.mm
+++ b/accessible/mac/CachedTextMarker.mm
@@ -327,6 +327,10 @@ NSAttributedString* CachedTextMarkerRange::AttributedText() const {
     TextLeafPoint attributesNext;
     do {
       attributesNext = start.FindTextAttrsStart(eDirNext, false);
+      if (attributesNext == start) {
+        MOZ_ASSERT_UNREACHABLE("Cannot proceed further in attribute run");
+        break;
+      }
       RefPtr<AccAttributes> attributes = start.GetTextAttributes();
       MOZ_ASSERT(attributes);
       if (attributes && !attributes->Equal(currentRun)) {
diff --git a/accessible/mac/LegacyTextMarker.mm b/accessible/mac/LegacyTextMarker.mm
index 95ef1800d8b4..7d67913d2700 100644
--- a/accessible/mac/LegacyTextMarker.mm
+++ b/accessible/mac/LegacyTextMarker.mm
@@ -365,24 +365,23 @@ void LegacyTextMarkerRange::Select() const {
 }
 
 bool LegacyTextMarkerRange::Crop(Accessible* aContainer) {
-  LegacyTextMarker containerStart(aContainer, 0);
-  LegacyTextMarker containerEnd(aContainer, CharacterCount(aContainer));
+  LegacyTextMarkerRange containerRange(aContainer);
 
-  if (mEnd < containerStart || containerEnd < mStart) {
+  if (mEnd < containerRange.mStart || containerRange.mEnd < mStart) {
     // The range ends before the container, or starts after it.
     return false;
   }
 
-  if (mStart < containerStart) {
+  if (mStart < containerRange.mStart) {
     // If range start is before container start, adjust range start to
     // start of container.
-    mStart = containerStart;
+    mStart = containerRange.mStart;
   }
 
-  if (containerEnd < mEnd) {
+  if (containerRange.mEnd < mEnd) {
     // If range end is after container end, adjust range end to end of
     // container.
-    mEnd = containerEnd;
+    mEnd = containerRange.mEnd;
   }
 
   return true;
diff --git a/accessible/mac/mozTextAccessible.mm b/accessible/mac/mozTextAccessible.mm
index c193f05dc355..9dbdc8e7b8cf 100644
--- a/accessible/mac/mozTextAccessible.mm
+++ b/accessible/mac/mozTextAccessible.mm
@@ -415,9 +415,13 @@ static GeckoTextMarkerRange TextMarkerSubrange(Accessible* aAccessible,
 
   NSRange r = [aRange rangeValue];
   start.Offset() += r.location;
-  end.Offset() = start.Offset() + r.location + r.length;
+  end.Offset() = start.Offset() + r.length;
 
-  return GeckoTextMarkerRange(start, end);
+  textMarkerRange = GeckoTextMarkerRange(start, end);
+  // Crop range to accessible
+  textMarkerRange.Crop(aAccessible);
+
+  return textMarkerRange;
 }
 
 - (NSString*)moxStringForRange:(NSValue*)range {
diff --git a/accessible/tests/browser/mac/browser_attributed_text.js b/accessible/tests/browser/mac/browser_attributed_text.js
index 6f99bd788554..6f6200751c62 100644
--- a/accessible/tests/browser/mac/browser_attributed_text.js
+++ b/accessible/tests/browser/mac/browser_attributed_text.js
@@ -61,6 +61,24 @@ addAccessibleTask(
       [" ", "#000000", "#ffffff", null, null, null, 16, null, null],
       ["test", "#0000ee", "#ffffff", 1, "#0000ee", null, 16, "a2", null],
     ]);
+
+    // Test different NSRange parameters for AXAttributedStringForRange
+    let worldLeaf = findAccessibleChildByID(accDoc, "a1").firstChild;
+    let wordStaticText = worldLeaf.nativeInterface.QueryInterface(
+      Ci.nsIAccessibleMacInterface
+    );
+    attributedText = wordStaticText.getParameterizedAttributeValue(
+      "AXAttributedStringForRange",
+      NSRange(4, 1)
+    );
+    is(attributedText.length, 1, "Last character is in single attribute run");
+    is(attributedText[0].string, "d", "Last character matches");
+
+    attributedText = wordStaticText.getParameterizedAttributeValue(
+      "AXAttributedStringForRange",
+      NSRange(5, 1)
+    );
+    is(attributedText.length, 0, "Range is past accessible bounds");
   }
 );
 
diff --git a/accessible/tests/browser/mac/browser_text_leaf.js b/accessible/tests/browser/mac/browser_text_leaf.js
index 3bc951489666..21deed62129b 100644
--- a/accessible/tests/browser/mac/browser_text_leaf.js
+++ b/accessible/tests/browser/mac/browser_text_leaf.js
@@ -65,7 +65,7 @@ addAccessibleTask(
       NSRange(3, 6)
     );
 
-    is(str, "lo, this ", "AXStringForRange matches.");
+    is(str, "lo, th", "AXStringForRange matches.");
 
     let smallBounds = textLeaf.getParameterizedAttributeValue(
       "AXBoundsForRange",