Bug 70082 – implement flex-flow:column

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

implement flex-flow:column

[Ojan Vafai]

Created attachment 110947 [details]
Patch

[Tony Chang]

Comment on attachment 110947 [details]
Patch

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

> LayoutTests/css3/flexbox/flex-flow-border-expected.txt:35
> +FAIL:
> +Expected 160 for width, but got 140. 

Can you mention in the ChangeLog why some tests fail?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:178
> +    // FIXME: We should only need to call one of these for a given flex-flow + writing-modeb combination.

Nit: typo writing-modeb

> Source/WebCore/rendering/RenderFlexibleBox.cpp:227
> +    isHorizontalFlow() ? setHeight(size) : setWidth(size);

Using a ternary without a return value is weird.  I would just make this an if/else statement.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:675
> -        startEdge += childLogicalWidth + marginEndForChild(child);
> +        startEdge += childLogicalWidth + flowAwareMarginEndForChild(child);

Heh, I just made this fix too.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:695
> +        // direction:rtl + flex-flow:column means the cross-axis direction is flipped.
> +        if (!style()->isLeftToRightDirection() && isColumnFlow()) {

I'm not sure I understand this.  I guess we can fix when adding more flex-flow:column + flex-align tests.

[Ojan Vafai]

Comment on attachment 110947 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:695
>> +        if (!style()->isLeftToRightDirection() && isColumnFlow()) {
> 
> I'm not sure I understand this.  I guess we can fix when adding more flex-flow:column + flex-align tests.

We need to do this regardless of flex-align since direction flips the cross-axis direction when flex-flow is column. I'm sure the code in the switch below doesn't handle this case correctly, but in the interests of keeping this patch reasonably sized, I just put the FIXME below.

I think the only thing we need to do is swap the meanings of AlignStart and AlignEnd, but I'm not sure about AlignBaseline. Now that I think about it, we might want to do this after the switch statement since AlignStretch might affect the child's height. In either case, I think the FIXME below is sufficient until we add tests and make flex-align + flex-flow:column + direction:rtl play nicely.

[Ojan Vafai]

Created attachment 111097 [details]
Patch

[Dave Hyatt]

Comment on attachment 111097 [details]
Patch

I'll r+, but I'm confused as to who sets the logical height of a flexbox. Normal layout algorithm is:

(1) Compute your logical width
(2) Reset your height to just your borderBefore+paddingBefore
(3) Places your kids, increasing your logical height as you go
(4) When done, you have an intrinsic logical height
(5) Now call computeLogicalHeight to see if you have to shrink

Maybe flexbox will be different, but the above is a bit more like how blocks and tables work at least. I'm guessing flexboxes with unspecified heights just aren't working at the moment?

[Ojan Vafai]

Committed r97783: <http://trac.webkit.org/changeset/97783>