WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
112398
Auto height column flexboxes with border and padding are too short
https://bugs.webkit.org/show_bug.cgi?id=112398
Summary
Auto height column flexboxes with border and padding are too short
Ojan Vafai
Reported
2013-03-14 19:15:13 PDT
Auto height column flexboxes with border and padding are too short
Attachments
Patch
(5.27 KB, patch)
2013-03-14 19:15 PDT
,
Ojan Vafai
no flags
Details
Formatted Diff
Diff
Patch
(5.16 KB, patch)
2013-03-15 14:09 PDT
,
Ojan Vafai
tony
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Ojan Vafai
Comment 1
2013-03-14 19:15:49 PDT
Created
attachment 193220
[details]
Patch
Tony Chang
Comment 2
2013-03-15 10:01:39 PDT
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 3
2013-03-15 10:44:16 PDT
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 4
2013-03-15 11:10:09 PDT
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
Comment 5
2013-03-15 14:09:17 PDT
Created
attachment 193373
[details]
Patch
Ojan Vafai
Comment 6
2013-03-15 14:16:00 PDT
Committed
r145937
: <
http://trac.webkit.org/changeset/145937
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug