Bug 124042

Created attachment 216398 [details] patch

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.

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.

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.

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

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

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.

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).

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).

Comment on attachment 221971 [details] patch Clearing flags on attachment: 221971 Committed r162605: <http://trac.webkit.org/changeset/162605>

All reviewed patches have been landed. Closing bug.