Title: [166060] trunk
Revision
166060
Author
[email protected]
Date
2014-03-21 06:55:40 -0700 (Fri, 21 Mar 2014)

Log Message

Subpixel rendering: RenderBox is positioned off by one when non-compositing transform is present.
https://bugs.webkit.org/show_bug.cgi?id=130430

Reviewed by Simon Fraser.

div {
    position: absolute;
     top: 10.25px;
     left: 10.25px;
 }

 The <div> with (10.25px, 10.25px) is painted to (10.5px, 10.5px) after device pixel snapping on 2x display.
 Moving <div> to its own RenderLayer should not change the painting position.

 div {
     position: absolute;
     top: 10.25px;
     left: 10.25px;
     -webkit-transform: rotate(0deg);
 }

When we paint the RenderLayer's content, the graphics context is translated by the rounded value of
renderer's offset from parent.

    (10.25px,10.25px) -> rounded to (10.5px,10.5px).

When the translate moves the graphics context's origin over the renderer's top-left position,
the renderer's relative top-left coordinates end up being negative.

    Graphics context translated by (10.5px,10.5px) -> pushes renderer's relative top-left coords to (-0.25px,-0.25px)

When we round (pixel snap) these negative coordinates, half-way values get translated to the wrong direction.

(relative coords (-0.25px,-0.25px) -> pixel snapped to (-0.5px,-0.5px) -> final absolute(painting) coords (10px,10px))

This patch changes the rounding to flooring to ensure that the relative top-left position never gets negative as the result
of subpixel shifting.

Source/WebCore:

Tests: compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present.html
       fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present.html

* css/CSSComputedStyleDeclaration.cpp:
(WebCore::computedTransform):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::updateTransform):
(WebCore::RenderLayer::currentTransform):
(WebCore::RenderLayer::paintLayerByApplyingTransform):
* rendering/RenderLayerBacking.cpp:
(WebCore::RenderLayerBacking::updateTransform):
* rendering/style/RenderStyle.cpp:
* rendering/style/RenderStyle.h:
* svg/SVGTextElement.cpp:
(WebCore::SVGTextElement::animatedLocalTransform):

LayoutTests:

* TestExpectations:
* compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present-expected.html: Added.
* compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present.html: Added.
* fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present-expected.html: Added.
* fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (166059 => 166060)


--- trunk/LayoutTests/ChangeLog	2014-03-21 13:01:56 UTC (rev 166059)
+++ trunk/LayoutTests/ChangeLog	2014-03-21 13:55:40 UTC (rev 166060)
@@ -1,3 +1,49 @@
+2014-03-21  Zalan Bujtas  <[email protected]>
+
+        Subpixel rendering: RenderBox is positioned off by one when non-compositing transform is present.
+        https://bugs.webkit.org/show_bug.cgi?id=130430
+
+        Reviewed by Simon Fraser.
+
+        div {
+            position: absolute;
+             top: 10.25px;
+             left: 10.25px;
+         }
+
+         The <div> with (10.25px, 10.25px) is painted to (10.5px, 10.5px) after device pixel snapping on 2x display.
+         Moving <div> to its own RenderLayer should not change the painting position.
+
+         div {
+             position: absolute;
+             top: 10.25px;
+             left: 10.25px;
+             -webkit-transform: rotate(0deg);
+         }
+
+        When we paint the RenderLayer's content, the graphics context is translated by the rounded value of
+        renderer's offset from parent.
+
+            (10.25px,10.25px) -> rounded to (10.5px,10.5px).
+
+        When the translate moves the graphics context's origin over the renderer's top-left position,
+        the renderer's relative top-left coordinates end up being negative.
+
+            Graphics context translated by (10.5px,10.5px) -> pushes renderer's relative top-left coords to (-0.25px,-0.25px)
+
+        When we round (pixel snap) these negative coordinates, half-way values get translated to the wrong direction.
+
+        (relative coords (-0.25px,-0.25px) -> pixel snapped to (-0.5px,-0.5px) -> final absolute(painting) coords (10px,10px))
+
+        This patch changes the rounding to flooring to ensure that the relative top-left position never gets negative as the result
+        of subpixel shifting.
+
+        * TestExpectations:
+        * compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present-expected.html: Added.
+        * compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present.html: Added.
+        * fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present-expected.html: Added.
+        * fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present.html: Added.
+
 2014-03-19  Frédéric Wang  <[email protected]>
 
         Update some references for MathML pixels tests

