|Summary:||Regression(r143542): -webkit-align-items: center with overflow: auto/scroll has extra bottom padding|
|Product:||WebKit||Reporter:||Timothy Hatcher <timothy>|
|Component:||CSS||Assignee:||Tony Chang <tony>|
|Severity:||Normal||CC:||eric, esprehn+autocc, esprehn, ojan.autocc, ojan, simon.fraser, tabatkins, timothy, tony, webkit-bug-importer, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Timothy Hatcher 2013-03-11 11:59:31 PDT
Created attachment 192522 [details] Testcase See the attached test case and scroll down. See the empty space being allotted for the image but not being used? The image should not extend off the top into the area that can't be seen. The behavior as-is really only makes sense with overflow: visible. Is this a problem with WebKit or a problem with the spec? <rdar://problem/13392441>
Comment 2 Simon Fraser (smfr) 2013-03-11 12:03:45 PDT
*** Bug 112048 has been marked as a duplicate of this bug. ***
Comment 3 Tony Chang 2013-03-11 12:15:48 PDT
This is how the spec is defined. center is supposed to distribute space equally between the top and the bottom, so we overflow off the top. If you want the more common behavior of "safe" centering (if there's overflow, only overflow in the scroll direction only), you can set the margin top and margin bottom to auto. It's not clear to me if that works for what you're trying to do here since that doesn't produce a scroll bar. What behavior do you want?
Comment 4 Tony Chang 2013-03-11 12:16:35 PDT
To clarify safe centering: You set the margin top and margin bottom to auto on the img (flex item).
Comment 5 Tony Chang 2013-03-11 12:23:22 PDT
Also, it looks like on Chrome 26, there's no extra space below the image, but in Chrome 27 and ToT there is. That seems like a regression.
Comment 6 Ojan Vafai 2013-03-11 12:25:01 PDT
Even in Chrome 26, we do true centering, which is kind of weird when overflow is not visible. I think we should propose changing the spec to only do true centering for overflow:visible.
Comment 7 Timothy Hatcher 2013-03-11 12:39:26 PDT
I think it only makes sense for overflow: visible. I will try margin: auto in my original content for now. Thanks!
Comment 8 Timothy Hatcher 2013-03-11 13:03:11 PDT
FWIW, the deprecated flexbox smartly centers in overflow: auto/scroll.
Comment 10 Ojan Vafai 2013-03-11 13:09:12 PDT
Can you give a use-case for using center alignment and overflow:scroll/auto? A use-case would be helpful for making an argument on www-style.
Comment 11 Timothy Hatcher 2013-03-11 13:25:15 PDT
The use case is to just center an image in the container when it is small. When it is large overflow where you can still see the whole image by scrolling down. Using margin: auto does work, so I'm fine with that. If the spec isn't changes then -webkit-align-items: center causing dead whitespace should be fixed. Right now it is weird you can only see the whole image if you use flex-start or stretch. The other keywords can hide content off the top and still allow you to scroll to see content at the end. Thinking about it logically, I think flex-end should set the scroll position and force you to scroll up to see the overflow. Same could be said for center, force the scroll position to be centered and you can scroll up or down in the overflow. The initial visual display is just a clipped version of the overflow: visible result. Does that make sense? I'm not sure if that would fly with the spec editors though…
Comment 12 Tony Chang 2013-03-11 13:48:30 PDT
I'm going to focus this bug on fixing the extra whitespace being allotted for the image. We can file a new bug if the spec gets updated and we want to special case centering based on overflow.
Comment 14 Ojan Vafai 2013-03-13 12:19:33 PDT
Comment on attachment 192844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192844&action=review > Source/WebCore/rendering/RenderFlexibleBox.cpp:442 > + if (child->isOutOfFlowPositioned()) > + continue; Maybe add a test-case with out of flow positioned children? Or does one of the existing test cases already cover this? > Source/WebCore/rendering/RenderFlexibleBox.cpp:444 > + LayoutUnit childLogicalBottom = logicalTopForChild(child) + logicalHeightForChild(child) + marginAfterForChild(child); > + maxChildLogicalBottom = std::max(maxChildLogicalBottom, childLogicalBottom); Technically, we only need to do this if the child is aligned center or flex-end, but I suppose it's not worth the extra code.
Comment 15 WebKit Review Bot 2013-03-13 12:31:14 PDT
Comment on attachment 192844 [details] Patch Clearing flags on attachment: 192844 Committed r145736: <http://trac.webkit.org/changeset/145736>
Comment 16 WebKit Review Bot 2013-03-13 12:31:18 PDT
All reviewed patches have been landed. Closing bug.
Comment 17 Tony Chang 2013-03-13 15:06:19 PDT
(In reply to comment #14) > (From update of attachment 192844 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192844&action=review > > > Source/WebCore/rendering/RenderFlexibleBox.cpp:442 > > + if (child->isOutOfFlowPositioned()) > > + continue; > > Maybe add a test-case with out of flow positioned children? Or does one of the existing test cases already cover this? Sure, https://bugs.webkit.org/show_bug.cgi?id=112294 has a test case for this.