Bug 92667

Summary: REGRESSION: flexbox content-size fails to exclude scrollbar
Product: WebKit Reporter: Scott Miles <sjmiles>
Component: CSSAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, esprehn, ojan, sjmiles, tony, webkit.review.bot
Priority: P2 Keywords: HasReduction, Regression
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.7   
Attachments:
Description Flags
Reduction shows improper sizing of scrollable flexbox DIV
none
Patch
none
Patch none

Scott Miles
Reported 2012-07-30 12:38:09 PDT
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.
Attachments
Reduction shows improper sizing of scrollable flexbox DIV (885 bytes, text/html)
2012-07-30 12:38 PDT, Scott Miles 		no flags
Details
Patch (8.88 KB, patch)
2012-07-31 11:35 PDT, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch (8.86 KB, patch)
2012-07-31 16:05 PDT, Tony Chang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Elliott Sprehn
Comment 1 2012-07-30 12:43:25 PDT
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
Comment 2 2012-07-30 12:49:04 PDT
The only things that seem related are r123909 and r123783. Any ideas Tony?
Tony Chang
Comment 3 2012-07-30 14:13:35 PDT
I regressed this in r123909. Investigating.
Tony Chang
Comment 4 2012-07-31 11:35:26 PDT
Created attachment 155593 [details] Patch
Tony Chang
Comment 5 2012-07-31 11:36:11 PDT
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 6 2012-07-31 11:50:44 PDT
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 7 2012-07-31 11:57:03 PDT
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 8 2012-07-31 15:54:11 PDT
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
Comment 9 2012-07-31 16:05:25 PDT
Created attachment 155660 [details] Patch
Tony Chang
Comment 10 2012-07-31 16:05:52 PDT
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 11 2012-07-31 17:54:35 PDT
Comment on attachment 155660 [details] Patch Clearing flags on attachment: 155660 Committed r124278: <http://trac.webkit.org/changeset/124278>
WebKit Review Bot
Comment 12 2012-07-31 17:54:39 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.