Bug 70754 - implement flex-align for flex-flow: column
: implement flex-align for flex-flow: column
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 71161
: 62048
  Show dependency treegraph
 
Reported: 2011-10-24 13:28 PST by
Modified: 2011-11-15 16:29 PST (History)


Attachments
testcase (206 bytes, text/html)
2011-10-26 16:40 PST, Ojan Vafai
no flags Details
Patch (11.59 KB, patch)
2011-10-28 19:49 PST, Ojan Vafai
no flags Review Patch | Details | Formatted Diff | Diff
Patch (11.63 KB, patch)
2011-10-29 14:16 PST, Ojan Vafai
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.21 KB, patch)
2011-10-30 13:45 PST, Ojan Vafai
hyatt: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-10-24 13:28:51 PST
flex-align landed before flex-flow: column, so there are probably some bugs here.  We also need tests.
------- Comment #1 From 2011-10-26 16:40:11 PST -------
*** Bug 70980 has been marked as a duplicate of this bug. ***
------- Comment #2 From 2011-10-26 16:40:25 PST -------
Created an attachment (id=112618) [details]
testcase
------- Comment #3 From 2011-10-28 19:49:42 PST -------
Created an attachment (id=112963) [details]
Patch
------- Comment #4 From 2011-10-28 19:50:39 PST -------
This patch needs on the patch in bug 71161 to pass the new tests.
------- Comment #5 From 2011-10-28 19:54:17 PST -------
(From update of attachment 112963 [details])
Attachment 112963 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10240240
------- Comment #6 From 2011-10-28 19:54:21 PST -------
(From update of attachment 112963 [details])
Attachment 112963 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10237433
------- Comment #7 From 2011-10-28 19:57:29 PST -------
(From update of attachment 112963 [details])
Attachment 112963 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10244120
------- Comment #8 From 2011-10-28 21:27:40 PST -------
(From update of attachment 112963 [details])
Attachment 112963 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10238440
------- Comment #9 From 2011-10-29 06:18:13 PST -------
(From update of attachment 112963 [details])
Attachment 112963 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10238503
------- Comment #10 From 2011-10-29 14:16:19 PST -------
Created an attachment (id=112977) [details]
Patch
------- Comment #11 From 2011-10-30 13:45:43 PST -------
Created an attachment (id=113001) [details]
Patch
------- Comment #12 From 2011-10-30 13:46:40 PST -------
*** Bug 70774 has been marked as a duplicate of this bug. ***
------- Comment #13 From 2011-10-31 10:18:54 PST -------
(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?

> 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.
------- Comment #14 From 2011-11-01 16:07:54 PST -------
(In reply to comment #13)
> (From update of attachment 113001 [details] [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.
------- Comment #15 From 2011-11-03 13:43:55 PST -------
(From update of attachment 113001 [details])
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.
------- Comment #16 From 2011-11-03 13:45:49 PST -------
(In reply to comment #13)
> (From update of attachment 113001 [details] [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.
------- Comment #17 From 2011-11-15 12:49:49 PST -------
(From update of attachment 113001 [details])
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.
------- Comment #18 From 2011-11-15 16:29:24 PST -------
Committed r100365: <http://trac.webkit.org/changeset/100365>