Bug 46496 – calcReplacedWidthUsing has wrong containing block width.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

calcReplacedWidthUsing has wrong containing block width.  The width used has to match the block-flow directionality of the replaced element and not of the containing block.

[Yuki Sekiguchi]

Created attachment 223081 [details]
Patch

[Darin Adler]

Dave, would you review+ or review- please?

[David Kilzer (:ddkilzer)]

Comment on attachment 223081 [details]
Patch

Clearing review? flag on old patch that no longer applies to trunk.

[Rob Buis]

Created attachment 441486 [details]
Patch

[Rob Buis]

Created attachment 441503 [details]
Patch

[Rob Buis]

Created attachment 441735 [details]
Patch

[Darin Adler]

Comment on attachment 441735 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3334
> +        LayoutUnit containerWidth;
> +        if (isOutOfFlowPositioned())
> +            containerWidth = containingBlockLogicalWidthForPositioned(downcast<RenderBoxModelObject>(*container()));
> +        else if (isHorizontalWritingMode() == containingBlock()->isHorizontalWritingMode())
> +            containerWidth = containingBlockLogicalWidthForContent();
> +        else
> +            containerWidth = perpendicularContainingBlockLogicalHeight();

I’ve been noticing something like this, perhaps if not exactly the same, as a repeated idiom in patches recently correcting orthogonal flow behavior. Is there a way to add another function to help us get the repeated pattern right, without the code having an if statement like this in the middle or hurting efficiency? Or maybe it’s not repeated often enough.

[Rob Buis]

Comment on attachment 441735 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:3334
>> +            containerWidth = perpendicularContainingBlockLogicalHeight();
> 
> I’ve been noticing something like this, perhaps if not exactly the same, as a repeated idiom in patches recently correcting orthogonal flow behavior. Is there a way to add another function to help us get the repeated pattern right, without the code having an if statement like this in the middle or hurting efficiency? Or maybe it’s not repeated often enough.

I checked but I do not see it repeated enough. I do think we can provide something for the frequent "isHorizontalWritingMode() == containingBlock()->isHorizontalWritingMode()" like checks, but that sounds like a different bug.

[zalan]

Comment on attachment 441735 [details]
Patch

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

>>> Source/WebCore/rendering/RenderBox.cpp:3334
>>> +            containerWidth = perpendicularContainingBlockLogicalHeight();
>> 
>> I’ve been noticing something like this, perhaps if not exactly the same, as a repeated idiom in patches recently correcting orthogonal flow behavior. Is there a way to add another function to help us get the repeated pattern right, without the code having an if statement like this in the middle or hurting efficiency? Or maybe it’s not repeated often enough.
> 
> I checked but I do not see it repeated enough. I do think we can provide something for the frequent "isHorizontalWritingMode() == containingBlock()->isHorizontalWritingMode()" like checks, but that sounds like a different bug.

I always get confused when I see code like this sine the writing-direction defines (at least the part that's relevant here) the block direction for the content. 
Now the block level child box's writing direction (replaced box in this case) should have nothing to do with how we get the available width constraint from the containing block. It's only the containing block's writing direction that rules the logical width/height for its content (again, replaced box in this case). 
When the containing block has horizontal writing mode, the available width for its block level boxes matches its "physical" width, while with vertical direction the available width becomes its "physical" height so that the box level child boxes grow vertically with their widths. i.e. when you call containingBlockLogicalWidthForContent() it shouldn't matter whether _this_ box is horizontal or vertical in the context of finding the available width for the child box. I know this code copies the logic from computeReplacedLogicalHeightUsing, but we either do yet another flip somewhere to have correct final values or simply the functions/variable names don't mean logical values.

[zalan]

or put it simply:
when the child box ask “how much I can grow (logical)horizontally in this containing block”, the child box's writing direction is irrelevant.

[Sergio Villar Senin]

Comment on attachment 441735 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderBox.cpp:3334
>>>> +            containerWidth = perpendicularContainingBlockLogicalHeight();
>>> 
>>> I’ve been noticing something like this, perhaps if not exactly the same, as a repeated idiom in patches recently correcting orthogonal flow behavior. Is there a way to add another function to help us get the repeated pattern right, without the code having an if statement like this in the middle or hurting efficiency? Or maybe it’s not repeated often enough.
>> 
>> I checked but I do not see it repeated enough. I do think we can provide something for the frequent "isHorizontalWritingMode() == containingBlock()->isHorizontalWritingMode()" like checks, but that sounds like a different bug.
> 
> I always get confused when I see code like this sine the writing-direction defines (at least the part that's relevant here) the block direction for the content. 
> Now the block level child box's writing direction (replaced box in this case) should have nothing to do with how we get the available width constraint from the containing block. It's only the containing block's writing direction that rules the logical width/height for its content (again, replaced box in this case). 
> When the containing block has horizontal writing mode, the available width for its block level boxes matches its "physical" width, while with vertical direction the available width becomes its "physical" height so that the box level child boxes grow vertically with their widths. i.e. when you call containingBlockLogicalWidthForContent() it shouldn't matter whether _this_ box is horizontal or vertical in the context of finding the available width for the child box. I know this code copies the logic from computeReplacedLogicalHeightUsing, but we either do yet another flip somewhere to have correct final values or simply the functions/variable names don't mean logical values.

I agree it's confusing but I don't think there is a problem with the code. In this case we're interested in the size of the container in the inline size of the child. If both are parallel then we need the inline size of the container, otherwise we are looking for the block size of the container. So we do need to check whether or not they're orthogonal. The question is I guess whether we do it in the caller does or the callee.

BTW orthogonal to this bug, should we push for using Inline/Block instead of LogicalWidth/LogicalHeight ?

[EWS]

Committed r284548 (243290@main): <https://commits.webkit.org/243290@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441735 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84468824>