Bug 128693 – [CSS Shapes] Adjust lineTop position to the next available wrapping location at shape-outsides

• Component: CSS
• Status: RESOLVED FIXED

[Zoltan Horvath]

This patch is part of a set of fixes, which is tracked under bug #128340.
When we don't have space next to the floating container, but we have space inside the floating-container next to the defined shape-outside, we should wrap around the shape-outside.
This patch fixes the case when the top of the line (from the 2nd line in the block) needs to be adjusted to the position where the content fits next.
Solving the issue for the first line requires additional changes, since due to a fast code-path if it can't fits at the first line, it just pushed under the floating box. I opened a separate bug for that: bug #128692.

[Zoltan Horvath]

Created attachment 224009 [details]
Patch

[Zoltan Horvath]

Created attachment 224012 [details]
Patch

[Zoltan Horvath]

Created attachment 224048 [details]
update patch after r164006

[Bem Jones-Bey]

Comment on attachment 224048 [details]
update patch after r164006

I don't think this is the correct approach. It doesn't work for stacked floats, and is going to be very hard at bast to make it work in cases where you have floats on either side of the container. I think the first solution should be to simply change the line layout code to not jump to the bottom of the float if it has shape-outside and just walk down the float. This way, stacked floats, etc will all work. Then we should attack trying to make it position in a more optimal manner.

[Zoltan Horvath]

Created attachment 224765 [details]
Patch

I moved the logic into fitBelowFloats.

[Zoltan Horvath]

Created attachment 224789 [details]
Patch

[Zoltan Horvath]

Created attachment 224790 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 224790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224790&action=review

In general, I like this much better than the last attempt.

Why didn't you change all of the calls to fitBelowFloats to pass in the height argument when shape outside is enabled?

Also, I think it would be good if Hyatt had a look at this.

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:787
> +                m_width.fitBelowFloats(lineHeight);

Make fitBelowFloats take m_block and isFirstLine, and then do the lineHeight computation in fitBelowFloats. That way, you don't need all of this nastiness in the call.

> Source/WebCore/rendering/line/LineWidth.cpp:169
> +void LineWidth::updateLineDimension(LayoutUnit newLineTop, LayoutUnit newLineWidth)

This should not be in the compile guard.

[Zoltan Horvath]

Created attachment 224912 [details]
Patch

(In reply to comment #8)
> (From update of attachment 224790 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224790&action=review
> 
> In general, I like this much better than the last attempt.
> 
> Why didn't you change all of the calls to fitBelowFloats to pass in the height argument when shape outside is enabled?

I changed only the the sites, where I also had tests as well. - In this patch I updated the 2 other places, but I don't know any use case for those with shape-outside at this point.

> Also, I think it would be good if Hyatt had a look at this.
> 
> > Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:787
> > +                m_width.fitBelowFloats(lineHeight);
> 
> Make fitBelowFloats take m_block and isFirstLine, and then do the lineHeight computation in fitBelowFloats. That way, you don't need all of this nastiness in the call.

LineWidth has got an m_block reference. However, I updated to pass isFirstLine, I don't think it's less/more nastier than passing LineHeight. LineHeight is going to be more interesting once we support variable line-heights.

> > Source/WebCore/rendering/line/LineWidth.cpp:169
> > +void LineWidth::updateLineDimension(LayoutUnit newLineTop, LayoutUnit newLineWidth)
> 
> This should not be in the compile guard.

Good catch. I moved it out, thanks!

[Bem Jones-Bey]

Comment on attachment 224912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224912&action=review

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:784
> +#if ENABLE(CSS_SHAPES)

Why do you still have this #if ENABLE here? why not just pass isFiestLine all the time? (same for all the calls to fitBelowFloats)

[Dave Hyatt]

Comment on attachment 224912 [details]
Patch

r=me

[Zoltan Horvath]

Created attachment 225097 [details]
Patch for landing

[Zoltan Horvath]

Comment on attachment 225097 [details]
Patch for landing

I removed the guards too.

[WebKit Commit Bot]

Comment on attachment 225097 [details]
Patch for landing

Clearing flags on attachment: 225097

Committed r164613: <http://trac.webkit.org/changeset/164613>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.