Modified: trunk/LayoutTests/TestExpectations (166059 => 166060)


--- trunk/LayoutTests/TestExpectations	2014-03-21 13:01:56 UTC (rev 166059)
+++ trunk/LayoutTests/TestExpectations	2014-03-21 13:55:40 UTC (rev 166060)
@@ -88,6 +88,8 @@
 #subpixel failure on non-retina displays.
 webkit.org/b/129050 fast/sub-pixel/compositing-layers-on-subpixel-position.html [ ImageOnlyFailure ]
 webkit.org/b/129113 fast/multicol/newmulticol/clipping.html [ ImageOnlyFailure ]
+#subpixel rotation
+webkit.org/b/130556 compositing/hidpi-absolute-subpixel-positioned-transformed-elements.html [ ImageOnlyFailure ]
 
 webkit.org/b/129327 inspector-protocol/indexeddb/basics.html [ Pass Failure ]
 

Added: trunk/LayoutTests/compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present-expected.html (0 => 166060)


--- trunk/LayoutTests/compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present-expected.html	                        (rev 0)
+++ trunk/LayoutTests/compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present-expected.html	2014-03-21 13:55:40 UTC (rev 166060)
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>This tests if flooring the transformed boxes' coordinates produce the same result as if compositing transform is present.</title>
+<head>
+<style>
+  div {
+    background: green;
+    width: 5px;
+    height: 5px;
+    position: absolute;
+    -webkit-transform: rotate3D(0, 0, 0, 0deg);
+  }
+</style>
+</head>
+<body>
+<p id="container"></p>
+<script>
+  var container = document.getElementById("container");
+  for (i = 0; i < 40; ++i) {
+    adjustment = 0.25;
+    for (j = 0; j < 40; ++j) {
+      var e = document.createElement("div");
+      e.style.top = (i * 5 + adjustment) +  "px";
+      e.style.left = ( j * 5 + adjustment) + "px";
+      container.appendChild(e);
+      adjustment+=0.05;
+    }
+  }
+</script>
+</body>
+</html>

Added: trunk/LayoutTests/compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present.html (0 => 166060)


--- trunk/LayoutTests/compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present.html	                        (rev 0)
+++ trunk/LayoutTests/compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present.html	2014-03-21 13:55:40 UTC (rev 166060)
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>This tests if flooring the transformed boxes' coordinates produce the same result as if compositing transform is present.</title>
+<head>
+<style>
+  div {
+    background: green;
+    width: 5px;
+    height: 5px;
+    position: absolute;
+    -webkit-transform: rotate(0deg);
+  }
+</style>
+</head>
+<body>
+<p id="container"></p>
+<script>
+  var container = document.getElementById("container");
+  for (i = 0; i < 40; ++i) {
+    adjustment = 0.25;
+    for (j = 0; j < 40; ++j) {
+      var e = document.createElement("div");
+      e.style.top = (i * 5 + adjustment) +  "px";
+      e.style.left = ( j * 5 + adjustment) + "px";
+      container.appendChild(e);
+      adjustment+=0.05;
+    }
+  }
+</script>
+</body>
+</html>

Added: trunk/LayoutTests/fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present-expected.html (0 => 166060)


--- trunk/LayoutTests/fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present-expected.html	                        (rev 0)
+++ trunk/LayoutTests/fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present-expected.html	2014-03-21 13:55:40 UTC (rev 166060)
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>This tests if flooring the transformed boxes' coordinates produce the same result as if there was no transform present.</title>
+<head>
+<style>
+  div {
+    background: green;
+    width: 50px;
+    height: 200px;
+    position: absolute;
+  }
+</style>
+</head>
+<body>
+<div style="left: 0.5px; top: 0.5px;"></div><div style="left: 51px; top: 1px;"></div>
+<div style="left: 101.5px; top: 1.5px;"></div><div style="left: 152px; top: 2px;"></div>
+</script>
+</body>
+</html>

Added: trunk/LayoutTests/fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present.html (0 => 166060)


