Bug 117885

Summary: [CSS Shapes] Fixed shape size on an auto height container should determine the containing block's height
Product: WebKit Reporter: Zoltan Horvath <zoltan>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED LATER    
Severity: Normal CC: achicu, commit-queue, esprehn+autocc, glenn, kondapallykalyan, stearns
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 89256    
Attachments:
Description Flags
proposed patch
hyatt: review-, buildbot: commit-queue-
incorporating review
none
updated patch hyatt: review-

Description Zoltan Horvath 2013-06-21 11:43:26 PDT
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.
Comment 1 Zoltan Horvath 2013-06-21 15:45:51 PDT
Created attachment 205223 [details]
proposed patch
Comment 2 Build Bot 2013-06-21 17:29:50 PDT
Comment on attachment 205223 [details]
proposed patch

Attachment 205223 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/964049
Comment 3 Alexandru Chiculita 2013-07-17 00:27:55 PDT
Can you please link to the specification for this behavior?
Comment 4 Alan Stearns 2013-07-18 13:42:54 PDT
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
Comment 5 Dave Hyatt 2013-07-19 10:20:28 PDT
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.
Comment 6 Zoltan Horvath 2013-07-23 16:23:30 PDT
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.
Comment 7 Alexandru Chiculita 2013-07-30 13:27:53 PDT
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.
Comment 8 Zoltan Horvath 2013-08-14 10:33:29 PDT
Created attachment 208741 [details]
updated patch
Comment 9 Zoltan Horvath 2013-08-14 10:40:15 PDT
(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.
Comment 10 Dave Hyatt 2013-08-28 13:46:25 PDT
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.