Bug 107880

Summary: [CSS Regions] Region boxes should respect -shape-inside CSS property
Product: WebKit Reporter: Zoltan Horvath <zoltan>
Component: CSSAssignee: Zoltan Horvath <zoltan>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dglazkov, eric, esprehn+autocc, jchaffraix, mihnea, ojan.autocc, peter+ews, rego+ews, rniwa, vsevik, WebkitBugTracker, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 74132    
Bug Blocks: 57312, 110445    
Attachments:
Description Flags
basic test case
none
proposed patch - based on #74132 => won't compile until we landing that
zoltan: commit-queue-
#74132 is landed, reupload to fire the bots
webkit.review.bot: commit-queue-
#74132 is landed, reupload to fire the bots
webkit.review.bot: commit-queue-
Added support for additional cases as well
none
Added support for additional cases as well
webkit-ews: commit-queue-
Added support for additional cases as well
webkit.review.bot: commit-queue-
Updated patch, based on Mihnea's review
webkit.review.bot: commit-queue-
Updated patch, based on Mihnea's review + test fixed for cr-linux
none
Updated patch, based on Mihnea's review + test fixed for cr-linux
hyatt: review-, hyatt: commit-queue-
proposed patch hyatt: review+, zoltan: commit-queue-

Description Zoltan Horvath 2013-01-24 16:49:45 PST
Created attachment 184611 [details]
basic test case

Region boxes should respect -shape-inside CSS property
Comment 1 Zoltan Horvath 2013-02-14 13:41:24 PST
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 2 Early Warning System Bot 2013-02-14 14:05:46 PST
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 3 Early Warning System Bot 2013-02-14 14:08:42 PST
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 4 WebKit Review Bot 2013-02-14 14:55:17 PST
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 5 WebKit Review Bot 2013-02-14 15:43:01 PST
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 6 EFL EWS Bot 2013-02-14 15:46:30 PST
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 7 Peter Beverloo (cr-android ews) 2013-02-14 16:38:28 PST
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 8 Build Bot 2013-02-14 19:49:12 PST
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 9 Build Bot 2013-02-15 04:47:14 PST
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
Comment 10 Zoltan Horvath 2013-02-15 10:04:19 PST
Created attachment 188594 [details]
#74132 is landed, reupload to fire the bots
Comment 11 WebKit Review Bot 2013-02-15 11:16:03 PST
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 12 Build Bot 2013-02-15 11:51:54 PST
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
Comment 13 Zoltan Horvath 2013-02-15 12:02:33 PST
Created attachment 188613 [details]
#74132 is landed, reupload to fire the bots
Comment 14 WebKit Review Bot 2013-02-15 13:25:28 PST
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 15 WebKit Review Bot 2013-02-15 14:05:28 PST
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
Comment 16 Zoltan Horvath 2013-02-19 06:13:02 PST
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.
Comment 17 Zoltan Horvath 2013-02-19 06:23:48 PST
Created attachment 189074 [details]
Added support for additional cases as well
Comment 18 Early Warning System Bot 2013-02-19 06:29:51 PST
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 19 Early Warning System Bot 2013-02-19 06:32:02 PST
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 20 EFL EWS Bot 2013-02-19 06:34:45 PST
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 21 WebKit Review Bot 2013-02-19 06:34:57 PST
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 22 WebKit Review Bot 2013-02-19 06:36:46 PST
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
Comment 23 Zoltan Horvath 2013-02-19 06:42:23 PST
Created attachment 189079 [details]
Added support for additional cases as well
Comment 24 Zoltan Horvath 2013-02-19 06:57:50 PST
*** Bug 109858 has been marked as a duplicate of this bug. ***
Comment 25 Mihnea Ovidenie 2013-02-19 07:38:09 PST
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 26 WebKit Review Bot 2013-02-19 07:54:00 PST
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
Comment 27 Zoltan Horvath 2013-02-19 08:57:06 PST
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 28 WebKit Review Bot 2013-02-19 09:41:06 PST
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
Comment 29 Zoltan Horvath 2013-02-21 01:28:10 PST
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.
Comment 30 Zoltan Horvath 2013-02-21 01:32:59 PST
Created attachment 189473 [details]
Updated patch, based on Mihnea's review + test fixed for cr-linux
Comment 31 Dave Hyatt 2013-02-21 08:21:26 PST
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().
Comment 32 Zoltan Horvath 2013-02-22 02:23:12 PST
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 33 Zoltan Horvath 2013-02-22 02:25:04 PST
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 34 Dave Hyatt 2013-02-22 11:17:39 PST
Comment on attachment 189726 [details]
proposed patch

r=me
Comment 35 Zoltan Horvath 2013-02-22 12:04:43 PST
Thanks for the review Dave!

Landed in http://trac.webkit.org/changeset/143766.
Comment 36 Vsevolod Vlasov 2013-02-25 02:08:17 PST
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>