REGRESSION: flexbox content-size fails to exclude scrollbar

[Scott Miles]

Created attachment 155340 [details]
Reduction shows improper sizing of scrollable flexbox DIV 

The content-size of a DIV marked as display: -webkit-flex looks incorrect when the DIV has scrollbars. Reduction attached.

[Elliott Sprehn]

This is a regression. I tried this in my WebKit nightly and it looked fine at r123765, but then when I updated it was broken.

[Elliott Sprehn]

The only things that seem related are r123909 and r123783. Any ideas Tony?

[Tony Chang]

I regressed this in r123909.  Investigating.

[Tony Chang]

Created attachment 155593 [details]
Patch

[Tony Chang]

I decided to go with the name computeLogicalClientHeight instead of computePaddingBoxLogicalHeight since the padding box would include the scrollable area. I'm flexible on the name so let me know if you prefer something else.

[Elliott Sprehn]

Comment on attachment 155593 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.h:87
> +    LayoutUnit computeLogicalClientHeight(SizeType, const Length& height);

This seems like a generally applicable concept for all block things. Any reason to not put it on RenderBlock?

[Tony Chang]

Comment on attachment 155593 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.h:87
>> +    LayoutUnit computeLogicalClientHeight(SizeType, const Length& height);
> 
> This seems like a generally applicable concept for all block things. Any reason to not put it on RenderBlock?

It's needed in cases where you need to compute the height, but don't want to compute the final height (because you're not done layout out your children yet).  We need this for flexbox (and maybe for grid), but for regular RenderBlock, it wouldn't be useful.

The old flexbox code actually just computes the final height multiple times.

[Ojan Vafai]

Comment on attachment 155593 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:394
> +        return std::max(LayoutUnit(0), computeLogicalClientHeight(MainOrPreferredSize, style()->logicalHeight()) - scrollbarLogicalHeight());

Aren't we subtracting out the scrollbar height twice now?

[Tony Chang]

Created attachment 155660 [details]
Patch

[Tony Chang]

Comment on attachment 155660 [details]
Patch

(In reply to comment #8)
> (From update of attachment 155593 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155593&action=review
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:394
> > +        return std::max(LayoutUnit(0), computeLogicalClientHeight(MainOrPreferredSize, style()->logicalHeight()) - scrollbarLogicalHeight());
> 
> Aren't we subtracting out the scrollbar height twice now?

I'm dumb.  Fixed.

[WebKit Review Bot]

Comment on attachment 155660 [details]
Patch

Clearing flags on attachment: 155660

Committed r124278: <http://trac.webkit.org/changeset/124278>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.