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: | CSS | Assignee: | 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
Zoltan Horvath
2013-06-21 11:43:26 PDT
Created attachment 205223 [details]
proposed patch
Comment on attachment 205223 [details] proposed patch Attachment 205223 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/964049 Can you please link to the specification for this behavior? 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 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. 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 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. Created attachment 208741 [details]
updated patch
(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 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. |