Bug 70754 - implement flex-align for flex-flow: column
: implement flex-align for flex-flow: column
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Ojan Vafai
:
Depends on: 71161
Blocks: 62048
  Show dependency treegraph
 
Reported: 2011-10-24 13:28 PDT by Tony Chang
Modified: 2011-11-15 16:29 PST (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2011-10-24 13:28:51 PDT
flex-align landed before flex-flow: column, so there are probably some bugs here.  We also need tests.
Comment 1 Ojan Vafai 2011-10-26 16:40:11 PDT
*** Bug 70980 has been marked as a duplicate of this bug. ***
Comment 2 Ojan Vafai 2011-10-26 16:40:25 PDT
Created attachment 112618 [details]
testcase
Comment 3 Ojan Vafai 2011-10-28 19:49:42 PDT
Created attachment 112963 [details]
Patch
Comment 4 Ojan Vafai 2011-10-28 19:50:39 PDT
This patch needs on the patch in bug 71161 to pass the new tests.
Comment 5 WebKit Review Bot 2011-10-28 19:54:17 PDT
Comment on attachment 112963 [details]
Patch

Attachment 112963 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10240240
Comment 6 Gyuyoung Kim 2011-10-28 19:54:21 PDT
Comment on attachment 112963 [details]
Patch

Attachment 112963 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10237433
Comment 7 Early Warning System Bot 2011-10-28 19:57:29 PDT
Comment on attachment 112963 [details]
Patch

Attachment 112963 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10244120
Comment 8 Gustavo Noronha (kov) 2011-10-28 21:27:40 PDT
Comment on attachment 112963 [details]
Patch

Attachment 112963 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10238440
Comment 9 Daniel Bates 2011-10-29 06:18:13 PDT
Comment on attachment 112963 [details]
Patch

Attachment 112963 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10238503
Comment 10 Ojan Vafai 2011-10-29 14:16:19 PDT
Created attachment 112977 [details]
Patch
Comment 11 Ojan Vafai 2011-10-30 13:45:43 PDT
Created attachment 113001 [details]
Patch
Comment 12 Ojan Vafai 2011-10-30 13:46:40 PDT
*** Bug 70774 has been marked as a duplicate of this bug. ***
Comment 13 Tony Chang 2011-10-31 10:18:54 PDT
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.
Comment 14 Tony Chang 2011-11-01 16:07:54 PDT
(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.
Comment 15 Ojan Vafai 2011-11-03 13:43:55 PDT
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.
Comment 16 Ojan Vafai 2011-11-03 13:45:49 PDT
(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.
Comment 17 Dave Hyatt 2011-11-15 12:49:49 PST
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.
Comment 18 Ojan Vafai 2011-11-15 16:29:24 PST
Committed r100365: <http://trac.webkit.org/changeset/100365>