Bug 124680 – [CSS Shapes] ShapeOutsideInfo needs to use the parent's writing mode when calculating offsets

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

[Bear Travis]

Created attachment 217482 [details]
Test case

ShapeOutsideInfo is currently using its element's writing mode to setup the shape and compute line offsets. This needs to be done in writing mode of the parent of the line box instead.

[Rob Buis]

Created attachment 218523 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 218523 [details]
Patch

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

> Source/WebCore/rendering/shapes/ShapeInfo.cpp:63
> +    WritingMode writingMode = m_renderer.isFloating() ? m_renderer.containingBlock()->style().writingMode() : m_renderer.style().writingMode();

You actually need to switch on it being shape-outside versus shape-inside because we can have the situation where m_renderer is floating and we want to compute the shape-inside for it's content, and in that case, we need to use m_renderer's writing mode, not that of the parent.

[Bear Travis]

You may want to factor out a virtual method for ShapeOutsideInfo vs ShapeInsideInfo to solve the problem Bem mentioned.

Also, you don't always want the containing block's writing mode. For example:
<div>
    <div style='shape-outside: content-box; float: left;'></div>
    <div style='writing-mode: vertical-lr'>I will be affected by the shape</div>
</div>

You may also have overhanging floats:
<div>
    <div style='shape-outside: content-box; float: left;'>I am bigger than my container</div>
</div>
<div style='writing-mode: vertical-lr'>I will be affected by the shape</div>

So you'll need to work out a transformation between the writing mode of the inline content, and whatever coordinate system the float is using. I think it is probably more than one patch.

[Bem Jones-Bey]

(In reply to comment #3)
> You may want to factor out a virtual method for ShapeOutsideInfo vs ShapeInsideInfo to solve the problem Bem mentioned.
> 
> Also, you don't always want the containing block's writing mode. For example:
> <div>
>     <div style='shape-outside: content-box; float: left;'></div>
>     <div style='writing-mode: vertical-lr'>I will be affected by the shape</div>
> </div>
> 
> You may also have overhanging floats:
> <div>
>     <div style='shape-outside: content-box; float: left;'>I am bigger than my container</div>
> </div>
> <div style='writing-mode: vertical-lr'>I will be affected by the shape</div>
> 
> So you'll need to work out a transformation between the writing mode of the inline content, and whatever coordinate system the float is using. I think it is probably more than one patch.

Bear, your example is incorrect; a writing mode establishes a block formatting context, and floats don't cross BFCs, so the float won't affect the content in your example. Overhanging/intruding floats also can only affect content with the same writing mode as their containing block, so checking the writing mode on the containing block is sufficient.

[Rob Buis]

Created attachment 218527 [details]
Patch

[Rob Buis]

Created attachment 218537 [details]
Patch

[Build Bot]

Comment on attachment 218537 [details]
Patch

Attachment 218537 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/44298010

New failing tests:
inspector-protocol/model/highlight-shape-outside.html

[Build Bot]

Created attachment 218545 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Rob Buis]

Created attachment 218547 [details]
Patch

[Dirk Schulze]

Comment on attachment 218547 [details]
Patch

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

I would like to hear Bems or Bears opinion before r=me. At the moment I do not understand what this change is about.

> Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:106
> +    if (m_renderer.isFloating()) {
> +        if (m_renderer.containingBlock())

this can be 

if (m_renderer.isFloating() && m_renderer.containingBlock())
    return m_renderer.containingBlock()->style().writingMode();

[Bem Jones-Bey]

Comment on attachment 218547 [details]
Patch

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

>> Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:106
>> +        if (m_renderer.containingBlock())
> 
> this can be 
> 
> if (m_renderer.isFloating() && m_renderer.containingBlock())
>     return m_renderer.containingBlock()->style().writingMode();

ShapeOutside shouldn't ever use the internal writing mode, since it always affects content outside of the block, so I'd remove the isFloating() check. As for the containingBlock() check: will we ever be in a situation where we have a valid shape-outside and don't have a containingBlock? I'd lean more towards an ASSERT for this and to remove the if statement entirely.

> LayoutTests/TestExpectations:72
> +# A problem in both EFL and GTK.

Doesn't this fail on Mac as well after this patch? Perhaps just remove the comment?

[Bear Travis]

The test passes on Mac, the root of the problem is whether rectangle paths contain the last line segment or omit it. It's a platform dependent issue (yay).

Looks fine to me.

[Rob Buis]

Created attachment 218611 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 218611 [details]
Patch

Clearing flags on attachment: 218611

Committed r160243: <http://trac.webkit.org/changeset/160243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.