--- trunk/LayoutTests/fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present.html	                        (rev 0)
+++ trunk/LayoutTests/fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present.html	2014-03-21 13:55:40 UTC (rev 166060)
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>This tests if flooring the transformed boxes' coordinates produce the same result as if there was no transform present.</title>
+<head>
+<style>
+  div {
+    background: green;
+    width: 5px;
+    height: 5px;
+    position: absolute;
+    -webkit-transform: rotate(0deg);
+  }
+</style>
+</head>
+<body>
+<p id="container"></p>
+<script>
+  var container = document.getElementById("container");
+  for (i = 0; i < 40; ++i) {
+    adjustment = 0.25;
+    for (j = 0; j < 40; ++j) {
+      var e = document.createElement("div");
+      e.style.top = (i * 5 + adjustment) +  "px";
+      e.style.left = ( j * 5 + adjustment) + "px";
+      container.appendChild(e);
+      adjustment+=0.05;
+    }
+  }
+</script>
+</body>
+</html>

Modified: trunk/Source/WebCore/ChangeLog (166059 => 166060)


--- trunk/Source/WebCore/ChangeLog	2014-03-21 13:01:56 UTC (rev 166059)
+++ trunk/Source/WebCore/ChangeLog	2014-03-21 13:55:40 UTC (rev 166060)
@@ -1,3 +1,59 @@
+2014-03-21  Zalan Bujtas  <[email protected]>
+
+        Subpixel rendering: RenderBox is positioned off by one when non-compositing transform is present.
+        https://bugs.webkit.org/show_bug.cgi?id=130430
+
+        Reviewed by Simon Fraser.
+
+        div {
+            position: absolute;
+             top: 10.25px;
+             left: 10.25px;
+         }
+
+         The <div> with (10.25px, 10.25px) is painted to (10.5px, 10.5px) after device pixel snapping on 2x display.
+         Moving <div> to its own RenderLayer should not change the painting position.
+
+         div {
+             position: absolute;
+             top: 10.25px;
+             left: 10.25px;
+             -webkit-transform: rotate(0deg);
+         }
+
+        When we paint the RenderLayer's content, the graphics context is translated by the rounded value of
+        renderer's offset from parent.
+
+            (10.25px,10.25px) -> rounded to (10.5px,10.5px).
+
+        When the translate moves the graphics context's origin over the renderer's top-left position,
+        the renderer's relative top-left coordinates end up being negative.
+
+            Graphics context translated by (10.5px,10.5px) -> pushes renderer's relative top-left coords to (-0.25px,-0.25px)
+
+        When we round (pixel snap) these negative coordinates, half-way values get translated to the wrong direction.
+
+        (relative coords (-0.25px,-0.25px) -> pixel snapped to (-0.5px,-0.5px) -> final absolute(painting) coords (10px,10px))
+
+        This patch changes the rounding to flooring to ensure that the relative top-left position never gets negative as the result
+        of subpixel shifting.
+
+        Tests: compositing/hidpi-box-positioned-off-by-one-when-non-compositing-transform-is-present.html
+               fast/layers/hidpi-box-positioned-off-by-one-when-transform-is-present.html
+
+        * css/CSSComputedStyleDeclaration.cpp:
+        (WebCore::computedTransform):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::updateTransform):
+        (WebCore::RenderLayer::currentTransform):
+        (WebCore::RenderLayer::paintLayerByApplyingTransform):
+        * rendering/RenderLayerBacking.cpp:
+        (WebCore::RenderLayerBacking::updateTransform):
+        * rendering/style/RenderStyle.cpp:
+        * rendering/style/RenderStyle.h:
+        * svg/SVGTextElement.cpp:
+        (WebCore::SVGTextElement::animatedLocalTransform):
+
 2014-03-21  Pratik Solanki  <[email protected]>
 
         Unreviewed. iOS build fix after r166046.

Modified: trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp (166059 => 166060)


--- trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp	2014-03-21 13:01:56 UTC (rev 166059)
+++ trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp	2014-03-21 13:55:40 UTC (rev 166060)
@@ -826,12 +826,12 @@
     if (!renderer || !renderer->hasTransform() || !style->hasTransform())
         return cssValuePool().createIdentifierValue(CSSValueNone);
 
-    IntRect box;
+    FloatRect pixelSnappedRect;
     if (renderer->isBox())
-        box = pixelSnappedIntRect(toRenderBox(renderer)->borderBoxRect());
+        pixelSnappedRect = pixelSnappedForPainting(toRenderBox(renderer)->borderBoxRect(), renderer->document().deviceScaleFactor());
 
     TransformationMatrix transform;
-    style->applyTransform(transform, box.size(), RenderStyle::ExcludeTransformOrigin);
+    style->applyTransform(transform, pixelSnappedRect, RenderStyle::ExcludeTransformOrigin);
     // Note that this does not flatten to an affine transform if ENABLE(3D_RENDERING) is off, by design.
 
     // FIXME: Need to print out individual functions (https://bugs.webkit.org/show_bug.cgi?id=23924)

