TransformState::move should not round offset to int
authoreae@chromium.org <eae@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Feb 2013 17:40:28 +0000 (17:40 +0000)
committereae@chromium.org <eae@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Feb 2013 17:40:28 +0000 (17:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=108266

Source/WebCore:

Reviewed by Simon Fraser.

Currently TransformState::move rounds the offset to the nearest
integer values, this results in operations using TransformState
to compute a position to misreport the location, specifically
Element:getBoundingClientRect and repaint rects. Sizes are
handled correctly and do not have the same problem.

Tests: fast/sub-pixel/boundingclientrect-subpixel-margin.html
       fast/sub-pixel/clip-rect-box-consistent-rounding.html

* page/FrameView.cpp:
(WebCore::FrameView::convertFromRenderer):
Change to use pixel snapping instead of enclosing box. All other
code paths use pixelSnappedIntRect to align the rects to device
pixels however this used enclosingIntRect (indirectly through
the FloatQuad::enclosingBoundingBox call).
Without the rounding in TransformState this causes repaint rects
for elements on subpixel bounds to be too large by up to one
pixel on each axis. For normal repaints this isn't really a
problem but in scrollContentsSlowPath it can result in moving
too large a rect.

* platform/graphics/transforms/TransformState.cpp:
(WebCore::TransformState::translateTransform):
(WebCore::TransformState::translateMappedCoordinates):
Change to take a LayoutSize instead of an IntSize.

(WebCore::TransformState::move):
(WebCore::TransformState::applyAccumulatedOffset):
* platform/graphics/transforms/TransformState.h:
Remove rounding logic and use original, more precise, value.

* rendering/RenderGeometryMap.cpp:
(WebCore::RenderGeometryMap::mapToContainer):
Remove rounding logic and use original, more precise, value.

LayoutTests:

Reviewed by Simon Fraser.

Add new tests for Element::boundingClientRect and clip rects for
elements on subpixel boundaries.

* fast/dom/Window/webkitConvertPoint.html:
* platform/chromium-linux/fast/dom/Window/webkitConvertPoint-expected.txt:
Update test and expectations to take new rounding into account.

* fast/sub-pixel/boundingclientrect-subpixel-margin-expected.txt: Added.
* fast/sub-pixel/boundingclientrect-subpixel-margin.html: Added.
Add test ensuring that boundingClientRect returns accurate and
precise (as opposed to rounded) metrics.

* fast/sub-pixel/clip-rect-box-consistent-rounding-expected.html: Added.
* fast/sub-pixel/clip-rect-box-consistent-rounding.html: Added.
Add test ensuring that clip rects and elements use consistent rounding.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@142638 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/dom/Window/webkitConvertPoint.html
LayoutTests/fast/sub-pixel/boundingclientrect-subpixel-margin-expected.txt [new file with mode: 0644]
LayoutTests/fast/sub-pixel/boundingclientrect-subpixel-margin.html [new file with mode: 0644]
LayoutTests/fast/sub-pixel/clip-rect-box-consistent-rounding-expected.html [new file with mode: 0644]
LayoutTests/fast/sub-pixel/clip-rect-box-consistent-rounding.html [new file with mode: 0644]
LayoutTests/platform/chromium-linux/fast/dom/Window/webkitConvertPoint-expected.txt
LayoutTests/platform/chromium/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/platform/graphics/transforms/TransformState.cpp
Source/WebCore/platform/graphics/transforms/TransformState.h
Source/WebCore/rendering/RenderGeometryMap.cpp

index 124da7d..497b4b2 100644 (file)
@@ -1,3 +1,27 @@
+2013-02-12  Emil A Eklund  <eae@chromium.org>
+
+        TransformState::move should not round offset to int
+        https://bugs.webkit.org/show_bug.cgi?id=108266
+
+        Reviewed by Simon Fraser.
+        
+        Add new tests for Element::boundingClientRect and clip rects for
+        elements on subpixel boundaries.
+
+        * fast/dom/Window/webkitConvertPoint.html:
+        * platform/chromium-linux/fast/dom/Window/webkitConvertPoint-expected.txt:
+        Update test and expectations to take new rounding into account.
+        
+        * fast/sub-pixel/boundingclientrect-subpixel-margin-expected.txt: Added.
+        * fast/sub-pixel/boundingclientrect-subpixel-margin.html: Added.
+        Add test ensuring that boundingClientRect returns accurate and
+        precise (as opposed to rounded) metrics.
+        
+        * fast/sub-pixel/clip-rect-box-consistent-rounding-expected.html: Added.
+        * fast/sub-pixel/clip-rect-box-consistent-rounding.html: Added.
+        Add test ensuring that clip rects and elements use consistent rounding.
+
+
 2013-02-12  Rafael Weinstein  <rafaelw@chromium.org>
 
         [HTMLTemplateElement] <template> inside of <head> may not create <body> if EOF is hit
index b5b82a5..78fdd46 100644 (file)
                 runTest("Test 8",  "test8",  8, 273, 13, 313);
                 runTest("Test 9",  "test9",  28, 291, 33, 331);
                 runTest("Test 10", "test10", 28, 309, 33, 349);
-                runTest("Test 11", "test11", 158, 356, 174, 374);
-                runTest("Test 12", "test12", 168, 429, 184, 447);
+                runTest("Test 11", "test11", 158, 376, 174, 394);
+                runTest("Test 12", "test12", 168, 451, 184, 469);
                 runTest("Test 13", "test13", 28, 487, 33, 527);
             } else {
                 runTest("Test 1",  "test1",  8, 12, 13, 52);
diff --git a/LayoutTests/fast/sub-pixel/boundingclientrect-subpixel-margin-expected.txt b/LayoutTests/fast/sub-pixel/boundingclientrect-subpixel-margin-expected.txt
new file mode 100644 (file)
index 0000000..070a4ac
--- /dev/null
@@ -0,0 +1,3 @@
+PASS testRect.left - parentRect.left is 1.5
+PASS parentRect.right - testRect.right is 1.5
+
diff --git a/LayoutTests/fast/sub-pixel/boundingclientrect-subpixel-margin.html b/LayoutTests/fast/sub-pixel/boundingclientrect-subpixel-margin.html
new file mode 100644 (file)
index 0000000..eaed67a
--- /dev/null
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <script src="../js/resources/js-test-pre.js"></script>
+    </head>
+    <body style="margin: 0; padding: 0;">
+        <div style="margin: 0; width: 700px; margin: 10px; background: black; overflow: hidden;">
+            <div id="test" style="width: 697px; height: 200px; background-color: red; margin: 0 auto"></div>
+        </div>
+        <script>
+            var testEl = document.getElementById('test');
+            var testRect = testEl.getBoundingClientRect();
+            var parentRect = testEl.parentElement.getBoundingClientRect();
+
+            var expectedMarginWidth = String((parentRect.width - testRect.width) / 2);
+            shouldBe('testRect.left - parentRect.left', expectedMarginWidth);
+            shouldBe('parentRect.right - testRect.right', expectedMarginWidth);
+        </script>
+    </body>
+</html>
diff --git a/LayoutTests/fast/sub-pixel/clip-rect-box-consistent-rounding-expected.html b/LayoutTests/fast/sub-pixel/clip-rect-box-consistent-rounding-expected.html
new file mode 100644 (file)
index 0000000..5a3c3c5
--- /dev/null
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html>
+       <head>
+               <style type="text/css">
+                       body {
+                               margin: 10px;
+                       }
+                       .main {
+                               margin: 0 auto;
+                               width: 625px;
+                               height: 125px;
+                               background: black;
+                               position: relative;
+                       }
+                       .site {
+                               top: 5px;
+                               left: 5px;
+                               width: 232px;
+                               height: 115px;
+                               position: absolute;
+                               overflow: hidden;
+                       }
+                       .desaturated {
+                               position: absolute;
+                               width: 250px;
+                               height: 125px;
+                               background: white;
+                       }
+                       .infobox {
+                               position: absolute;
+                               top: 0;
+                               left: 0;
+                               width: 232px;
+                               height: 63px;
+                               background: red;
+                       }
+               </style>
+       </head>
+
+       <body>
+               <div class="main">
+                       <div class="site">
+                               <div class="desaturated"></div>
+                               <div class="infobox"></div>
+                       </div>
+               </div>
+       </body>
+</html>
diff --git a/LayoutTests/fast/sub-pixel/clip-rect-box-consistent-rounding.html b/LayoutTests/fast/sub-pixel/clip-rect-box-consistent-rounding.html
new file mode 100644 (file)
index 0000000..c2971f1
--- /dev/null
@@ -0,0 +1,49 @@
+<!DOCTYPE html>
+<html>
+       <head>
+               <style type="text/css">
+                       body {
+                               margin: 8px;
+                               zoom: 1.25;
+                       }
+                       .main {
+                               margin: 0 auto;
+                               width: 500px;
+                               height: 100px;
+                               background: black;
+                               position: relative;
+                       }
+                       .site {
+                               top: 4px;
+                               left: 4px;
+                               width: 186px;
+                               height: 92px;
+                               position: absolute;
+                               overflow: hidden;
+                       }
+                       .desaturated {
+                               position: absolute;
+                               width: 200px;
+                               height: 100px;
+                               background: white;
+                       }
+                       .infobox {
+                               position: absolute;
+                               top: 0;
+                               left: 0;
+                               width: 186px;
+                               height: 50px;
+                               background: red;
+                       }
+               </style>
+       </head>
+
+       <body>
+               <div class="main">
+                       <div class="site">
+                               <div class="desaturated"></div>
+                               <div class="infobox"></div>
+                       </div>
+               </div>
+       </body>
+</html>
index d98e875..864b611 100644 (file)
@@ -158,24 +158,24 @@ PASS y is 40
 
 Test 11
 PASS x is 158
-FAIL y should be 356. Was 377.
+PASS y is 376
 Round Trip of (0,0)
 PASS x is 0
 PASS y is 0
 PASS x is 174
-FAIL y should be 374. Was 395.
+PASS y is 394
 Round Trip of (5,40)
 PASS x is 5
 PASS y is 40
 
 Test 12
 PASS x is 168
-FAIL y should be 429. Was 452.
+PASS y is 451
 Round Trip of (0,0)
 PASS x is 0
 PASS y is 0
 PASS x is 184
-FAIL y should be 447. Was 470.
+PASS y is 469
 Round Trip of (5,40)
 PASS x is 5
 PASS y is 40
index 20890ed..f3d8acc 100644 (file)
@@ -954,6 +954,11 @@ webkit.org/b/109272 platform/chromium/fast/forms/calendar-picker/calendar-picker
 webkit.org/b/109272 platform/chromium/fast/forms/suggestion-picker/datetime-suggestion-picker-appearance.html [ Skip ]
 webkit.org/b/109272 platform/chromium/fast/forms/suggestion-picker/datetime-suggestion-picker-mouse-operations.html [ Skip ]
 
+# Skip until 109528 is fixed.
+webkit.org/b/109528 platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar.html [ Skip ]
+webkit.org/b/109528 platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-with-scroll-bar.html [ Skip ]
+webkit.org/b/109528 platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-with-scroll-bar.html [ Skip ]
+
 # Skipping rules for ANDROID are in platform/chromium-android/TestExpectations.
 
 # -----------------------------------------------------------------
index f437580..c391911 100644 (file)
@@ -1,3 +1,45 @@
+2013-02-12  Emil A Eklund  <eae@chromium.org>
+
+        TransformState::move should not round offset to int
+        https://bugs.webkit.org/show_bug.cgi?id=108266
+
+        Reviewed by Simon Fraser.
+        
+        Currently TransformState::move rounds the offset to the nearest
+        integer values, this results in operations using TransformState
+        to compute a position to misreport the location, specifically
+        Element:getBoundingClientRect and repaint rects. Sizes are
+        handled correctly and do not have the same problem.
+
+        Tests: fast/sub-pixel/boundingclientrect-subpixel-margin.html
+               fast/sub-pixel/clip-rect-box-consistent-rounding.html
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::convertFromRenderer):
+        Change to use pixel snapping instead of enclosing box. All other
+        code paths use pixelSnappedIntRect to align the rects to device
+        pixels however this used enclosingIntRect (indirectly through
+        the FloatQuad::enclosingBoundingBox call).
+        Without the rounding in TransformState this causes repaint rects
+        for elements on subpixel bounds to be too large by up to one
+        pixel on each axis. For normal repaints this isn't really a
+        problem but in scrollContentsSlowPath it can result in moving
+        too large a rect.
+
+        * platform/graphics/transforms/TransformState.cpp:
+        (WebCore::TransformState::translateTransform):
+        (WebCore::TransformState::translateMappedCoordinates):
+        Change to take a LayoutSize instead of an IntSize.
+
+        (WebCore::TransformState::move):
+        (WebCore::TransformState::applyAccumulatedOffset):
+        * platform/graphics/transforms/TransformState.h:
+        Remove rounding logic and use original, more precise, value.
+
+        * rendering/RenderGeometryMap.cpp:
+        (WebCore::RenderGeometryMap::mapToContainer):
+        Remove rounding logic and use original, more precise, value.
+
 2013-02-12  Jessie Berlin  <jberlin@apple.com>
 
         Rollout r142618, it broke all the Mac builds.
