Bug 107880 - [CSS Regions] Region boxes should respect -shape-inside CSS property
Summary: [CSS Regions] Region boxes should respect -shape-inside CSS property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zoltan Horvath
URL:
Keywords:
: 109858 (view as bug list)
Depends on: 74132
Blocks: 57312 110445
  Show dependency treegraph
 
Reported: 2013-01-24 16:49 PST by Zoltan Horvath
Modified: 2013-02-25 02:08 PST (History)
15 users (show)

See Also:


Attachments
basic test case (1.61 KB, text/html)
2013-01-24 16:49 PST, Zoltan Horvath
no flags Details
proposed patch - based on #74132 => won't compile until we landing that (9.48 KB, patch)
2013-02-14 13:41 PST, Zoltan Horvath
zoltan: commit-queue-
Details | Formatted Diff | Diff
#74132 is landed, reupload to fire the bots (9.48 KB, patch)
2013-02-15 10:04 PST, Zoltan Horvath
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
#74132 is landed, reupload to fire the bots (9.77 KB, patch)
2013-02-15 12:02 PST, Zoltan Horvath
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Added support for additional cases as well (12.58 KB, patch)
2013-02-19 06:13 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
Added support for additional cases as well (12.67 KB, patch)
2013-02-19 06:23 PST, Zoltan Horvath
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Added support for additional cases as well (12.55 KB, patch)
2013-02-19 06:42 PST, Zoltan Horvath
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch, based on Mihnea's review (12.85 KB, patch)
2013-02-19 08:57 PST, Zoltan Horvath
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch, based on Mihnea's review + test fixed for cr-linux (9.08 KB, text/plain)
2013-02-21 01:28 PST, Zoltan Horvath
no flags Details
Updated patch, based on Mihnea's review + test fixed for cr-linux (12.56 KB, patch)
2013-02-21 01:32 PST, Zoltan Horvath
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
proposed patch (12.02 KB, patch)
2013-02-22 02:23 PST, Zoltan Horvath
hyatt: review+
zoltan: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>