Modified: trunk/Source/WebCore/rendering/RenderLayer.cpp (166059 => 166060)


--- trunk/Source/WebCore/rendering/RenderLayer.cpp	2014-03-21 13:01:56 UTC (rev 166059)
+++ trunk/Source/WebCore/rendering/RenderLayer.cpp	2014-03-21 13:55:40 UTC (rev 166060)
@@ -878,7 +878,7 @@
         RenderBox* box = renderBox();
         ASSERT(box);
         m_transform->makeIdentity();
-        box->style().applyTransform(*m_transform, box->pixelSnappedBorderBoxRect().size(), RenderStyle::IncludeTransformOrigin);
+        box->style().applyTransform(*m_transform, pixelSnappedForPainting(box->borderBoxRect(), box->document().deviceScaleFactor()), RenderStyle::IncludeTransformOrigin);
         makeMatrixRenderable(*m_transform, canRender3DTransforms());
     }
 
@@ -891,19 +891,21 @@
     if (!m_transform)
         return TransformationMatrix();
 
+    RenderBox* box = renderBox();
+    ASSERT(box);
+    FloatRect pixelSnappedBorderRect = pixelSnappedForPainting(box->borderBoxRect(), box->document().deviceScaleFactor());
     if (renderer().style().isRunningAcceleratedAnimation()) {
         TransformationMatrix currTransform;
         RefPtr<RenderStyle> style = renderer().animation().getAnimatedStyleForRenderer(&renderer());
-        style->applyTransform(currTransform, renderBox()->pixelSnappedBorderBoxRect().size(), applyOrigin);
+        style->applyTransform(currTransform, pixelSnappedBorderRect, applyOrigin);
         makeMatrixRenderable(currTransform, canRender3DTransforms());
         return currTransform;
     }
 
     // m_transform includes transform-origin, so we need to recompute the transform here.
     if (applyOrigin == RenderStyle::ExcludeTransformOrigin) {
-        RenderBox* box = renderBox();
         TransformationMatrix currTransform;
-        box->style().applyTransform(currTransform, box->pixelSnappedBorderBoxRect().size(), RenderStyle::ExcludeTransformOrigin);
+        box->style().applyTransform(currTransform, pixelSnappedBorderRect, RenderStyle::ExcludeTransformOrigin);
         makeMatrixRenderable(currTransform, canRender3DTransforms());
         return currTransform;
     }
@@ -4202,13 +4204,13 @@
     convertToLayerCoords(paintingInfo.rootLayer, offsetFromParent);
     offsetFromParent.moveBy(translationOffset);
     TransformationMatrix transform(renderableTransform(paintingInfo.paintBehavior));
-    FloatPoint devicePixelSnappeddOffsetFromParent = roundedForPainting(offsetFromParent, deviceScaleFactor);
+    FloatPoint devicePixelFlooredOffsetFromParent = flooredForPainting(offsetFromParent, deviceScaleFactor);
     // Translate the graphics context to the snapping position to avoid off-device-pixel positing.
-    transform.translateRight(devicePixelSnappeddOffsetFromParent.x(), devicePixelSnappeddOffsetFromParent.y());
+    transform.translateRight(devicePixelFlooredOffsetFromParent.x(), devicePixelFlooredOffsetFromParent.y());
     // We handle accumulated subpixels through nested layers here. Since the context gets translated to device pixels,
     // all we need to do is add the delta to the accumulated pixels coming from ancestor layers. With deep nesting of subpixel positioned
     // boxes, this could grow to a relatively large number, but the translateRight() balances it.
-    FloatSize delta = offsetFromParent - devicePixelSnappeddOffsetFromParent;
+    FloatSize delta = offsetFromParent - devicePixelFlooredOffsetFromParent;
     LayoutSize adjustedSubPixelAccumulation = paintingInfo.subPixelAccumulation + LayoutSize(delta);
     // Apply the transform.
     GraphicsContextStateSaver stateSaver(*context);

Modified: trunk/Source/WebCore/rendering/RenderLayerBacking.cpp (166059 => 166060)


