Bug 107880

Created attachment 188416 [details] proposed patch - based on #74132 => won't compile until we landing that I'm uploading this work in pieces to make the review process smoother. The first part is for adding support the cases like we apply shape-inside property onto a simple region. This work is based on the patch from #74132 so it won't compile now.

Comment on attachment 188416 [details] proposed patch - based on #74132 => won't compile until we landing that Attachment 188416 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16543812

Comment on attachment 188416 [details] proposed patch - based on #74132 => won't compile until we landing that Attachment 188416 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16550823

Comment on attachment 188416 [details] proposed patch - based on #74132 => won't compile until we landing that Attachment 188416 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16559646

Comment on attachment 188416 [details] proposed patch - based on #74132 => won't compile until we landing that Attachment 188416 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16543851

Comment on attachment 188416 [details] proposed patch - based on #74132 => won't compile until we landing that Attachment 188416 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16537804

Comment on attachment 188416 [details] proposed patch - based on #74132 => won't compile until we landing that Attachment 188416 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16580573

Comment on attachment 188416 [details] proposed patch - based on #74132 => won't compile until we landing that Attachment 188416 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16537897

Comment on attachment 188416 [details] proposed patch - based on #74132 => won't compile until we landing that Attachment 188416 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16593068

Created attachment 188594 [details] #74132 is landed, reupload to fire the bots

Comment on attachment 188594 [details] #74132 is landed, reupload to fire the bots Attachment 188594 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16585230 New failing tests: fast/regions/shape-inside-on-region.html

Comment on attachment 188594 [details] #74132 is landed, reupload to fire the bots Attachment 188594 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16581955 New failing tests: fast/regions/shape-inside-on-region.html

Created attachment 188613 [details] #74132 is landed, reupload to fire the bots

Comment on attachment 188613 [details] #74132 is landed, reupload to fire the bots Attachment 188613 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16589249 New failing tests: fast/regions/shape-inside-on-region.html

Comment on attachment 188613 [details] #74132 is landed, reupload to fire the bots Attachment 188613 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16600104 New failing tests: fast/regions/shape-inside-on-region.html

Created attachment 189071 [details] Added support for additional cases as well Support -shape-inside property on regions. This fix covers the additional cases as well.

Created attachment 189074 [details] Added support for additional cases as well

Comment on attachment 189074 [details] Added support for additional cases as well Attachment 189074 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16627546

Comment on attachment 189074 [details] Added support for additional cases as well Attachment 189074 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16610812

Comment on attachment 189074 [details] Added support for additional cases as well Attachment 189074 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16617686

Comment on attachment 189074 [details] Added support for additional cases as well Attachment 189074 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16611797

Comment on attachment 189074 [details] Added support for additional cases as well Attachment 189074 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16621721

Created attachment 189079 [details] Added support for additional cases as well

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

Comment on attachment 189071 [details] Added support for additional cases as well View in context: https://bugs.webkit.org/attachment.cgi?id=189071&action=review > LayoutTests/fast/regions/shape-inside-on-regions.html:14 > + width: 300px; All regions, except region1 are defining width. You should add the width to region1 and remove it from the class definition. > LayoutTests/fast/regions/shape-inside-on-regions.html:35 > + <div id="rectangle"> I would like to see a mention about the bug that is tested, like <p> Bug <a href="http://webkit.org/b/XXXX">XXXX</a>: Regions should respect shape-inside</p> > Source/WebCore/ChangeLog:9 > + I would like to see more in description about this bug. Can you put a link to shape-inside property from the spec? Can you give more details about what "basic support" means in this case? What is the general approach when solving it? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:85 > + if (renderFlowThread) { Why do you need this test? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1542 > + exclusionShapeInsideInfo = layoutExclusionShapeInsideInfo(this); How was exclusionShapeInsideInfo computed here before since you are testing it below? > Source/WebCore/rendering/RenderRegion.cpp:270 > } Why do you need this here? I thought we had code to compute exclusion shape for blocks and region is a render block. > Source/WebCore/rendering/ExclusionShapeInfo.cpp:63 > + logicalTopOffset += toRenderRegion(m_renderer)->logicalTopForFlowThreadContent(); I would like to see an explanation about what this code is trying to achieve.