index 36a20c2..586c054 100644 (file)
@@ -3587,7 +3587,7 @@ void FrameView::adjustPageHeightDeprecated(float *newBottom, float oldTop, float
 
 IntRect FrameView::convertFromRenderer(const RenderObject* renderer, const IntRect& rendererRect) const
 {
-    IntRect rect = renderer->localToAbsoluteQuad(FloatRect(rendererRect)).enclosingBoundingBox();
+    IntRect rect = pixelSnappedIntRect(enclosingLayoutRect(renderer->localToAbsoluteQuad(FloatRect(rendererRect)).boundingBox()));
 
     // Convert from page ("absolute") to FrameView coordinates.
     if (!delegatesScrolling())
index 369d4b6..364a9f1 100644 (file)
@@ -50,7 +50,7 @@ TransformState& TransformState::operator=(const TransformState& other)
     return *this;
 }
 
-void TransformState::translateTransform(const IntSize& offset)
+void TransformState::translateTransform(const LayoutSize& offset)
 {
     if (m_direction == ApplyTransformDirection)
         m_accumulatedTransform->translateRight(offset.width(), offset.height());
@@ -58,9 +58,9 @@ void TransformState::translateTransform(const IntSize& offset)
         m_accumulatedTransform->translate(offset.width(), offset.height());
 }
 
-void TransformState::translateMappedCoordinates(const IntSize& offset)
+void TransformState::translateMappedCoordinates(const LayoutSize& offset)
 {
-    IntSize adjustedOffset = (m_direction == ApplyTransformDirection) ? offset : -offset;
+    LayoutSize adjustedOffset = (m_direction == ApplyTransformDirection) ? offset : -offset;
     if (m_mapPoint)
         m_lastPlanarPoint.move(adjustedOffset);
     if (m_mapQuad)
@@ -75,21 +75,21 @@ void TransformState::move(const LayoutSize& offset, TransformAccumulation accumu
         applyAccumulatedOffset();
         if (m_accumulatingTransform && m_accumulatedTransform) {
             // If we're accumulating into an existing transform, apply the translation.
-            translateTransform(roundedIntSize(offset));
+            translateTransform(offset);
 
             // Then flatten if necessary.
             if (accumulate == FlattenTransform)
                 flatten();
         } else
             // Just move the point and/or quad.
-            translateMappedCoordinates(roundedIntSize(offset));
+            translateMappedCoordinates(offset);
     }
     m_accumulatingTransform = accumulate == AccumulateTransform;
 }
 
 void TransformState::applyAccumulatedOffset()
 {
-    IntSize offset = roundedIntSize(m_accumulatedOffset);
+    LayoutSize offset = m_accumulatedOffset;
     m_accumulatedOffset = LayoutSize();
     if (!offset.isZero()) {
         if (m_accumulatedTransform) {
index 68d59ed..874177b 100644 (file)
@@ -101,8 +101,8 @@ public:
     FloatQuad mappedQuad(bool* wasClamped = 0) const;
 
 private:
-    void translateTransform(const IntSize&);
-    void translateMappedCoordinates(const IntSize&);
+    void translateTransform(const LayoutSize&);
+    void translateMappedCoordinates(const LayoutSize&);
     void flattenWithTransform(const TransformationMatrix&, bool* wasClamped);
     void applyAccumulatedOffset();
     
index 4f2793f..4513ef6 100644 (file)
@@ -128,7 +128,7 @@ FloatQuad RenderGeometryMap::mapToContainer(const FloatRect& rect, const RenderL
     
     if (!hasFixedPositionStep() && !hasTransformStep() && !hasNonUniformStep() && (!container || (m_mapping.size() && container == m_mapping[0].m_renderer))) {
         result = rect;
-        result.move(roundedIntSize(m_accumulatedOffset));
+        result.move(m_accumulatedOffset);
     } else {
         TransformState transformState(TransformState::ApplyTransformDirection, rect.center(), rect);
         mapToContainer(transformState, container);