--- trunk/Source/WebCore/rendering/RenderLayerBacking.cpp	2014-03-21 13:01:56 UTC (rev 166059)
+++ trunk/Source/WebCore/rendering/RenderLayerBacking.cpp	2014-03-21 13:55:40 UTC (rev 166060)
@@ -370,7 +370,8 @@
     // baked into it, and we don't want that.
     TransformationMatrix t;
     if (m_owningLayer.hasTransform()) {
-        style->applyTransform(t, toRenderBox(renderer()).pixelSnappedBorderBoxRect().size(), RenderStyle::ExcludeTransformOrigin);
+        RenderBox& renderBox = toRenderBox(renderer());
+        style->applyTransform(t, pixelSnappedForPainting(renderBox.borderBoxRect(), renderBox.document().deviceScaleFactor()), RenderStyle::ExcludeTransformOrigin);
         makeMatrixRenderable(t, compositor().canRender3DTransforms());
     }
     

Modified: trunk/Source/WebCore/rendering/style/RenderStyle.cpp (166059 => 166060)


--- trunk/Source/WebCore/rendering/style/RenderStyle.cpp	2014-03-21 13:01:56 UTC (rev 166059)
+++ trunk/Source/WebCore/rendering/style/RenderStyle.cpp	2014-03-21 13:55:40 UTC (rev 166060)
@@ -958,25 +958,6 @@
     return false;
 }
 
-void RenderStyle::applyTransform(TransformationMatrix& transform, const LayoutSize& borderBoxSize, ApplyTransformOrigin applyOrigin) const
-{
-    // FIXME: when subpixel layout is supported (bug 71143) the body of this function could be replaced by
-    // applyTransform(transform, FloatRect(FloatPoint(), borderBoxSize), applyOrigin);
-    
-    const Vector<RefPtr<TransformOperation>>& transformOperations = rareNonInheritedData->m_transform->m_operations.operations();
-    bool applyTransformOrigin = requireTransformOrigin(transformOperations, applyOrigin);
-
-    if (applyTransformOrigin)
-        transform.translate3d(floatValueForLength(transformOriginX(), borderBoxSize.width()), floatValueForLength(transformOriginY(), borderBoxSize.height()), transformOriginZ());
-
-    unsigned size = transformOperations.size();
-    for (unsigned i = 0; i < size; ++i)
-        transformOperations[i]->apply(transform, borderBoxSize);
-
-    if (applyTransformOrigin)
-        transform.translate3d(-floatValueForLength(transformOriginX(), borderBoxSize.width()), -floatValueForLength(transformOriginY(), borderBoxSize.height()), -transformOriginZ()); 
-}
-
 void RenderStyle::applyTransform(TransformationMatrix& transform, const FloatRect& boundingBox, ApplyTransformOrigin applyOrigin) const
 {
     const Vector<RefPtr<TransformOperation>>& transformOperations = rareNonInheritedData->m_transform->m_operations.operations();

Modified: trunk/Source/WebCore/rendering/style/RenderStyle.h (166059 => 166060)


--- trunk/Source/WebCore/rendering/style/RenderStyle.h	2014-03-21 13:01:56 UTC (rev 166059)
+++ trunk/Source/WebCore/rendering/style/RenderStyle.h	2014-03-21 13:55:40 UTC (rev 166060)
@@ -874,7 +874,6 @@
     bool hasTransformRelatedProperty() const { return hasTransform() || preserves3D() || hasPerspective(); }
 
     enum ApplyTransformOrigin { IncludeTransformOrigin, ExcludeTransformOrigin };
-    void applyTransform(TransformationMatrix&, const LayoutSize& borderBoxSize, ApplyTransformOrigin = IncludeTransformOrigin) const;
     void applyTransform(TransformationMatrix&, const FloatRect& boundingBox, ApplyTransformOrigin = IncludeTransformOrigin) const;
     void setPageScaleTransform(float);
 

Modified: trunk/Source/WebCore/svg/SVGTextElement.cpp (166059 => 166060)


--- trunk/Source/WebCore/svg/SVGTextElement.cpp	2014-03-21 13:01:56 UTC (rev 166059)
+++ trunk/Source/WebCore/svg/SVGTextElement.cpp	2014-03-21 13:55:40 UTC (rev 166060)
@@ -54,7 +54,7 @@
         TransformationMatrix t;
         // For now, the transform-origin is not taken into account
         // Also, any percentage values will not be taken into account
-        style->applyTransform(t, IntSize(0, 0), RenderStyle::ExcludeTransformOrigin);
+        style->applyTransform(t, FloatRect(0, 0, 0, 0), RenderStyle::ExcludeTransformOrigin);
         // Flatten any 3D transform
         matrix = t.toAffineTransform();
     } else
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to