Comment on attachment 189079 [details] Added support for additional cases as well Attachment 189079 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16616709 New failing tests: fast/regions/shape-inside-on-regions.html

Created attachment 189098 [details] Updated patch, based on Mihnea's review (In reply to comment #25) > (From update of attachment 189071 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189071&action=review > > > LayoutTests/fast/regions/shape-inside-on-regions.html:14 > > + width: 300px; > > All regions, except region1 are defining width. You should add the width to region1 and remove it from the class definition. Fixed. > > LayoutTests/fast/regions/shape-inside-on-regions.html:35 > > + <div id="rectangle"> > > I would like to see a mention about the bug that is tested, like <p> Bug <a href="http://webkit.org/b/XXXX">XXXX</a>: Regions should respect shape-inside</p> I added a link to the bug. > > Source/WebCore/ChangeLog:9 > > + > > I would like to see more in description about this bug. Can you put a link to shape-inside property from the spec? Can you give more details about what "basic support" means in this case? What is the general approach when solving it? I added reference to the spec and explanation to shape-inside. > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:85 > > + if (renderFlowThread) { > > Why do you need this test? We don't. I removed it. > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1542 > > + exclusionShapeInsideInfo = layoutExclusionShapeInsideInfo(this); > > How was exclusionShapeInsideInfo computed here before since you are testing it below? I added explanation to the changelog. > > Source/WebCore/rendering/RenderRegion.cpp:270 > > } > > Why do you need this here? I thought we had code to compute exclusion shape for blocks and region is a render block. Good catch. It's no longer needed since the region-block refactoring is landed. > > Source/WebCore/rendering/ExclusionShapeInfo.cpp:63 > > + logicalTopOffset += toRenderRegion(m_renderer)->logicalTopForFlowThreadContent(); > > I would like to see an explanation about what this code is trying to achieve. I added a comment to the source to make it clear.

Comment on attachment 189098 [details] Updated patch, based on Mihnea's review Attachment 189098 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16617768 New failing tests: fast/regions/shape-inside-on-regions.html

Created attachment 189472 [details] Updated patch, based on Mihnea's review + test fixed for cr-linux It was a font-sizing + shape-inside + regions + cr linux issue on cr linux with the test, I fixed the test. It's a separate issue, I'll file a bug for it.

Created attachment 189473 [details] Updated patch, based on Mihnea's review + test fixed for cr-linux

Comment on attachment 189473 [details] Updated patch, based on Mihnea's review + test fixed for cr-linux View in context: https://bugs.webkit.org/attachment.cgi?id=189473&action=review r=me Remember that logicalHeightForLine is really just a heuristic that can be wrong. You should make a more complex test case that actually has multiple paragraphs inside the flow thread to illustrate this issue. Basically just make sure to add in the offsetFromLogicalTopOfFirstPage and you'll be fine, but let's get a test case that covers this. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:84 > + LayoutUnit offset = block->logicalHeight() + logicalHeightForLine(block, false); This offset isn't correct. It needs to be in the coordinates of the object that you called regionAtBlockOffset on. That's the enclosingRenderFlowThread().

Created attachment 189726 [details] proposed patch As we discussed on IRC I modified layoutExclusionShapeInsideInfo function to use the right coordinate space and I also modified the test, so the flow thread contains paragraphs and nested paragraph now.

Comment on attachment 189726 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=189726&action=review > LayoutTests/fast/regions/shape-inside-on-regions-expected.html:38 > + <p style="color: green; margin-top 0px; padding-top: 0px;">X X</p> margin-top and padding-top are not necessary here. I can remove this before land.

Comment on attachment 189726 [details] proposed patch r=me

Thanks for the review Dave! Landed in http://trac.webkit.org/changeset/143766.

Layout test fast/regions/shape-inside-on-regions.html fails on Chromium Mountain Lion after this patch Filed https://bugs.webkit.org/show_bug.cgi?id=110739 Skipped in <http://trac.webkit.org/changeset/143897>