Bug 112398 – Auto height column flexboxes with border and padding are too short

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Auto height column flexboxes with border and padding are too short

[Ojan Vafai]

Created attachment 193220 [details]
Patch

[Tony Chang]

Comment on attachment 193220 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:750
> +        LayoutUnit availableFreeSpace = mainAxisContentExtent(preferredMainAxisExtent + borderAndPaddingLogicalHeight() + scrollbarLogicalHeight()) - preferredMainAxisExtent;

This can't be correct for horizontal flexbox.

> LayoutTests/css3/flexbox/auto-height-column-with-border-and-padding.html:5
> +    <div class="flex-one-one-auto" style="min-height: 0" data-expected-height=50>

It looks like the min-height is needed, but it's not clear to me why it's needed.  Why does adding a min-heigth change the available free space?

[Ojan Vafai]

Comment on attachment 193220 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:750
>> +        LayoutUnit availableFreeSpace = mainAxisContentExtent(preferredMainAxisExtent + borderAndPaddingLogicalHeight() + scrollbarLogicalHeight()) - preferredMainAxisExtent;
> 
> This can't be correct for horizontal flexbox.

The passed in argument is only used for column flexboxes. I was going to put this extra logic directly in mainAxisContentExtent, but the other caller of that function takes LayoutUnit::max as it's argument and thus we'd overflow. When we enable saturated layout arithmetic, we can move this.

>> LayoutTests/css3/flexbox/auto-height-column-with-border-and-padding.html:5
>> +    <div class="flex-one-one-auto" style="min-height: 0" data-expected-height=50>
> 
> It looks like the min-height is needed, but it's not clear to me why it's needed.  Why does adding a min-heigth change the available free space?

It doesn't change the available free space. In the old code, the available free space would still be -20, and we'd flex-shrink the flex item to 0, but then adjustChildSizeForMinAndMax would expand the item out to its auto-size again.

[Tony Chang]

Comment on attachment 193220 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Test: css3/flexbox/auto-height-column-with-border-and-padding.html
> +
> +        * rendering/RenderFlexibleBox.cpp:
> +        (WebCore::RenderFlexibleBox::mainAxisContentExtent):
> +        (WebCore::RenderFlexibleBox::layoutFlexItems):

Please explain the cause of the bug and the fix in the ChangeLog.

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:750
>>> +        LayoutUnit availableFreeSpace = mainAxisContentExtent(preferredMainAxisExtent + borderAndPaddingLogicalHeight() + scrollbarLogicalHeight()) - preferredMainAxisExtent;
>> 
>> This can't be correct for horizontal flexbox.
> 
> The passed in argument is only used for column flexboxes. I was going to put this extra logic directly in mainAxisContentExtent, but the other caller of that function takes LayoutUnit::max as it's argument and thus we'd overflow. When we enable saturated layout arithmetic, we can move this.

I see, the bug is that we're passing the content height to computeLogicalHeight rather than the 'height'.

Let's put this in mainAxisContentExtent() and do something like:
  LayoutUnit logicalHeight = std::max(contentLogicalHeight, contentLogicalHeight + borderAndPaddingLogicalHeight() + scrollbarLogicalHeight());
to catch overflow.  We could add a FIXME about removing this when saturated layout is enabled.

Can you add a test with a overflow-x:scroll?

[Ojan Vafai]

Created attachment 193373 [details]
Patch

[Ojan Vafai]

Committed r145937: <http://trac.webkit.org/changeset/145937>