Bug 106927 – [CSS Exclusions] Handle shape-outside changing a float's overhang behavior

• Component: CSS
• Status: RESOLVED FIXED

[Bem Jones-Bey]

When a float has shape-outside, it is possible for the shape to be positioned such that either only the shape overflows into a subsequent block or only the float does. This needs to be handled properly.

[Bem Jones-Bey]

This bug is supposed to be about overhang, not overflow. Overflow is another topic altogether, which will be handled in Bug 107594.

[Bem Jones-Bey]

Created attachment 184570 [details]
Patch

Initial implementation

[Alexandru Chiculita]

Comment on attachment 184570 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:335
> +    ExclusionShapeOutsideInfo* shapeOutside = exclusionShapeOutsideInfo();
> +    if (isFloating() && shapeOutside)

isFloating is a quick check in the m_bitfields, so I would only lookup the shape outside info just in case we have a float. (Initialize the shapeOutside variable after the isFloating() == true)

> Source/WebCore/rendering/RenderBox.h:634
> +    // for situations in which the flags on the box will be out of sync with

Did you actually find any case where that happens? I would run all the tests without using this hack and investigate what happens.

[Bem Jones-Bey]

Comment on attachment 184570 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:335
>> +    if (isFloating() && shapeOutside)
> 
> isFloating is a quick check in the m_bitfields, so I would only lookup the shape outside info just in case we have a float. (Initialize the shapeOutside variable after the isFloating() == true)

Ok.

>> Source/WebCore/rendering/RenderBox.h:634
>> +    // for situations in which the flags on the box will be out of sync with
> 
> Did you actually find any case where that happens? I would run all the tests without using this hack and investigate what happens.

One of the places I use this (RenderBox.cpp:1857) has a comment stating that it does happen. The other place (RenderBox.cpp:1606) does not have any such comment, but I figured I should play it safe here and not change it unless I had to. I do know that the tests all pass if  (RenderBox.cpp:1606) does not have this hack. I did not test with (RenderBox.cpp:1857) given that the comment explicitly says that it must check on the style.

[Bem Jones-Bey]

Created attachment 184594 [details]
Patch

Update based on feedback

[Bem Jones-Bey]

The original approach to positioning was done in bug 100399.

[Julien Chaffraix]

Comment on attachment 184594 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        When the position on a shapw outside causes a float to spill out into

typo: shapw

> Source/WebCore/ChangeLog:10
> +        looking at a fix, the simaliarities between a float with shape outside

typo: simaliarities

> Source/WebCore/ChangeLog:13
> +        to float positioning has been changed to be similar to the
> +        implementation for relative positioning by using a RenderLayer. This

I have an issue with this explanation: relative positioning only shifts the content *at paint time*. shape-outside changes the layout so both concepts seems different in how they work to warrant more explanations.

> Source/WebCore/ChangeLog:39
> +

If this route pans out (which I am not convinced ATM), you would need to also touch RenderBox::computeRectForRepaint as it checks position before adding the offsetForInFlowPosition but wasn't amended to cover the new condition.

