Created attachment 110947 [details] Patch

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.

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.

Created attachment 111097 [details] Patch

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?

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