Bug 107880 – [CSS Regions] Region boxes should respect -shape-inside CSS property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Zoltan Horvath]

Created attachment 184611 [details]
basic test case

Region boxes should respect -shape-inside CSS property

[Zoltan Horvath]

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.

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[EFL EWS Bot]

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

[Peter Beverloo (cr-android ews)]

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

[Build Bot]

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

[Build Bot]

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

[Zoltan Horvath]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Zoltan Horvath]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Zoltan Horvath]

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.

[Zoltan Horvath]

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

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[EFL EWS Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Zoltan Horvath]

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

[Zoltan Horvath]

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

[Mihnea Ovidenie]

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.

[WebKit Review Bot]

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

[Zoltan Horvath]

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.

[WebKit Review Bot]

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

[Zoltan Horvath]

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.

[Zoltan Horvath]

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

[Dave Hyatt]

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

[Zoltan Horvath]

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.

[Zoltan Horvath]

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.

[Dave Hyatt]

Comment on attachment 189726 [details]
proposed patch

r=me

[Zoltan Horvath]

Thanks for the review Dave!

Landed in http://trac.webkit.org/changeset/143766.

[Vsevolod Vlasov]

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>