> Source/WebCore/rendering/RenderObject.h:533
> +    bool isInFlowPositioned() const
> +    {
> +        bool positioned = m_bitfields.isRelPositioned() || m_bitfields.isStickyPositioned(); // relative or sticky positioning
> +#if ENABLE(CSS_EXCLUSIONS)
> +        // Shape outside on a float can reposition the float in much the
> +        // same way as relative positioning, so treat it as such.
> +        positioned = positioned || (m_bitfields.floating() && m_bitfields.isBox() && style()->shapeOutside());

I don't think it's valid to call a floating box in-flow or in normal flow per CSS 2.1 (see http://www.w3.org/TR/CSS2/visuren.html#comparison, especially the part about floats).

[Bem Jones-Bey]

(In reply to comment #7)
> (From update of attachment 184594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184594&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        When the position on a shapw outside causes a float to spill out into
> 
> typo: shapw
> 
> > Source/WebCore/ChangeLog:10
> > +        looking at a fix, the simaliarities between a float with shape outside
> 
> typo: simaliarities

Will fix these.

> 
> > Source/WebCore/ChangeLog:13
> > +        to float positioning has been changed to be similar to the
> > +        implementation for relative positioning by using a RenderLayer. This
> 
> I have an issue with this explanation: relative positioning only shifts the content *at paint time*. shape-outside changes the layout so both concepts seems different in how they work to warrant more explanations.

When a shape outside is on a float, the float positioning fixes the top and left/right (depending on if it's floar: right or float: left) coordinates of the shape in the containing block for layout purposes. The only thing that changes (as far as layout is concerned) is the height and width due to the shape. This means that if you give an x and y position to the shape outside when applied to a float, the effect only takes effect at paint time and positions the float at -x and -y relative to the shape position. Does that make more sense?

(I will also update the ChangeLog to be more explicit about the relationship with relative positioning)

> 
> > Source/WebCore/ChangeLog:39
> > +
> 
> If this route pans out (which I am not convinced ATM), you would need to also touch RenderBox::computeRectForRepaint as it checks position before adding the offsetForInFlowPosition but wasn't amended to cover the new condition.

You're right, I did miss that. I'll update that. Do you have any other concerns that you did not mention here?

> 
> > Source/WebCore/rendering/RenderObject.h:533
> > +    bool isInFlowPositioned() const
> > +    {
> > +        bool positioned = m_bitfields.isRelPositioned() || m_bitfields.isStickyPositioned(); // relative or sticky positioning
> > +#if ENABLE(CSS_EXCLUSIONS)
> > +        // Shape outside on a float can reposition the float in much the
> > +        // same way as relative positioning, so treat it as such.
> > +        positioned = positioned || (m_bitfields.floating() && m_bitfields.isBox() && style()->shapeOutside());
> 
> I don't think it's valid to call a floating box in-flow or in normal flow per CSS 2.1 (see http://www.w3.org/TR/CSS2/visuren.html#comparison, especially the part about floats).

Would you be happier if instead of changing isInFlowPositioned() and hasInFlowPosition(), I added new methods (these names are horrible, would have to spend more time thinking of good names) isInFlowPositionedOrFloatWithShapeOutside() and hasInFlowPositionedOrFloatWithShapeOutside() and changed the places that use the previous methods to use these methods?

[Julien Chaffraix]

Comment on attachment 184594 [details]
Patch

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

>>> Source/WebCore/ChangeLog:13
>>> +        implementation for relative positioning by using a RenderLayer. This
>> 
>> I have an issue with this explanation: relative positioning only shifts the content *at paint time*. shape-outside changes the layout so both concepts seems different in how they work to warrant more explanations.
> 
> When a shape outside is on a float, the float positioning fixes the top and left/right (depending on if it's floar: right or float: left) coordinates of the shape in the containing block for layout purposes. The only thing that changes (as far as layout is concerned) is the height and width due to the shape. This means that if you give an x and y position to the shape outside when applied to a float, the effect only takes effect at paint time and positions the float at -x and -y relative to the shape position. Does that make more sense?
> 
> (I will also update the ChangeLog to be more explicit about the relationship with relative positioning)

I think I understand your point but would like to see the updated ChangeLog with the long explanation.

>>> Source/WebCore/ChangeLog:39
>>> +
>> 
>> If this route pans out (which I am not convinced ATM), you would need to also touch RenderBox::computeRectForRepaint as it checks position before adding the offsetForInFlowPosition but wasn't amended to cover the new condition.
> 
> You're right, I did miss that. I'll update that. Do you have any other concerns that you did not mention here?

Nope.

>>> Source/WebCore/rendering/RenderObject.h:533
>>> +        positioned = positioned || (m_bitfields.floating() && m_bitfields.isBox() && style()->shapeOutside());
>> 
>> I don't think it's valid to call a floating box in-flow or in normal flow per CSS 2.1 (see http://www.w3.org/TR/CSS2/visuren.html#comparison, especially the part about floats).
> 
> Would you be happier if instead of changing isInFlowPositioned() and hasInFlowPosition(), I added new methods (these names are horrible, would have to spend more time thinking of good names) isInFlowPositionedOrFloatWithShapeOutside() and hasInFlowPositionedOrFloatWithShapeOutside() and changed the places that use the previous methods to use these methods?

If the new methods are not used by anyone (which I would expect), I would rather see a rename instead. The function names you offered are too long but I couldn't think of something as clear but shorter (didn't spend too much time on it though).

[Bem Jones-Bey]

(In reply to comment #9)
> (From update of attachment 184594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184594&action=review
> 
> >>> Source/WebCore/ChangeLog:13
> >>> +        implementation for relative positioning by using a RenderLayer. This
> >> 
> >> I have an issue with this explanation: relative positioning only shifts the content *at paint time*. shape-outside changes the layout so both concepts seems different in how they work to warrant more explanations.
> > 
> > When a shape outside is on a float, the float positioning fixes the top and left/right (depending on if it's floar: right or float: left) coordinates of the shape in the containing block for layout purposes. The only thing that changes (as far as layout is concerned) is the height and width due to the shape. This means that if you give an x and y position to the shape outside when applied to a float, the effect only takes effect at paint time and positions the float at -x and -y relative to the shape position. Does that make more sense?
> > 
> > (I will also update the ChangeLog to be more explicit about the relationship with relative positioning)
> 
> I think I understand your point but would like to see the updated ChangeLog with the long explanation.

Ok, I will post that today.

> 
> >>> Source/WebCore/rendering/RenderObject.h:533
> >>> +        positioned = positioned || (m_bitfields.floating() && m_bitfields.isBox() && style()->shapeOutside());
> >> 
> >> I don't think it's valid to call a floating box in-flow or in normal flow per CSS 2.1 (see http://www.w3.org/TR/CSS2/visuren.html#comparison, especially the part about floats).
> > 
> > Would you be happier if instead of changing isInFlowPositioned() and hasInFlowPosition(), I added new methods (these names are horrible, would have to spend more time thinking of good names) isInFlowPositionedOrFloatWithShapeOutside() and hasInFlowPositionedOrFloatWithShapeOutside() and changed the places that use the previous methods to use these methods?
> 
> If the new methods are not used by anyone (which I would expect), I would rather see a rename instead. The function names you offered are too long but I couldn't think of something as clear but shorter (didn't spend too much time on it though).

I don't understand why you would expect it not to be used by anyone. I would expect to be using them in all of the places where hasInFlowPosition and isInFlowPositioned are used right now. The other alternative would be to just add another if statement that checks for shapeOutside on a float in the places where hasInFlowPosition and isInFlowPositioned are currently checked for. Since we need to add the offset due to the shape in all of the same cases where we would have to adjust for relative positioning.

[Julien Chaffraix]

Comment on attachment 184594 [details]
Patch

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

>>>>> Source/WebCore/rendering/RenderObject.h:533
>>>>> +        positioned = positioned || (m_bitfields.floating() && m_bitfields.isBox() && style()->shapeOutside());
>>>> 
>>>> I don't think it's valid to call a floating box in-flow or in normal flow per CSS 2.1 (see http://www.w3.org/TR/CSS2/visuren.html#comparison, especially the part about floats).
>>> 
>>> Would you be happier if instead of changing isInFlowPositioned() and hasInFlowPosition(), I added new methods (these names are horrible, would have to spend more time thinking of good names) isInFlowPositionedOrFloatWithShapeOutside() and hasInFlowPositionedOrFloatWithShapeOutside() and changed the places that use the previous methods to use these methods?
>> 
>> If the new methods are not used by anyone (which I would expect), I would rather see a rename instead. The function names you offered are too long but I couldn't think of something as clear but shorter (didn't spend too much time on it though).
> 
> I don't understand why you would expect it not to be used by anyone. I would expect to be using them in all of the places where hasInFlowPosition and isInFlowPositioned are used right now. The other alternative would be to just add another if statement that checks for shapeOutside on a float in the places where hasInFlowPosition and isInFlowPositioned are currently checked for. Since we need to add the offset due to the shape in all of the same cases where we would have to adjust for relative positioning.

s/new methods/old methods/ in my earlier reply which should make a lot more sense.

[Bem Jones-Bey]

(In reply to comment #11)
> (From update of attachment 184594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184594&action=review
> 
> >>>>> Source/WebCore/rendering/RenderObject.h:533
> >>>>> +        positioned = positioned || (m_bitfields.floating() && m_bitfields.isBox() && style()->shapeOutside());
> >>>> 
> >>>> I don't think it's valid to call a floating box in-flow or in normal flow per CSS 2.1 (see http://www.w3.org/TR/CSS2/visuren.html#comparison, especially the part about floats).
> >>> 
> >>> Would you be happier if instead of changing isInFlowPositioned() and hasInFlowPosition(), I added new methods (these names are horrible, would have to spend more time thinking of good names) isInFlowPositionedOrFloatWithShapeOutside() and hasInFlowPositionedOrFloatWithShapeOutside() and changed the places that use the previous methods to use these methods?
> >> 
> >> If the new methods are not used by anyone (which I would expect), I would rather see a rename instead. The function names you offered are too long but I couldn't think of something as clear but shorter (didn't spend too much time on it though).
> > 
> > I don't understand why you would expect it not to be used by anyone. I would expect to be using them in all of the places where hasInFlowPosition and isInFlowPositioned are used right now. The other alternative would be to just add another if statement that checks for shapeOutside on a float in the places where hasInFlowPosition and isInFlowPositioned are currently checked for. Since we need to add the offset due to the shape in all of the same cases where we would have to adjust for relative positioning.
> 
> s/new methods/old methods/ in my earlier reply which should make a lot more sense.

Yes, that does make a lot more sense, thanks. I'll give some more thought to the naming as well.

[Bem Jones-Bey]

Created attachment 187189 [details]
Patch

Update based on feedback

[Julien Chaffraix]

Comment on attachment 187189 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:544
> +        ExclusionShapeOutsideInfo* shapeOutside = toRenderBox(this)->exclusionShapeOutsideInfo();
> +        if (shapeOutside)

This should be on one line.

> Source/WebCore/rendering/RenderLayer.h:1127
>      // Our current relative position offset.

Let's update this comment as it is stale now.

// Paint time offset only, it is used for properly paint relative / sticky positioned elements and exclusion boxes on floats.

> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-overhang.html:31
> +      You should only be able to see part of this text. If you can read all of this, then it isn't working.

I would capitalize the second sentence and make it more prominent that this is the failure:

You should only be able to see this sentence. <span style="color: red">IF YOU SEE THIS TEXT, THE TEST HAS FAILED.</span>

If you expect only to see part of the second sentence, this test should defined a fixed font-size (probably Ahem) and use a text that ensures the proper layout (like XXXXXXXXXX which is 100px if font-size: 10px)

[Bem Jones-Bey]

Created attachment 187632 [details]
Final Patch

Update based on feedback

[WebKit Review Bot]

Comment on attachment 187632 [details]
Final Patch

Rejecting attachment 187632 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'validate-changelog', '--non-interactive', 187632, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/16488471

[Michelangelo De Simone]

Created attachment 187665 [details]
Patch for landing

[Michelangelo De Simone]

Created attachment 187666 [details]
Patch for landing

[Michelangelo De Simone]

Created attachment 187668 [details]
Patch for landing

[Michelangelo De Simone]

Created attachment 187670 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 187670 [details]
Patch for landing

Clearing flags on attachment: 187670

Committed r142527: <http://trac.webkit.org/changeset/142527>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.