Bug 132535 – Top content inset: Margin tiles should not display in the inset area when pinned to the top of the page

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

Top content inset: Margin tiles should not display in the inset area when pinned to the top of the page. 

<rdar://problem/16609602>

[Beth Dakin]

Oops, wrong radar number. I mean <rdar://problem/16613039>

[Beth Dakin]

Created attachment 230768 [details]
Patch

[WebKit Commit Bot]

Attachment 230768 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayerCompositor.cpp:1580:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/rendering/RenderLayerCompositor.cpp:1594:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 230768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230768&action=review

r+ but it would be nice to try to share more of the layer position math, and fix UI-side compositing (at least encode/decode the new values).

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:223
> +                positionForInsetClipLayer = FloatPoint();

No need, it's already 0,0

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:226
> +                positionForInsetClipLayer = FloatPoint(0, topContentInset - scrollY);

positionForInsetClipLayer.setY(...)?

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:235
> +                if (insetClipLayer)
> +                    insetClipLayer->setPosition(positionForInsetClipLayer);

Why did we spend all that time computing positionForInsetClipLayer when we don't have one :(

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:128
> +    // This is a clipping layer that will scroll with the page for all y-delta scroll values between 0
> +    // and topContentInset(). Once the y-deltas get beyond the content inset point, this layer no longer
> +    // needs to move.

Comment only applies to some configurations.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1603
> +FloatPoint RenderLayerCompositor::positionForClipLayer() const
> +{
> +    float contentInset = m_renderView.frameView().topContentInset();
> +    if (contentInset == 0)
> +        return FloatPoint();
> +
> +    int scrollY = std::max(0, m_renderView.frameView().scrollPosition().y());
> +
> +    if (scrollY >= contentInset)
> +        return FloatPoint();
> +
> +    return FloatPoint(0, contentInset - scrollY);
> +}
> +
> +float RenderLayerCompositor::topContentOffsetForRootLayer() const
> +{
> +    float contentInset = m_renderView.frameView().topContentInset();
> +    if (contentInset == 0)
> +        return 0;
> +
> +    int scrollY = std::max(0, m_renderView.frameView().scrollPosition().y());
> +
> +    if (scrollY >= contentInset)
> +        return contentInset;
> +
> +    return scrollY;
> +}

Doesn't this duplicate code in RenderLayerCompositor and teh scrolling tree node? Can we factor into a static FrameView function?

[Build Bot]

Comment on attachment 230768 [details]
Patch

Attachment 230768 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5751922292162560

New failing tests:
compositing/columns/untransformed-composited-in-paginated.html
compositing/columns/hittest-composited-in-paginated.html
compositing/contents-opaque/body-background-painted.html
compositing/absolute-inside-out-of-view-fixed.html
compositing/columns/composited-nested-columns.html
compositing/columns/clipped-in-paginated.html
compositing/columns/composited-columns.html
compositing/backing/no-backing-for-clip-overhang.html
compositing/columns/rotated-in-paginated.html
compositing/columns/composited-columns-vertical-rl.html
compositing/columns/ancestor-clipped-in-paginated.html
compositing/contents-opaque/background-color.html
compositing/bounds-in-flipped-writing-mode.html
compositing/columns/composited-in-paginated-writing-mode-rl.html
compositing/backing/backface-visibility-in-3dtransformed.html
compositing/tiled-layers-hidpi.html
compositing/contents-opaque/filter.html
compositing/animation/layer-for-filling-animation.html
compositing/columns/composited-in-paginated.html
compositing/contents-opaque/body-background-skipped.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/columns/composited-in-paginated-rl.html
compositing/animation/filling-animation-overlap.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/animation/filling-animation-overlap-at-end.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/backing/no-backing-for-perspective.html
compositing/contents-opaque/control-layer.html
compositing/contents-opaque/background-clip.html

[Build Bot]

Created attachment 230771 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Tim Horton]

(In reply to comment #5)
> (From update of attachment 230768 [details])
> Attachment 230768 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/5751922292162560

You could adjust RLB::shouldDumpPropertyForLayer, but if the new results are consistent between all platforms I think you should just extend your rebaselining and not introduce additional lying in the logging.

[Build Bot]

Comment on attachment 230768 [details]
Patch

Attachment 230768 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4998075806384128

New failing tests:
compositing/columns/hittest-composited-in-paginated.html
compositing/contents-opaque/body-background-painted.html
compositing/absolute-inside-out-of-view-fixed.html
compositing/columns/composited-nested-columns.html
compositing/columns/clipped-in-paginated.html
compositing/columns/composited-columns.html
compositing/backing/no-backing-for-clip-overhang.html
compositing/columns/rotated-in-paginated.html
compositing/columns/composited-columns-vertical-rl.html
compositing/columns/ancestor-clipped-in-paginated.html
compositing/contents-opaque/background-color.html
compositing/bounds-in-flipped-writing-mode.html
compositing/columns/composited-in-paginated-writing-mode-rl.html
compositing/backing/backface-visibility-in-3dtransformed.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/contents-scale/animating.html
compositing/contents-opaque/filter.html
compositing/animation/layer-for-filling-animation.html
compositing/columns/composited-in-paginated.html
compositing/contents-opaque/hidden-with-visible-child.html
compositing/contents-opaque/body-background-skipped.html
compositing/contents-opaque/background-clip.html
compositing/columns/composited-in-paginated-rl.html
compositing/animation/filling-animation-overlap.html
compositing/backing/no-backing-for-clip.html
compositing/animation/filling-animation-overlap-at-end.html
compositing/columns/composited-lr-paginated-repaint.html
compositing/backing/no-backing-for-perspective.html
compositing/contents-opaque/control-layer.html
compositing/clip-child-by-non-stacking-ancestor.html

[Build Bot]

Created attachment 230777 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Beth Dakin]

I addressed all of Simon's comments, and I continued re-baselining the tests. I'm not sure if that was the right decision, because it ended up being a ton of tests, but it's done for now at least.

http://trac.webkit.org/changeset/168244

[Alexey Proskuryakov]

This patch broke compositing/visibility/visibility-image-layers-dynamic.html

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=compositing%2Fvisibility%2Fvisibility-image-layers-dynamic.html

Actually, maybe it broke all these tests? I didn't check.

http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r168244%20(19672)/results.html

[Beth Dakin]

(In reply to comment #11)
> This patch broke compositing/visibility/visibility-image-layers-dynamic.html
> 

Still investigating what those differences are all about with this bug:
https://bugs.webkit.org/show_bug.cgi?id=132551

> http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=compositing%2Fvisibility%2Fvisibility-image-layers-dynamic.html
> 
> Actually, maybe it broke all these tests? I didn't check.
> 
> http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r168244%20(19672)/results.html

It did cause other failures, but they were all straightforward re-baselines, and I have committed the re-baselines.

(In reply to comment #10)
> I addressed all of Simon's comments, and I continued re-baselining the tests. I'm not sure if that was the right decision, because it ended up being a ton of tests, but it's done for now at least.
> 
> http://trac.webkit.org/changeset/168244

This caused bug 136019.