Bug 124042 – [CSS Regions] Fix positioning composited layers when the region has overflow:hidden

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mihai Maerean]

Composited layers are positioned differently when the region has overflow:hidden than when the region doesn't have overflow:hidden.

[Mihai Maerean]

Created attachment 216398 [details]
patch

[Alexandru Chiculita]

Comment on attachment 216398 [details]
patch

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

Interesting case. I have a few comments below.

> LayoutTests/compositing/regions/position-layer-inside-region-overflow-hidden-expected.html:3
> +		<title>Bug #124042: [CSS Regions] Fix positioning composited layers when the region has overflow:hidden</title>

nit: Use 4 spaces instead of tabs.

> LayoutTests/compositing/regions/position-layer-inside-region-overflow-hidden-expected.html:6
> +				-webkit-font-smoothing: none;

nit: Add a comment to explain why you need this.

> LayoutTests/compositing/regions/position-layer-inside-region-overflow-hidden-expected.html:18
> +				/*overflow: hidden;*/

nit: Remove commented code.

> LayoutTests/compositing/regions/position-layer-inside-region-overflow-hidden-expected.html:22
> +			* {

Avoid the * and just inline the class names that actually require this.

> LayoutTests/compositing/regions/position-layer-inside-region-overflow-hidden.html:23
> +				border:solid 1px #888;

I guess this will overwrite your border-width in .region. Is the border-width needed?

The HTML comments apply to both test and expected files.

> Source/WebCore/rendering/RenderLayerBacking.cpp:877
> +                RenderLayerBacking* layerOwnerBacking = layerOwner->layer()->backing();

nit: Put this line after the comment.

> Source/WebCore/rendering/RenderLayerBacking.cpp:887
> +                if (layerOwnerBacking->clippingLayer())

Can you make a test for the case when the clipping layer is not the region itself? Example: Nested composited layers inside the region where the parent one is clipping. I think this change is only needed when the clippingLayer is the region. In any case, I would rather use the RenderLayers instead of the GraphicsLayers to compute the correct location.

[Mihai Maerean]

Comment on attachment 216398 [details]
patch

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

>> Source/WebCore/rendering/RenderLayerBacking.cpp:887
>> +                if (layerOwnerBacking->clippingLayer())
> 
> Can you make a test for the case when the clipping layer is not the region itself? Example: Nested composited layers inside the region where the parent one is clipping. I think this change is only needed when the clippingLayer is the region. In any case, I would rather use the RenderLayers instead of the GraphicsLayers to compute the correct location.

I created the test you mentioned (nested composited layers inside the region where the parent one is clipping) and it passes on both the old and the new code.
I will add the test to the patch, even if it's not required.

[Mihai Maerean]

Comment on attachment 216398 [details]
patch

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

>> Source/WebCore/rendering/RenderLayerBacking.cpp:887
>> +                if (layerOwnerBacking->clippingLayer())
> 
> I would rather use the RenderLayers instead of the GraphicsLayers to compute the correct location.

The RenderLayer trees are exactly the same in the 2 cases (with and without the overflow:hidden).
The problem is an incorrect positioning of where the painting occurs.

[Mihai Maerean]

Created attachment 216673 [details]
patch. incorporates the feedback.

[Mihai Maerean]

Created attachment 221385 [details]
patch. incorporates the feedback.

[Mihnea Ovidenie]

Comment on attachment 221385 [details]
patch. incorporates the feedback.

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

r=me with some comments. I suggest you should consider for future some possible improvements:
* early returns
* do we need the code for the multicol based regions here?

> Source/WebCore/ChangeLog:8
> +        If there's a clipping GraphicsLayer on the hierarchy, substract its offset, since it's its parent that

I would like you to rephase it to make it more clear:
* what hierarchy you are talking about
* which parent you are talking about

> Source/WebCore/ChangeLog:10
> +        The position must also be correct when the region has box-shadow that inflates the region's layer.

Please put this line as a comment in the changelog near adjustAncestorCompositingBoundsForFlowThread instead of here. See below.

> Source/WebCore/ChangeLog:11
> +        We must have the same position on the screen as when the parent doesn't have clipping.

Maybe it should be:
"The composited layers from the flow thread should be rendered in the same position whether the associated region has clipping or not".

> Source/WebCore/rendering/RenderLayerBacking.cpp:1003
> +                // positions us. We must have the same position on the screen as when the parent doesn't have clipping.

This is the same message as in the changelog, why do we need this duplication? I would keep here only:
"If there's a clipping GraphicsLayer on the hierarchy, substract its offset, since it's its parent that positions us" with the clarification about the hierarchy and the parent you are talking.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1006
> +                    // makes it also work with box-shadow that inflates the layer.

I would move this comment in the changelog, near the adjustAncestor.. function.

[Mihai Maerean]

Created attachment 221963 [details]
patch for landing

patch incorporates the feedback.
also, I have added 2 new tests:
nested composited layers in 1 and in 2 regions (with fragmentation).

[Mihai Maerean]

Created attachment 221971 [details]
patch

rebased patch that incorporates the feedback.
also, I have added 2 new tests:
nested composited layers in 1 and in 2 regions (with fragmentation).

[WebKit Commit Bot]

Comment on attachment 221971 [details]
patch

Clearing flags on attachment: 221971

Committed r162605: <http://trac.webkit.org/changeset/162605>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.