Bug 1831148: Increase clamping range for TransformGfxRectToAncestor…

…. r=emilio The clamping is overaly pessimistic because the returned `Rect` is still float-based, and the call site does its own clamping to `nscoord`. Clamping to float's numeric limit causes significant floating point errors when clipping the transformed rect, resulting in incorrect transforms. Differential Revision: https://phabricator.services.mozilla.com/D178081
iaxat · May 16, 2023 · b73c5cc · b73c5cc
1 parent 321414f
commit b73c5cc
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 2 deletions.
diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
@@ -2359,9 +2359,13 @@ static Rect TransformGfxRectToAncestor(
   }
   const nsIFrame* ancestor = aOutAncestor ? *aOutAncestor : aAncestor.mFrame;
   float factor = ancestor->PresContext()->AppUnitsPerDevPixel();
+  // Caller is expected to properly clamp if the result is to be converted to
+  // app units. Technically, the clipping bound here is infinite, but that
+  // causes clipping to become unpredictable due to floating point errors.
+  const auto boundsAppUnits = Rect::MaxIntRect();
   Rect maxBounds =
-      Rect(float(nscoord_MIN) / factor * 0.5, float(nscoord_MIN) / factor * 0.5,
-           float(nscoord_MAX) / factor, float(nscoord_MAX) / factor);
+      Rect(boundsAppUnits.x / factor, boundsAppUnits.y / factor,
+           boundsAppUnits.width / factor, boundsAppUnits.height / factor);
   return ctm.TransformAndClipBounds(aRect, maxBounds);
 }
 

diff --git a/layout/generic/test/mochitest.ini b/layout/generic/test/mochitest.ini
@@ -164,3 +164,4 @@ support-files =
 [test_scroll_on_display_contents.html]
 support-files = !/gfx/layers/apz/test/mochitest/apz_test_native_event_utils.js
 [test_bug1803209.html]
+[test_bug1831148.html]
diff --git a/layout/generic/test/test_bug1831148.html b/layout/generic/test/test_bug1831148.html
@@ -0,0 +1,46 @@
+<!DOCTYPE html>
+<html>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+<style>
+#marker {
+  width: 1px;
+  height: 1px;
+}
+#outer {
+  height: 1000px;
+  background: #a0ffff;
+  overflow: scroll;
+}
+
+#inner {
+  height: 16593200px;
+  background: #ffa0a0;
+}
+
+.log {
+  white-space: pre;
+}
+</style>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1831148">Mozilla Bug 1831148</a>
+<div id="marker"></div>
+<div id="outer">
+  <div id="inner"></div>
+</div>
+<script>
+SimpleTest.waitForExplicitFinish();
+addLoadEvent(() => {
+  const interval = 20;
+  const increment = outer.scrollHeight / interval;
+  const offset = marker.getBoundingClientRect().bottom;
+  for (let i = 0; i < interval; i++) {
+    outer.scrollTop = i * increment;
+    console.log(outer.scrollTop);
+    // Shift to account for viewport coordinate shift.
+    const bcrTop = -inner.getBoundingClientRect().top + offset;
+    // Floating point value diverges from scrollTop.
+    isfuzzy(bcrTop, outer.scrollTop, 1, "scrollTop and BCR top match");
+  }
+  SimpleTest.finish();
+});
+</script>