Bug 117885 – [CSS Shapes] Fixed shape size on an auto height container should determine the containing block's height

• Component: CSS
• Status: RESOLVED LATER

[Zoltan Horvath]

When we have an autoheight container and there is a fixed size shape is applied on the container, the container should resolve its height to the height of the shape's bounding box.

[Zoltan Horvath]

Created attachment 205223 [details]
proposed patch

[Build Bot]

Comment on attachment 205223 [details]
proposed patch

Attachment 205223 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/964049

[Alexandru Chiculita]

Can you please link to the specification for this behavior?

[Alan Stearns]

I added a sentence to shapes level 2 for this - it will need to be more precisely defined.

http://dev.w3.org/csswg/css-shapes-2/#shape-inside-property

[Dave Hyatt]

Comment on attachment 205223 [details]
proposed patch

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

r-

> Source/WebCore/rendering/RenderBox.cpp:2545
> +        // If we have an auto sized block with a fixed size shape, the height should be adjusted to the height of the shape
> +        if (isRenderBlock() && h.isAuto()) {
> +            ShapeInsideInfo* shapeInsideInfo = toRenderBlock(this)->shapeInsideInfo();
> +            if (shapeInsideInfo && shapeInsideInfo->shapeLogicalBottom())
> +                heightResult = shapeInsideInfo->shapeLogicalBottom() + paddingBefore() + paddingAfter();
> +        }

This is misplaced. computeLogicalHeight is not about modifying intrinsic heights. I think you should do this in RenderBlock's layout method, since it is similar to the expansion we do for floats. I think it should go there instead of in RenderBox, where it's clearly misplaced.

[Zoltan Horvath]

Created attachment 207358 [details]
incorporating review

(In reply to comment #5)
> This is misplaced. computeLogicalHeight is not about modifying intrinsic heights. I think you should do this in RenderBlock's layout method, since it is similar to the expansion we do for floats. I think it should go there instead of in RenderBox, where it's clearly misplaced.

Fixed.

[Alexandru Chiculita]

Comment on attachment 207358 [details]
incorporating review

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

> Source/WebCore/rendering/RenderBlock.cpp:1681
> +            setLogicalHeight(shapeInsideInfo->shapeLogicalBottom() + paddingBefore() + paddingAfter());

Looks like we are going to overwrite max-height in here. What's the right behavior? We might need to do it before the updatelogicalheight call.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1670
> +    LayoutUnit shapeContainingBlockHeight = shapeInsideInfo->owner()->style()->height().isAuto() ? shapeLogicalBottom : shapeInsideInfo->shapeContainingBlockHeight();

Looks like shapeinfo is already doing something very similar. Why isn't ShapeInfo::m_shapeLogicalHeight the right thing here? Maybe we need to fix that first.

[Zoltan Horvath]

Created attachment 208741 [details]
updated patch

[Zoltan Horvath]

(In reply to comment #7)
> Looks like we are going to overwrite max-height in here. What's the right behavior? We might need to do it before the updatelogicalheight call.

Fixed. I added a test also.

> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1670
> > +    LayoutUnit shapeContainingBlockHeight = shapeInsideInfo->owner()->style()->height().isAuto() ? shapeLogicalBottom : shapeInsideInfo->shapeContainingBlockHeight();
> 
> Looks like shapeinfo is already doing something very similar. Why isn't ShapeInfo::m_shapeLogicalHeight the right thing here? Maybe we need to fix that first.

Fixed.

[Dave Hyatt]

Comment on attachment 208741 [details]
updated patch

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

r-

> Source/WebCore/rendering/RenderBlock.cpp:1491
> +    ShapeInsideInfo* shapeInsideInfo = this->shapeInsideInfo();
> +    // We don't modify the logicalHeight when the container isn't auto-height or the shape's height isn't resolved.
> +    if (!style()->height().isAuto() || !shapeInsideInfo || !(shapeInsideInfo->shapeLogicalBottom() - borderAndPaddingBefore()))
> +        return;
> +    // We don't modify the logicalHeight when we the max-height is defined and it's smaller than the shape's containingBlock height
> +    if (style()->logicalMaxHeight().isPositive() && style()->logicalMaxHeight().intValue() <= logicalHeight())
> +        return;
> +    setLogicalHeight(shapeInsideInfo->shapeLogicalBottom() + borderAndPaddingAfter());

It seems like you're making this too complicated. Remember that prior to computeLogicalHeight(), we really are just computing the block's intrinsic height. Then it gets adjusted.

In other words, I think you should just always increase the block and stop checking for !style()->height().isAuto() and for max height. These checks are wrong anyway, because of percentages that can end up being auto, etc.

You should be able to completely remove the style()->height() and logicalMaxHeight() checks and all the tests should still pass. You should probably also add percentage height and max-height tests, both where the percentage is honored (by being inside a container whose size is fixed) and where it is ignored (by being inside a container with no specified size).

Also, that style()->height() (if we do end up keeping it) should be a logicalHeight(), which means i'd like some writing-mode vertical-rl tests too.