Bug 1901457 part 2: Refactor TextLeafPoint caret retrieval so that the end of line insertion point can be stored without being tied to the current caret position. r=nlapre

Previously, TextLeafPoint::GetCaret() returned a placeholder representation of the caret.
ActualizeCaret() was then used to get the real caret position, optionally adjusting for the line end depending on the caller's needs.
This causes problems when we need to store the TextLeafPoint information in a UIA text range object because the only way to indicate the line end insertion point is to use the GetCaret() placeholder, but that also means the stored position changes if the caret moves.
For example, if you disabled NVDA's "caret moves review cursor" setting and then moved the caret, the review cursor should stay at the old position, but it didn't previously.

To fix this:

1. TextLeafPoint now has a new mIsEndOfLineInsertionPoint flag which is set to true by GetCaret() if appropriate.
2. GetCaret() sets mAcc and mOffset immediately, rather than needing to call ActualizeCaret() later.
3. TextLeafPoint methods still need to adjust the point to correctly handle the line end insertion point in some cases, but that is now handled by the private TextLeafPoint::AdjustEndOfLine method.
4. FindBoundary now correctly returns the previous character/cluster when requested for this end of line insertion point. Strictly speaking, this bug always existed, but no existing callers ever triggered it.

Differential Revision: https://phabricator.services.mozilla.com/D214342

accessible/android/Platform.cpp

