Regression(r143542): -webkit-align-items: center with overflow: auto/scroll has extra bottom padding

• Component: CSS
• Status: RESOLVED FIXED

[Timothy Hatcher]

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>

[Radar WebKit Bug Importer]

<rdar://problem/13392908>

[Simon Fraser (smfr)]

*** Bug 112048 has been marked as a duplicate of this bug. ***

[Tony Chang]

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?

[Tony Chang]

To clarify safe centering: You set the margin top and margin bottom to auto on the img (flex item).

[Tony Chang]

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.

[Ojan Vafai]

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.

[Timothy Hatcher]

I think it only makes sense for overflow: visible. I will try margin: auto in my original content for now. Thanks!

[Timothy Hatcher]

FWIW, the deprecated flexbox smartly centers in overflow: auto/scroll.

[Radar WebKit Bug Importer]

<rdar://problem/13393535>

[Ojan Vafai]

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.

[Timothy Hatcher]

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…

[Tony Chang]

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.

[Tony Chang]

Created attachment 192844 [details]
Patch

[Ojan Vafai]

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.

[WebKit Review Bot]

Comment on attachment 192844 [details]
Patch

Clearing flags on attachment: 192844

Committed r145736: <http://trac.webkit.org/changeset/145736>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

(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.