Bug 1482147 - Simplify nsTypeAheadFind visibility code to not lie. r=masayuki

Multiple issues here. The IsRangeVisible code was wrong, it was returning false
for ranges that were perfectly valid, but outside the viewport, because the
following piece of code:

 if (!aMustBeInViewPort) {
   // This is an early exit case because we don't care that that range
   // is out of viewport, so we return that the range is "visible".
   return true;
 }

Was incorrectly after some stuff checking viewport visibility. This code is
pretty complex for no good reason, it wants to do something very
simple: Start from the visible selection if possible.

This patch still achieves this, using IsRangeRendered (which does a proper
hit-test to figure out if a range is in the viewport). Should have no behavior
differences except for non-collapsed ranges that are partially inside the
viewport.

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

dom/base/test/test_find.html

@@ -84,6 +84,14 @@ let runTests = t.step_func_done(function() {
     </div>
   `);
 
+  testFindable(true, "This text should be visible", `
+    <style>:root { overflow: hidden }</style>
+    <div style="overflow: auto;">
+      <div style="height: 300vh"></div>
+      This text should be visible
+    </div>
+  `);
+
   testFindable(true, "Shadow text", function(document) {
     let div = document.createElement("div");
     div.attachShadow({ mode: "open" }).innerHTML = `

toolkit/components/typeaheadfind/nsTypeAheadFind.cpp

@@ -69,8 +69,6 @@ NS_IMPL_CYCLE_COLLECTION_WEAK(nsTypeAheadFind, mFoundLink, mFoundEditable,
                               mStartPointRange, mEndPointRange, mSoundInterface,
                               mFind, mFoundRange)
 
-static NS_DEFINE_CID(kFrameTraversalCID, NS_FRAMETRAVERSAL_CID);
-
 #define NS_FIND_CONTRACTID "@mozilla.org/embedcomp/rangefind;1"
 
 nsTypeAheadFind::nsTypeAheadFind()
@@ -464,14 +462,15 @@ nsresult nsTypeAheadFind::FindItNow(uint32_t aMode, bool aIsLinksOnly,
         RangeStartsInsideLink(returnRange, &isInsideLink, &isStartingLink);
       }
 
-      bool usesIndependentSelection;
+      bool usesIndependentSelection = false;
       // Check actual visibility of the range, and generate some
       // side effects (like updating mStartPointRange and
       // setting usesIndependentSelection) that we'll need whether
       // or not the range is visible.
       bool canSeeRange = IsRangeVisible(returnRange, aIsFirstVisiblePreferred,
-                                        false, getter_AddRefs(mStartPointRange),
+                                        false, &usesIndependentSelection);
-                                        &usesIndependentSelection);
+
+      mStartPointRange = returnRange->CloneRange();
 
       // If we can't see the range, we still might be able to scroll
       // it into view if usesIndependentSelection is true. If both are
@@ -479,41 +478,9 @@ nsresult nsTypeAheadFind::FindItNow(uint32_t aMode, bool aIsLinksOnly,
       if ((!canSeeRange && !usesIndependentSelection) ||
           (aIsLinksOnly && !isInsideLink) ||
           (mStartLinksOnlyPref && aIsLinksOnly && !isStartingLink)) {
-        // ------ Failure ------
+        // We want to jump over this range, so collapse to the start if we're
-        // At this point mStartPointRange got updated to the first
+        // finding backwards and vice versa.
-        // visible range in the viewport.  We _may_ be able to just
+        mStartPointRange->Collapse(findPrev);
-        // start there, if it's not taking us in the wrong direction.
-        if (findPrev) {
-          // We can continue at the end of mStartPointRange if its end is before
-          // the start of returnRange or coincides with it.  Otherwise, we need
-          // to continue at the start of returnRange.
-          IgnoredErrorResult rv;
-          int16_t compareResult = mStartPointRange->CompareBoundaryPoints(
-              Range_Binding::START_TO_END, *returnRange, rv);
-          if (!rv.Failed() && compareResult <= 0) {
-            // OK to start at the end of mStartPointRange
-            mStartPointRange->Collapse(false);
-          } else {
-            // Start at the beginning of returnRange
-            mStartPointRange = returnRange->CloneRange();
-            mStartPointRange->Collapse(true);
-          }
-        } else {
-          // We can continue at the start of mStartPointRange if its start is
-          // after the end of returnRange or coincides with it.  Otherwise, we
-          // need to continue at the end of returnRange.
-          IgnoredErrorResult rv;
-          int16_t compareResult = mStartPointRange->CompareBoundaryPoints(
-              Range_Binding::END_TO_START, *returnRange, rv);
-          if (!rv.Failed() && compareResult >= 0) {
-            // OK to start at the start of mStartPointRange
-            mStartPointRange->Collapse(true);
-          } else {
-            // Start at the end of returnRange
-            mStartPointRange = returnRange->CloneRange();
-            mStartPointRange->Collapse(false);
-          }
-        }
         continue;
       }
 
@@ -821,11 +788,7 @@ nsresult nsTypeAheadFind::GetSearchContainers(
   }
 
   if (!currentSelectionRange) {
-    // Ensure visible range, move forward if necessary
+    mStartPointRange = mSearchRange->CloneRange();
-    // This ignores the return value, but uses the side effect of
-    // IsRangeVisible. It returns the first visible range after searchRange
-    IsRangeVisible(mSearchRange, aIsFirstVisiblePreferred, true,
-                   getter_AddRefs(mStartPointRange), nullptr);
     // We want to search in the visible selection range. That means that the
     // start point needs to be the end if we're looking backwards, or vice
     // versa.
@@ -867,7 +830,7 @@ void nsTypeAheadFind::RangeStartsInsideLink(nsRange* aRange,
   uint32_t startOffset = aRange->StartOffset();
 
   nsCOMPtr<nsIContent> startContent =
-      do_QueryInterface(aRange->GetStartContainer());
+      nsIContent::FromNodeOrNull(aRange->GetStartContainer());
   if (!startContent) {
     MOZ_ASSERT_UNREACHABLE("startContent should never be null");
     return;
@@ -1148,116 +1111,17 @@ nsTypeAheadFind::GetFoundRange(nsRange** aFoundRange) {
 NS_IMETHODIMP
 nsTypeAheadFind::IsRangeVisible(nsRange* aRange, bool aMustBeInViewPort,
                                 bool* aResult) {
-  RefPtr<nsRange> ignored;
+  *aResult = IsRangeVisible(aRange, aMustBeInViewPort, false, nullptr);
-  *aResult = IsRangeVisible(aRange, aMustBeInViewPort, false,
-                            getter_AddRefs(ignored), nullptr);
   return NS_OK;
 }
 
-enum class RectVisibility {
-  Visible,
-  AboveViewport,
-  BelowViewport,
-  LeftOfViewport,
-  RightOfViewport,
-};
-
-/**
- * Determine if a rectangle specified in the frame's coordinate system
- * intersects "enough" with the viewport to be considered visible. This
- * is not a strict test against the viewport -- it's a test against
- * the intersection of the viewport and the frame's ancestor scrollable
- * frames. If it doesn't intersect enough, return a value indicating
- * which direction the frame's topmost ancestor scrollable frame would
- * need to be scrolled to bring the frame into view.
- * @param aFrame frame that aRect coordinates are specified relative to
- * @param aRect rectangle in twips to test for visibility
- * @param aMinTwips is the minimum distance in from the edge of the
- *                  visible area that an object must be to be counted
- *                  visible
- * @return RectVisibility::Visible if the rect is visible
- *         RectVisibility::AboveViewport
- *         RectVisibility::BelowViewport
- *         RectVisibility::LeftOfViewport
- *         RectVisibility::RightOfViewport rectangle is outside the
- *         topmost ancestor scrollable frame in the specified direction
- */
-static RectVisibility GetRectVisibility(nsIFrame* aFrame, const nsRect& aRect,
-                                        nscoord aMinTwips) {
-  PresShell* ps = aFrame->PresShell();
-  nsIFrame* rootFrame = ps->GetRootFrame();
-  MOZ_DIAGNOSTIC_ASSERT(
-      rootFrame,
-      "How can someone have a frame for this presshell when there's no root?");
-  nsIScrollableFrame* sf = ps->GetRootScrollFrameAsScrollable();
-  nsRect scrollPortRect;
-  if (sf) {
-    scrollPortRect = sf->GetScrollPortRect();
-    nsIFrame* f = do_QueryFrame(sf);
-    scrollPortRect += f->GetOffsetTo(rootFrame);
-  } else {
-    scrollPortRect = nsRect(nsPoint(0, 0), rootFrame->GetSize());
-  }
-
-  // scrollPortRect has the viewport visible area relative to rootFrame.
-  nsRect visibleAreaRect(scrollPortRect);
-  // Find the intersection of this and the frame's ancestor scrollable
-  // frames. We walk the whole ancestor chain to find all the scrollable
-  // frames.
-  nsIScrollableFrame* scrollAncestorFrame =
-      nsLayoutUtils::GetNearestScrollableFrame(
-          aFrame, nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN);
-  while (scrollAncestorFrame) {
-    nsRect scrollAncestorRect = scrollAncestorFrame->GetScrollPortRect();
-    nsIFrame* f = do_QueryFrame(scrollAncestorFrame);
-    scrollAncestorRect += f->GetOffsetTo(rootFrame);
-
-    visibleAreaRect = visibleAreaRect.Intersect(scrollAncestorRect);
-
-    // Continue up the chain.
-    scrollAncestorFrame = nsLayoutUtils::GetNearestScrollableFrame(
-        f->GetParent(), nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN);
-  }
-
-  // aRect is in the aFrame coordinate space, so bring it into rootFrame
-  // coordinate space.
-  nsRect r = aRect + aFrame->GetOffsetTo(rootFrame);
-  // If aRect is entirely visible then we don't need to ensure that
-  // at least aMinTwips of it is visible
-  if (visibleAreaRect.Contains(r)) {
-    return RectVisibility::Visible;
-  }
-
-  nsRect insetRect = visibleAreaRect;
-  insetRect.Deflate(aMinTwips, aMinTwips);
-  if (r.YMost() <= insetRect.y) {
-    return RectVisibility::AboveViewport;
-  }
-  if (r.y >= insetRect.YMost()) {
-    return RectVisibility::BelowViewport;
-  }
-  if (r.XMost() <= insetRect.x) {
-    return RectVisibility::LeftOfViewport;
-  }
-  if (r.x >= insetRect.XMost()) {
-    return RectVisibility::RightOfViewport;
-  }
-  return RectVisibility::Visible;
-}
-
 bool nsTypeAheadFind::IsRangeVisible(nsRange* aRange, bool aMustBeInViewPort,
                                      bool aGetTopVisibleLeaf,
-                                     nsRange** aFirstVisibleRange,
                                      bool* aUsesIndependentSelection) {
-  NS_ASSERTION(aRange && aFirstVisibleRange, "params are invalid");
-
   // We need to know if the range start is visible.
-  // Otherwise, return the first visible range start
+  // Otherwise, return the first visible range start in aFirstVisibleRange
-  // in aFirstVisibleRange
+  nsCOMPtr<nsIContent> content =
-
+      nsIContent::FromNodeOrNull(aRange->GetStartContainer());
-  *aFirstVisibleRange = aRange->CloneRange().take();
-
-  nsCOMPtr<nsIContent> content = do_QueryInterface(aRange->GetStartContainer());
   if (!content) {
     return false;
   }
@@ -1278,139 +1142,7 @@ bool nsTypeAheadFind::IsRangeVisible(nsRange* aRange, bool aMustBeInViewPort,
         (frame->GetStateBits() & NS_FRAME_INDEPENDENT_SELECTION);
   }
 
-  // ---- We have a frame ----
+  return aMustBeInViewPort ? IsRangeRendered(aRange) : true;
-
-  // Get the next in flow frame that contains the range start
-  int32_t startFrameOffset, endFrameOffset;
-  uint32_t startRangeOffset = aRange->StartOffset();
-  while (true) {
-    frame->GetOffsets(startFrameOffset, endFrameOffset);
-    if (static_cast<int32_t>(startRangeOffset) < endFrameOffset) {
-      break;
-    }
-
-    nsIFrame* nextContinuationFrame = frame->GetNextContinuation();
-    if (nextContinuationFrame)
-      frame = nextContinuationFrame;
-    else
-      break;
-  }
-
-  // Set up the variables we need, return true if we can't get at them all
-  const uint16_t kMinPixels = 12;
-  nscoord minDistance = nsPresContext::CSSPixelsToAppUnits(kMinPixels);
-
-  // Get the bounds of the current frame, relative to the current view.
-  // We don't use the more accurate AccGetBounds, because that is
-  // more expensive and the STATE_OFFSCREEN flag that this is used
-  // for only needs to be a rough indicator
-  RectVisibility rectVisibility = RectVisibility::AboveViewport;
-
-  if (!aGetTopVisibleLeaf && !frame->GetRect().IsEmpty()) {
-    rectVisibility =
-        GetRectVisibility(frame, frame->GetRectRelativeToSelf(), minDistance);
-
-    if (rectVisibility == RectVisibility::Visible) {
-      // The primary frame of the range is visible, but we don't yet know if
-      // any of the rects of the range itself are visible. Check to see if at
-      // least one of the rects is visible.
-      bool atLeastOneRangeRectVisible = false;
-
-      nsIFrame* containerFrame =
-          nsLayoutUtils::GetContainingBlockForClientRect(frame);
-      RefPtr<DOMRectList> rects = aRange->GetClientRects(true, true);
-      for (uint32_t i = 0; i < rects->Length(); ++i) {
-        RefPtr<DOMRect> rect = rects->Item(i);
-        nsRect r(nsPresContext::CSSPixelsToAppUnits((float)rect->X()),
-                 nsPresContext::CSSPixelsToAppUnits((float)rect->Y()),
-                 nsPresContext::CSSPixelsToAppUnits((float)rect->Width()),
-                 nsPresContext::CSSPixelsToAppUnits((float)rect->Height()));
-
-        // r is relative to containerFrame; transform it back to frame, so we
-        // can do a proper visibility check that is cropped to all of frame's
-        // ancestor scroll frames.
-        nsLayoutUtils::TransformResult res =
-            nsLayoutUtils::TransformRect(containerFrame, frame, r);
-        if (res == nsLayoutUtils::TransformResult::TRANSFORM_SUCCEEDED) {
-          RectVisibility rangeRectVisibility =
-              GetRectVisibility(frame, r, minDistance);
-
-          if (rangeRectVisibility == RectVisibility::Visible) {
-            atLeastOneRangeRectVisible = true;
-            break;
-          }
-        }
-      }
-
-      if (atLeastOneRangeRectVisible) {
-        // This is an early exit case, where we return true if and only if
-        // the range is actually rendered.
-        return IsRangeRendered(aRange);
-      }
-    }
-  }
-
-  // Below this point, we know the range is not in the viewport.
-
-  if (!aMustBeInViewPort) {
-    // This is an early exit case because we don't care that that range
-    // is out of viewport, so we return that the range is "visible".
-    return true;
-  }
-
-  // The range isn't in the viewport, but we could scroll it into view.
-  // Move range forward to first visible point,
-  // this speeds us up a lot in long documents
-  nsCOMPtr<nsIFrameEnumerator> frameTraversal;
-  nsCOMPtr<nsIFrameTraversal> trav(do_CreateInstance(kFrameTraversalCID));
-  if (trav)
-    trav->NewFrameTraversal(getter_AddRefs(frameTraversal),
-                            frame->PresContext(), frame, eLeaf,
-                            false,  // aVisual
-                            false,  // aLockInScrollView
-                            false,  // aFollowOOFs
-                            false   // aSkipPopupChecks
-    );
-
-  if (!frameTraversal) {
-    return false;
-  }
-
-  while (rectVisibility == RectVisibility::AboveViewport) {
-    frameTraversal->Next();
-    frame = frameTraversal->CurrentItem();
-    if (!frame) {
-      return false;
-    }
-
-    // We don't really want to start on NAC, because we may skip iterating
-    // actual non-anonymous children that matter.
-    while (frame->GetContent() &&
-           frame->GetContent()->IsInNativeAnonymousSubtree()) {
-      frame = frame->GetParent();
-    }
-
-    if (frame->GetRect().IsEmpty()) {
-      continue;
-    }
-
-    rectVisibility = GetRectVisibility(
-        frame, nsRect(nsPoint(0, 0), frame->GetSize()), minDistance);
-  }
-
-  if (frame) {
-    nsINode* firstVisibleNode = frame->GetContent();
-
-    if (firstVisibleNode) {
-      frame->GetOffsets(startFrameOffset, endFrameOffset);
-      (*aFirstVisibleRange)
-          ->SetStart(*firstVisibleNode, startFrameOffset, IgnoreErrors());
-      (*aFirstVisibleRange)
-          ->SetEnd(*firstVisibleNode, endFrameOffset, IgnoreErrors());
-    }
-  }
-
-  return false;
 }
 
 NS_IMETHODIMP
@@ -1422,7 +1154,7 @@ nsTypeAheadFind::IsRangeRendered(nsRange* aRange, bool* aResult) {
 bool nsTypeAheadFind::IsRangeRendered(nsRange* aRange) {
   using FrameForPointOption = nsLayoutUtils::FrameForPointOption;
   nsCOMPtr<nsIContent> content =
-      do_QueryInterface(aRange->GetClosestCommonInclusiveAncestor());
+      nsIContent::FromNodeOrNull(aRange->GetClosestCommonInclusiveAncestor());
   if (!content) {
     return false;
   }

toolkit/components/typeaheadfind/nsTypeAheadFind.h

@@ -61,11 +61,8 @@ class nsTypeAheadFind : public nsITypeAheadFind,
   void GetSelection(mozilla::PresShell* aPresShell,
                     nsISelectionController** aSelCon,
                     mozilla::dom::Selection** aDomSel);
-  // *aNewRange may not be collapsed.  If you want to collapse it in a
-  // particular way, you need to do it yourself.
   bool IsRangeVisible(nsRange* aRange, bool aMustBeVisible,
-                      bool aGetTopVisibleLeaf, nsRange** aNewRange,
+                      bool aGetTopVisibleLeaf, bool* aUsesIndependentSelection);
-                      bool* aUsesIndependentSelection);
   bool IsRangeRendered(nsRange* aRange);
   MOZ_CAN_RUN_SCRIPT_BOUNDARY
   nsresult FindItNow(uint32_t aMode, bool aIsLinksOnly,