@@ -162,7 +162,7 @@ void a11y::PlatformCaretMoveEvent(Accessible* aTarget, int32_t aOffset,
   if (!aTarget->IsDoc() && !aFromUser && !aIsSelectionCollapsed) {
     // Pivot to the caret's position if it has an expanded selection.
     // This is used mostly for find in page.
-    Accessible* leaf = TextLeafPoint::GetCaret(aTarget).ActualizeCaret().mAcc;
+    Accessible* leaf = TextLeafPoint::GetCaret(aTarget).mAcc;
     MOZ_ASSERT(leaf);
     if (leaf) {
       if (Accessible* result = AccessibleWrap::DoPivot(

accessible/base/TextLeafRange.cpp

@@ -995,30 +995,16 @@ TextLeafPoint TextLeafPoint::FindNextWordStartSameAcc(
   return TextLeafPoint(mAcc, wordStart);
 }
 
-bool TextLeafPoint::IsCaretAtEndOfLine() const {
-  MOZ_ASSERT(IsCaret());
-  if (LocalAccessible* acc = mAcc->AsLocal()) {
-    HyperTextAccessible* ht = HyperTextFor(acc);
-    if (!ht) {
-      return false;
-    }
-    // Use HyperTextAccessible::IsCaretAtEndOfLine. Eventually, we'll want to
-    // move that code into TextLeafPoint, but existing code depends on it living
-    // in HyperTextAccessible (including caret events).
-    return ht->IsCaretAtEndOfLine();
-  }
-  return mAcc->AsRemote()->Document()->IsCaretAtEndOfLine();
-}
-
-TextLeafPoint TextLeafPoint::ActualizeCaret(bool aAdjustAtEndOfLine) const {
-  MOZ_ASSERT(IsCaret());
+/* static */
+TextLeafPoint TextLeafPoint::GetCaret(Accessible* aAcc) {
   HyperTextAccessibleBase* ht;
   int32_t htOffset;
-  if (LocalAccessible* acc = mAcc->AsLocal()) {
+  bool isEndOfLine;
+  if (LocalAccessible* localAcc = aAcc->AsLocal()) {
     // Use HyperTextAccessible::CaretOffset. Eventually, we'll want to move
     // that code into TextLeafPoint, but existing code depends on it living in
     // HyperTextAccessible (including caret events).
-    ht = HyperTextFor(acc);
+    ht = HyperTextFor(localAcc);
     if (!ht) {
       return TextLeafPoint();
     }
@@ -1026,38 +1012,56 @@ TextLeafPoint TextLeafPoint::ActualizeCaret(bool aAdjustAtEndOfLine) const {
     if (htOffset == -1) {
       return TextLeafPoint();
     }
+    // Use HyperTextAccessible::IsCaretAtEndOfLine. Eventually, we'll want to
+    // move that code into TextLeafPoint, but existing code depends on it living
+    // in HyperTextAccessible (including caret events).
+    isEndOfLine = localAcc->AsHyperText()->IsCaretAtEndOfLine();
   } else {
     // Ideally, we'd cache the caret as a leaf, but our events are based on
     // HyperText for now.
-    std::tie(ht, htOffset) = mAcc->AsRemote()->Document()->GetCaret();
+    DocAccessibleParent* remoteDoc = aAcc->AsRemote()->Document();
+    std::tie(ht, htOffset) = remoteDoc->GetCaret();
     if (!ht) {
       return TextLeafPoint();
     }
+    isEndOfLine = remoteDoc->IsCaretAtEndOfLine();
   }
-  if (aAdjustAtEndOfLine && htOffset > 0 && IsCaretAtEndOfLine()) {
-    // It is the same character offset when the caret is visually at the very
-    // end of a line or the start of a new line (soft line break). Getting text
-    // at the line should provide the line with the visual caret. Otherwise,
-    // screen readers will announce the wrong line as the user presses up or
-    // down arrow and land at the end of a line.
-    --htOffset;
-  }
-  return ht->ToTextLeafPoint(htOffset);
+  TextLeafPoint point = ht->ToTextLeafPoint(htOffset);
+  point.mIsEndOfLineInsertionPoint = isEndOfLine;
+  return point;
+}
+
+TextLeafPoint TextLeafPoint::AdjustEndOfLine() const {
+  MOZ_ASSERT(mIsEndOfLineInsertionPoint);
+  // Use the last character on the line so that we search for word and line
+  // boundaries on the current line, not the next line.
+  return TextLeafPoint(mAcc, mOffset)
+      .FindBoundary(nsIAccessibleText::BOUNDARY_CHAR, eDirPrevious);
 }
 
 TextLeafPoint TextLeafPoint::FindBoundary(AccessibleTextBoundary aBoundaryType,
                                           nsDirection aDirection,
                                           BoundaryFlags aFlags) const {
-  if (IsCaret()) {
+  if (mIsEndOfLineInsertionPoint) {
+    // In this block, we deliberately don't propagate mIsEndOfLineInsertionPoint
+    // to derived points because otherwise, a call to FindBoundary on the
+    // returned point would also return the same point.
     if (aBoundaryType == nsIAccessibleText::BOUNDARY_CHAR ||
         aBoundaryType == nsIAccessibleText::BOUNDARY_CLUSTER) {
-      if (IsCaretAtEndOfLine()) {
-        // The caret is at the end of the line. Return no character.
-        return ActualizeCaret(/* aAdjustAtEndOfLine */ false);
+      if (aDirection == eDirNext || (aDirection == eDirPrevious &&
+                                     aFlags & BoundaryFlags::eIncludeOrigin)) {
+        // The caller wants the current or next character/cluster. Return no
+        // character, since otherwise, this would move past the first character
+        // on the next line.
+        return TextLeafPoint(mAcc, mOffset);
       }
+      // The caller wants the previous character/cluster. Return that as normal.
+      return TextLeafPoint(mAcc, mOffset)
+          .FindBoundary(aBoundaryType, aDirection, aFlags);
     }
-    return ActualizeCaret().FindBoundary(
-        aBoundaryType, aDirection, aFlags & BoundaryFlags::eIncludeOrigin);
+    // For any other boundary, we need to start on this line, not the next, even
+    // though mOffset refers to the next.
+    return AdjustEndOfLine().FindBoundary(aBoundaryType, aDirection, aFlags);
   }
 
   bool inEditableAndStopInIt = (aFlags & BoundaryFlags::eStopInEditable) &&
@@ -1746,8 +1750,8 @@ already_AddRefed<AccAttributes> TextLeafPoint::GetTextAttributes(
 
 TextLeafPoint TextLeafPoint::FindTextAttrsStart(nsDirection aDirection,
                                                 bool aIncludeOrigin) const {
-  if (IsCaret()) {
-    return ActualizeCaret().FindTextAttrsStart(aDirection, aIncludeOrigin);
+  if (mIsEndOfLineInsertionPoint) {
+    return AdjustEndOfLine().FindTextAttrsStart(aDirection, aIncludeOrigin);
   }
   const bool isRemote = mAcc->IsRemote();
   RefPtr<const AccAttributes> lastAttrs =

accessible/base/TextLeafRange.h

@@ -45,17 +45,24 @@ class TextLeafPoint final {
 
   /**
    * Construct a TextLeafPoint representing the caret.
-   * The actual offset used for the caret differs depending on whether the
-   * caret is at the end of a line and the query being made. Thus, mOffset on
-   * the returned TextLeafPoint is not a valid offset.
    */
-  static TextLeafPoint GetCaret(Accessible* aAcc) {
-    return TextLeafPoint(aAcc, nsIAccessibleText::TEXT_OFFSET_CARET);
-  }
+  static TextLeafPoint GetCaret(Accessible* aAcc);
 
   Accessible* mAcc;
   int32_t mOffset;
 
+  /**
+   * True if this point is the insertion point at the end of a line. This is the
+   * point where the caret is positioned when pressing the end key, for example.
+   * On the very last line, mOffset will be equal to the length of the text.
+   * However, where text wraps across lines, this line end insertion point
+   * doesn't have its own offset, so mOffset will be the offset for the first
+   * character on the next line. This is where this flag becomes important.
+   * Otherwise, for example, commanding a screen reader to read the current line
+   * would read the next line instead of the current line in this case.
+   */
+  bool mIsEndOfLineInsertionPoint = false;
+
   bool operator==(const TextLeafPoint& aPoint) const {
     return mAcc == aPoint.mAcc && mOffset == aPoint.mOffset;
   }
@@ -74,21 +81,6 @@ class TextLeafPoint final {
    */
   explicit operator bool() const { return !!mAcc; }
 
-  bool IsCaret() const {
-    return mOffset == nsIAccessibleText::TEXT_OFFSET_CARET;
-  }
-
-  bool IsCaretAtEndOfLine() const;
-
-  /**
-   * Get a TextLeafPoint at the actual caret offset.
-   * This should only be called on a TextLeafPoint created with GetCaret.
-   * If aAdjustAtEndOfLine is true, the point will be adjusted if the caret is
-   * at the end of a line so that word and line boundaries can be calculated
-   * correctly.
-   */
-  TextLeafPoint ActualizeCaret(bool aAdjustAtEndOfLine = true) const;
-
   enum class BoundaryFlags : uint32_t {
     eDefaultBoundaryFlags = 0,
     // Return point unchanged if it is at the given boundary type.
@@ -202,6 +194,12 @@ class TextLeafPoint final {
       bool aIncludeGenerated = true) const;
 
  private:
+  /**
+   * If this is the insertion point at the end of a line, return an adjusted
+   * point such that word and line boundaries can be calculated correctly.
+   */
+  TextLeafPoint AdjustEndOfLine() const;
+
   bool IsEmptyLastLine() const;
 
   bool IsDocEdge(nsDirection aDirection) const;

accessible/basetypes/HyperTextAccessibleBase.cpp

@@ -404,9 +404,11 @@ void HyperTextAccessibleBase::AdjustOriginIfEndBoundary(
       aBoundaryType != nsIAccessibleText::BOUNDARY_WORD_END) {
     return;
   }
-  TextLeafPoint actualOrig =
-      aOrigin.IsCaret() ? aOrigin.ActualizeCaret(/* aAdjustAtEndOfLine */ false)
-                        : aOrigin;
+  TextLeafPoint actualOrig = aOrigin;
+  // We explicitly care about the character at this offset. We don't want
+  // FindBoundary to behave differently even if this is the insertion point at
+  // the end of a line.
+  actualOrig.mIsEndOfLineInsertionPoint = false;
   if (aBoundaryType == nsIAccessibleText::BOUNDARY_LINE_END) {
     if (!actualOrig.IsLineFeedChar()) {
       return;
@@ -420,7 +422,7 @@ void HyperTextAccessibleBase::AdjustOriginIfEndBoundary(
       // boundary. Also, if the caret is at the end of a line, our tests expect
       // the word after the caret, not the word before. The reason for that
       // is a mystery lost to history. We can do that by explicitly using the
-      // actualized caret without adjusting for end of line.
+      // caret without adjusting for end of line.
       aOrigin = actualOrig;
       return;
     }
@@ -513,7 +515,7 @@ void HyperTextAccessibleBase::TextAtOffset(int32_t aOffset,
   if (aBoundaryType == nsIAccessibleText::BOUNDARY_CHAR) {
     if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET) {
       TextLeafPoint caret = TextLeafPoint::GetCaret(Acc());
-      if (caret.IsCaretAtEndOfLine()) {
+      if (caret.mIsEndOfLineInsertionPoint) {
         // The caret is at the end of the line. Return no character.
         *aStartOffset = *aEndOffset = static_cast<int32_t>(adjustedOffset);
         return;
@@ -581,7 +583,7 @@ void HyperTextAccessibleBase::TextAfterOffset(
 
   if (aBoundaryType == nsIAccessibleText::BOUNDARY_CHAR) {
     if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET && adjustedOffset > 0 &&
-        TextLeafPoint::GetCaret(Acc()).IsCaretAtEndOfLine()) {
+        TextLeafPoint::GetCaret(Acc()).mIsEndOfLineInsertionPoint) {
       --adjustedOffset;
     }
     uint32_t count = CharacterCount();
@@ -624,8 +626,7 @@ void HyperTextAccessibleBase::TextAfterOffset(
 }
 
 int32_t HyperTextAccessibleBase::CaretOffset() const {
-  TextLeafPoint point = TextLeafPoint::GetCaret(const_cast<Accessible*>(Acc()))
-                            .ActualizeCaret(/* aAdjustAtEndOfLine */ false);
+  TextLeafPoint point = TextLeafPoint::GetCaret(const_cast<Accessible*>(Acc()));
   if (point.mOffset == 0 && point.mAcc == Acc()) {
     // If a text box is empty, there will be no children, so point.mAcc will be
     // this HyperText.
@@ -641,8 +642,7 @@ int32_t HyperTextAccessibleBase::CaretOffset() const {
 }
 
 int32_t HyperTextAccessibleBase::CaretLineNumber() {
-  TextLeafPoint point = TextLeafPoint::GetCaret(const_cast<Accessible*>(Acc()))
-                            .ActualizeCaret(/* aAdjustAtEndOfLine */ false);
+  TextLeafPoint point = TextLeafPoint::GetCaret(const_cast<Accessible*>(Acc()));
   if (point.mOffset == 0 && point.mAcc == Acc()) {
     MOZ_ASSERT(CharacterCount() == 0);
     // If a text box is empty, there will be no children, so point.mAcc will be