Bug 1436431 Part 2: Change nsTypeAheadFind::IsRangeVisible to additionally check for visibility of range rects; not just the range's primary frame. draft
authorBrad Werth <bwerth@mozilla.com>
Wed, 25 Apr 2018 12:28:57 -0700
changeset 790313 0e7a7a22cf18e9721bf89f8064770b53c48e6283
parent 790312 f505ccea8d917e45611a9c4f89d7042f07628c13
child 790314 b62cce2bb2727c389642b50d115c4bacbc788bbd
push id108493
push userbwerth@mozilla.com
push dateTue, 01 May 2018 23:23:11 +0000
bugs1436431
milestone61.0a1
Bug 1436431 Part 2: Change nsTypeAheadFind::IsRangeVisible to additionally check for visibility of range rects; not just the range's primary frame. MozReview-Commit-ID: EQFyRswqy5L Since a range rect can be considerably smaller than the rect of its containing frame, this change avoids an unpleasant false-negative for find-in-page. Without this change, if a frame is reported as visible, but none of the range rects are visible, the later call to isRangeRendered will report the text as not findable, even though it could be scrolled into view. With this change, the text in such a case is reported as findable, but just out of view, and the find-in-page logic will scroll it into view as needed.
toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
--- a/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
+++ b/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
@@ -1263,23 +1263,56 @@ nsTypeAheadFind::IsRangeVisible(nsIPresS
   // 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
   nsRectVisibility rectVisibility = nsRectVisibility_kAboveViewport;
 
   if (!aGetTopVisibleLeaf && !frame->GetRect().IsEmpty()) {
     rectVisibility =
       aPresShell->GetRectVisibility(frame,
-                                    nsRect(nsPoint(0,0), frame->GetSize()),
+                                    frame->GetRectRelativeToSelf(),
                                     minDistance);
 
     if (rectVisibility == nsRectVisibility_kVisible) {
-      // This is an early exit case, where we return true if and only if
-      // the range is actually rendered.
-      return IsRangeRendered(aPresShell, aPresContext, aRange);
+      // 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) {
+          nsRectVisibility rangeRectVisibility =
+            aPresShell->GetRectVisibility(frame, r, minDistance);
+
+          if (rangeRectVisibility == nsRectVisibility_kVisible) {
+            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(aPresShell, aPresContext, 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".