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
Patch (5.16 KB, patch)
2013-03-15 14:09 PDT, Ojan Vafai
tony: review+
Ojan Vafai
Comment 1 2013-03-14 19:15:49 PDT
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
Ojan Vafai
Comment 6 2013-03-15 14:16:00 PDT
Note You need to log in before you can comment on or make changes to this bug.