Bug 70754 – implement flex-align for flex-flow: column

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Tony Chang]

flex-align landed before flex-flow: column, so there are probably some bugs here.  We also need tests.

[Ojan Vafai]

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

[Ojan Vafai]

Created attachment 112618 [details]
testcase

[Ojan Vafai]

Created attachment 112963 [details]
Patch

[Ojan Vafai]

This patch needs on the patch in bug 71161 to pass the new tests.

[WebKit Review Bot]

Comment on attachment 112963 [details]
Patch

Attachment 112963 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10240240

[Gyuyoung Kim]

Comment on attachment 112963 [details]
Patch

Attachment 112963 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10237433

[Early Warning System Bot]

Comment on attachment 112963 [details]
Patch

Attachment 112963 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10244120

[Gustavo Noronha (kov)]

Comment on attachment 112963 [details]
Patch

Attachment 112963 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10238440

[Daniel Bates]

Comment on attachment 112963 [details]
Patch

Attachment 112963 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10238503

[Ojan Vafai]

Created attachment 112977 [details]
Patch

[Ojan Vafai]

Created attachment 113001 [details]
Patch

[Ojan Vafai]

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

[Tony Chang]

Comment on attachment 113001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113001&action=review

> Source/WebCore/ChangeLog:13
> +        * rendering/RenderBox.cpp:
> +        (WebCore::RenderBox::sizesToIntrinsicLogicalWidth):
> +        When flexitems are column, they should size to the intrinsic width unless flex-flow is stretch.

Nit: Maybe explain why this is different from the old flexbox code.  Maybe we can get rid of the boxOrient and boxAlign checks on the old flexbox code?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:733
>                  else

If we change the width, do we have to relayout the flexitem?  Maybe not since it's only growing beyond the intrinsic size.

[Tony Chang]

(In reply to comment #13)
> (From update of attachment 113001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113001&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        * rendering/RenderBox.cpp:
> > +        (WebCore::RenderBox::sizesToIntrinsicLogicalWidth):
> > +        When flexitems are column, they should size to the intrinsic width unless flex-flow is stretch.
> 
> Nit: Maybe explain why this is different from the old flexbox code.  Maybe we can get rid of the boxOrient and boxAlign checks on the old flexbox code?

I tried removing the boxOrient and boxAlign checks in the old flexbox codepath and it broke a bunch of tests. I think the old spec didn't require everything to be shrinkwrapped.

[Ojan Vafai]

Comment on attachment 113001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113001&action=review

>>> Source/WebCore/ChangeLog:13
>>> +        When flexitems are column, they should size to the intrinsic width unless flex-flow is stretch.
>> 
>> Nit: Maybe explain why this is different from the old flexbox code.  Maybe we can get rid of the boxOrient and boxAlign checks on the old flexbox code?
> 
> I tried removing the boxOrient and boxAlign checks in the old flexbox codepath and it broke a bunch of tests. I think the old spec didn't require everything to be shrinkwrapped.

Yeah, I don't actually know the old code well enough to know why it doesn't require everything to be shrinkwrapped.

[Ojan Vafai]

(In reply to comment #13)
> (From update of attachment 113001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113001&action=review
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:733
> >                  else
> 
> If we change the width, do we have to relayout the flexitem?  Maybe not since it's only growing beyond the intrinsic size.

As discussed in person, there seem to definitely be cases where we need to do another layout:
-when changing the width
-if there are positioned items positioned wrt the bottom/right of the element

Possibly others. Definitely a separate patch. Maybe this calls for making the default value for flex-align not being stretch.

[Dave Hyatt]

Comment on attachment 113001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113001&action=review

r=me

> Source/WebCore/rendering/RenderBox.cpp:1811
> +#if ENABLE(CSS3_FLEXBOX)
> +    if (parent()->isFlexibleBox())
> +        return true;
> +#endif

Put a FIXME in here that the stretching children of vertical boxes will end up doing an extra layout with this approach.

[Ojan Vafai]

Committed r100365: <http://trac.webkit.org/changeset/100365>