Bug 89238

Created attachment 147889 [details] Patch

Created attachment 147890 [details] Patch

Comment on attachment 147890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147890&action=review r- for boolean params. When do you NOT want to pixel snap when mapping coords? > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:699 > + IntRect box = IntRect(sizingBox(renderer, true)); Gah, ugly boolean params!

(In reply to comment #3) > (From update of attachment 147890 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147890&action=review > > r- for boolean params. When do you NOT want to pixel snap when mapping coords? er, when do you WANT to pixel snap?

(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 147890 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=147890&action=review > > > > r- for boolean params. When do you NOT want to pixel snap when mapping coords? > > er, when do you WANT to pixel snap? We want to avoid pixel snapping for localToAbsolute to ensure that webkitConvertPoint works properly, but we want to pixel snap when translating a transform by a renderer's sub-pixel position. Otherwise, doing something as seemingly-innocuous as a rotateZ(0) can cause us to generate sub-pixel translations that'll misalign our painting. I'll get rid of the boolean parameters. Thanks for the review!

Created attachment 147917 [details] Patch

Comment on attachment 147917 [details] Patch Attachment 147917 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12955913

Comment on attachment 147917 [details] Patch Attachment 147917 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12960743 New failing tests: fast/transforms/rotated-transform-affects-scrolling-2.html fast/repaint/transform-translate.html transforms/svg-vs-css.xhtml fast/repaint/transform-absolute-child.html fast/transforms/transformed-document-element.html fast/repaint/transform-repaint-descendants.html fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html svg/transforms/svg-css-transforms.xhtml compositing/shadows/shadow-drawing.html fast/forms/validation-message-appearance.html

Created attachment 147924 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 147917 [details] Patch Attachment 147917 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12968215 New failing tests: fast/transforms/rotated-transform-affects-scrolling-2.html fast/repaint/transform-translate.html transforms/svg-vs-css.xhtml fast/repaint/transform-absolute-child.html fast/transforms/transformed-document-element.html fast/repaint/transform-repaint-descendants.html fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html svg/transforms/svg-css-transforms.xhtml compositing/shadows/shadow-drawing.html fast/forms/validation-message-appearance.html

Created attachment 147932 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #10) > (From update of attachment 147917 [details]) > Attachment 147917 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12968215 > > New failing tests: > fast/transforms/rotated-transform-affects-scrolling-2.html > fast/repaint/transform-translate.html > transforms/svg-vs-css.xhtml > fast/repaint/transform-absolute-child.html > fast/transforms/transformed-document-element.html > fast/repaint/transform-repaint-descendants.html > fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html > svg/transforms/svg-css-transforms.xhtml > compositing/shadows/shadow-drawing.html > fast/forms/validation-message-appearance.html FYI: All these test results are expected, and the repaint tests were actually off by a pixel previously, and are now correct.

Created attachment 148136 [details] Patch

(In reply to comment #13) > Created an attachment (id=148136) [details] > Patch Fixing the Mac build error.

Comment on attachment 148136 [details] Patch Attachment 148136 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12970794 New failing tests: fast/transforms/rotated-transform-affects-scrolling-2.html fast/repaint/transform-translate.html transforms/svg-vs-css.xhtml fast/repaint/transform-absolute-child.html fast/transforms/transformed-document-element.html fast/repaint/transform-repaint-descendants.html fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html svg/transforms/svg-css-transforms.xhtml compositing/shadows/shadow-drawing.html fast/forms/validation-message-appearance.html

Created attachment 148182 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 148136 [details] Patch Attachment 148136 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12979296 New failing tests: fast/transforms/rotated-transform-affects-scrolling-2.html fast/repaint/transform-translate.html transforms/svg-vs-css.xhtml fast/repaint/transform-absolute-child.html svg/css/getComputedStyle-basic.xhtml fast/transforms/transformed-document-element.html fast/repaint/transform-repaint-descendants.html fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html svg/transforms/svg-css-transforms.xhtml compositing/shadows/shadow-drawing.html fast/forms/validation-message-appearance.html

Created attachment 148199 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Pinging reviewers.

(In reply to comment #19) > Pinging reviewers. Been awhile, trying again.

Comment on attachment 148136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148136&action=review A first round of review. Obviously Simon should have a look at this as well! > Source/WebCore/ChangeLog:20 > + (WebCore::sizingBox): Adding a bool for pixel snapping. Aha, your code doesn't do that, it adds a new function duplicating sizingBox() functionality, but pixel-snaps. See below for a possible suggestion to avoid that. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:707 > - LayoutRect box = sizingBox(renderer); > + IntRect box = pixelSnappedSizingBox(renderer); Hm, can't you just use pixelSnappedIntRect(sizingBox(renderer)) here? From my understanding this should be equivalent, but avoids duplicating the logic of sizingBox() just to pixel-snap the values. > Source/WebCore/rendering/RenderInline.h:138 > - virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, bool fixed, bool useTransforms, TransformState&, ApplyContainerFlipOrNot = ApplyContainerFlip, bool* wasFixed = 0) const; > + virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, bool fixed, bool useTransforms, TransformState&, TransformingSubPixelOffsetBehavior adjustSubPixelOffsets = SnapOffsetsForTransforms, ApplyContainerFlipOrNot = ApplyContainerFlip, bool* wasFixed = 0) const OVERRIDE; All these bools should go away IMHO. And I'd rather do this now before bloating mapLocalToContainer any further with new extra arguments. (Though not your fault, but we should clean this up rather today than tomorrow :-) enum MapLocalToContainerMode { IsFixed = 1 << 0, UseTransforms = 1 << 1, ApplyContainerFlip = 1 << 2, SnapOffsetForTransforms 1 << 3; } typedef unsigned MapLocalToContainerFlags; virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, TransformState&, MapLocalToContainerFlags = SnapOffsetForTransforms | ApplyContainerFlip, bool* wasFixed = 0) const OVERRIDE; I didn't investigate in-detail if that would make some call-sites worse to read, but it should improve readability in general. > Source/WebCore/rendering/RenderLayer.cpp:549 > - box->style()->applyTransform(*m_transform, box->borderBoxRect().size(), RenderStyle::IncludeTransformOrigin); > + box->style()->applyTransform(*m_transform, box->pixelSnappedBorderBoxRect().size(), RenderStyle::IncludeTransformOrigin); I'd like to fully understand the effects of this change, please correct me if I got anything wrong: This change is relevant for computing the transform-origin induced translation (+ the argument is also passed to the TransformOperations::apply function). You're now using pixel-snapped values here. This is correct as you're aligning the layer at pixel boundaries, but respect any fractional translation while computing the layer transform (in RenderLayer::paint). So for borderBoxRect.size = 100.5 x 100.5, and a transform-origin of 50% 50%, this now results in a m_transform containing: translate(50, 50) * specifiedTransforms * translate(-50, -50). In RenderLayer::paint, you're computing the layer transform passed to the GraphicsContext via: m_transform * translate(100, 100), and store subPixelAccumulation="0.5, 0.5". Then In RenderLayer::paintLayerContents, you're adding the subPixelAccumulation to the paintOffset, so you're effectively aligning the layer on pixel boundaries, but compensate fractional sizes by translating the context before painting. Am I reading your code correctly?

Comment on attachment 148136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148136&action=review >> Source/WebCore/ChangeLog:20 >> + (WebCore::sizingBox): Adding a bool for pixel snapping. > > Aha, your code doesn't do that, it adds a new function duplicating sizingBox() functionality, but pixel-snaps. See below for a possible suggestion to avoid that. You caught me! >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:707 >> + IntRect box = pixelSnappedSizingBox(renderer); > > Hm, can't you just use pixelSnappedIntRect(sizingBox(renderer)) here? > From my understanding this should be equivalent, but avoids duplicating the logic of sizingBox() just to pixel-snap the values. The issue is that sizingBox can return RenderBox::borderBoxRect, which while maintaining the sub-pixel precision of the width and height, zeros out the location, resulting in incorrect pixel snapping. >> Source/WebCore/rendering/RenderInline.h:138 >> + virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, bool fixed, bool useTransforms, TransformState&, TransformingSubPixelOffsetBehavior adjustSubPixelOffsets = SnapOffsetsForTransforms, ApplyContainerFlipOrNot = ApplyContainerFlip, bool* wasFixed = 0) const OVERRIDE; > > All these bools should go away IMHO. And I'd rather do this now before bloating mapLocalToContainer any further with new extra arguments. > (Though not your fault, but we should clean this up rather today than tomorrow :-) > > enum MapLocalToContainerMode { > IsFixed = 1 << 0, > UseTransforms = 1 << 1, > ApplyContainerFlip = 1 << 2, > SnapOffsetForTransforms 1 << 3; > } > typedef unsigned MapLocalToContainerFlags; > virtual void mapLocalToContainer(RenderBoxModelObject* repaintContainer, TransformState&, MapLocalToContainerFlags = SnapOffsetForTransforms | ApplyContainerFlip, bool* wasFixed = 0) const OVERRIDE; > > I didn't investigate in-detail if that would make some call-sites worse to read, but it should improve readability in general. I'm happy to give this a try. The bools are bad and getting worse. >> Source/WebCore/rendering/RenderLayer.cpp:549 >> + box->style()->applyTransform(*m_transform, box->pixelSnappedBorderBoxRect().size(), RenderStyle::IncludeTransformOrigin); > > I'd like to fully understand the effects of this change, please correct me if I got anything wrong: > > This change is relevant for computing the transform-origin induced translation (+ the argument is also passed to the TransformOperations::apply function). > You're now using pixel-snapped values here. This is correct as you're aligning the layer at pixel boundaries, but respect any fractional translation while computing the layer transform (in RenderLayer::paint). > > So for borderBoxRect.size = 100.5 x 100.5, and a transform-origin of 50% 50%, this now results in a m_transform containing: translate(50, 50) * specifiedTransforms * translate(-50, -50). > In RenderLayer::paint, you're computing the layer transform passed to the GraphicsContext via: m_transform * translate(100, 100), and store subPixelAccumulation="0.5, 0.5". > Then In RenderLayer::paintLayerContents, you're adding the subPixelAccumulation to the paintOffset, so you're effectively aligning the layer on pixel boundaries, but compensate fractional sizes by translating the context before painting. > > Am I reading your code correctly? Currently, we carry a sub-pixel value down through paint offsets that influences the way things are pixel snapped. This is broken with Layers. We avoid translating the transform by sub-pixel values (this would be wrong for a relatively positioned object that just happens to be an a sub-pixel offset, for example), but the current code also drops the sub-pixel remainder on the floor, which causes elements in a layer to round differently than otherwise identical elements that aren't. As for your example, you're making the mistake of considering a fractional size without a location, which you can't quite get away with with sub-pixel layout enabled.

Created attachment 155600 [details] Patch. Patch.

Created attachment 155618 [details] Patch

Comment on attachment 155618 [details] Patch Attachment 155618 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13394811

Created attachment 155629 [details] Patch

Comment on attachment 155629 [details] Patch Attachment 155629 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13401543 New failing tests: fast/transforms/rotated-transform-affects-scrolling-2.html fast/repaint/transform-translate.html transforms/svg-vs-css.xhtml fast/repaint/transform-absolute-child.html fast/sub-pixel/layout-boxes-with-zoom.html media/audio-repaint.html fast/transforms/transformed-document-element.html fast/repaint/transform-repaint-descendants.html media/video-zoom-controls.html fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html svg/transforms/svg-css-transforms.xhtml compositing/shadows/shadow-drawing.html fast/multicol/vertical-lr/break-properties.html fast/forms/validation-message-appearance.html fast/multicol/vertical-rl/break-properties.html fast/repaint/repaint-across-writing-mode-boundary.html fast/block/float/shrink-to-avoid-float-complexity.html fast/sub-pixel/sub-pixel-accumulates-to-layers.html fast/multicol/break-properties.html

Created attachment 155641 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 155909 [details] Patch

Comment on attachment 155909 [details] Patch Attachment 155909 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13421237 New failing tests: fast/transforms/rotated-transform-affects-scrolling-2.html fast/repaint/transform-translate.html transforms/svg-vs-css.xhtml fast/repaint/transform-absolute-child.html media/audio-repaint.html fast/transforms/transformed-document-element.html fast/repaint/transform-repaint-descendants.html fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html svg/transforms/svg-css-transforms.xhtml compositing/shadows/shadow-drawing.html fast/forms/validation-message-appearance.html fast/sub-pixel/sub-pixel-accumulates-to-layers.html fast/overflow/overflow-update-transform.html

Created attachment 155925 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 157611 [details] Patch

Comment on attachment 157611 [details] Patch Attachment 157611 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13473189 New failing tests: fast/repaint/transform-translate.html transforms/svg-vs-css.xhtml fast/repaint/transform-absolute-child.html media/audio-repaint.html fast/transforms/transformed-document-element.html fast/repaint/transform-repaint-descendants.html fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html svg/transforms/svg-css-transforms.xhtml compositing/shadows/shadow-drawing.html

Created attachment 157620 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 157611 [details] Patch Attachment 157611 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13471387

Created attachment 157746 [details] Patch

Created attachment 158038 [details] Patch

Comment on attachment 158038 [details] Patch Attachment 158038 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13493356 New failing tests: fast/repaint/transform-translate.html transforms/svg-vs-css.xhtml fast/repaint/transform-absolute-child.html media/audio-repaint.html fast/transforms/transformed-document-element.html fast/repaint/transform-repaint-descendants.html fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html svg/transforms/svg-css-transforms.xhtml compositing/shadows/shadow-drawing.html fast/sub-pixel/sub-pixel-accumulates-to-layers.html

Created attachment 158165 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Smfr, any chance you can take a look at this?

Comment on attachment 158038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158038&action=review This change looks fine. > Source/WebCore/rendering/RenderLayer.h:719 > + void paintLayerContents(RenderLayer* rootLayer, GraphicsContext*, const LayoutRect& paintDirtyRect, const LayoutSize& subPixelAccumulation, You mentioned you wanted to ASSERT these were never over 1,1. > Source/WebCore/rendering/RenderObject.h:692 > + FloatQuad localToContainerQuad(const FloatQuad&, RenderBoxModelObject* repaintContainer, bool snapOffsetForTransforms = true, bool fixed = false, bool* wasFixed = 0) const; > + FloatPoint localToContainerPoint(const FloatPoint&, RenderBoxModelObject* repaintContainer, bool snapOffsetForTransforms = true, bool fixed = false, bool* wasFixed = 0) const; Not another bool!

Created attachment 158674 [details] Patch for landing

Comment on attachment 158674 [details] Patch for landing Attachment 158674 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13504793 New failing tests: fast/repaint/transform-translate.html transforms/svg-vs-css.xhtml fast/repaint/transform-absolute-child.html media/audio-repaint.html fast/transforms/transformed-document-element.html fast/repaint/transform-repaint-descendants.html fast/repaint/scroll-fixed-layer-with-transformed-parent-layer.html svg/transforms/svg-css-transforms.xhtml compositing/shadows/shadow-drawing.html fast/sub-pixel/sub-pixel-accumulates-to-layers.html

Created attachment 158687 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Committed r125794: <http://trac.webkit.org/changeset/125794>

*** Bug 91233 has been marked as a duplicate of this bug. ***