RESOLVED FIXED 124042
[CSS Regions] Fix positioning composited layers when the region has overflow:hidden
https://bugs.webkit.org/show_bug.cgi?id=124042
Summary [CSS Regions] Fix positioning composited layers when the region has overflow:...
Mihai Maerean
Reported 2013-11-08 06:46:22 PST
Composited layers are positioned differently when the region has overflow:hidden than when the region doesn't have overflow:hidden.
Attachments
patch (7.34 KB, patch)
2013-11-08 07:58 PST, Mihai Maerean
achicu: review-
achicu: commit-queue-
patch. incorporates the feedback. (15.09 KB, patch)
2013-11-12 07:36 PST, Mihai Maerean
no flags
patch. incorporates the feedback. (11.33 KB, patch)
2014-01-16 09:47 PST, Mihai Maerean
mihnea: review+
mihnea: commit-queue-
patch for landing (21.72 KB, patch)
2014-01-23 01:38 PST, Mihai Maerean
no flags
patch (21.62 KB, patch)
2014-01-23 05:10 PST, Mihai Maerean
no flags
Mihai Maerean
Comment 1 2013-11-08 07:58:10 PST
Alexandru Chiculita
Comment 2 2013-11-08 11:16:30 PST
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 3 2013-11-11 04:30:46 PST
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 4 2013-11-12 01:57:26 PST
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
Comment 5 2013-11-12 07:36:46 PST
Created attachment 216673 [details] patch. incorporates the feedback.
Mihai Maerean
Comment 6 2014-01-16 09:47:15 PST
Created attachment 221385 [details] patch. incorporates the feedback.
Mihnea Ovidenie
Comment 7 2014-01-22 05:35:28 PST
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
Comment 8 2014-01-23 01:38:15 PST
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
Comment 9 2014-01-23 05:10:53 PST
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 10 2014-01-23 06:15:29 PST
Comment on attachment 221971 [details] patch Clearing flags on attachment: 221971 Committed r162605: <http://trac.webkit.org/changeset/162605>
WebKit Commit Bot
Comment 11 2014-01-23 06:15:33 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.