Bug 69808 – Implement -webkit-flex-align for cross axis alignment in flex-flow: row

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Chang]

Implement -webkit-flex-align for cross axis alignment

[Tony Chang]

Created attachment 110454 [details]
Patch

[Ojan Vafai]

Comment on attachment 110454 [details]
Patch

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

A first pass review. I'd like hyatt to do the final r+ here since I'm unsure about the baseline related stuff.

> LayoutTests/css3/flexbox/flex-align.html:105
> +  <div data-expected-height="100" data-offset-y="0" style="width: -webkit-flex(1 0 0); height: 100px;"></div>

Would be good to have a test-case for items with different top/bottom margins. Specifically, I'd like to see flex-align:stretch and flex-align:baseline with something like the following:

<div class=flexbox>
  <div style="width: -webkit-flex(1); -webkit-flex-align: stretch; height: 50px; margin: 10px 20px 30px 40px"></div>
  <div style="width: -webkit-flex(1); -webkit-flex-align: stretch; height: 50px; margin: 2px 4px 6px 8px"></div>
</div>

> Source/WebCore/rendering/RenderFlexibleBox.cpp:512
> +            if (isFlowAwareLogicalHeightAuto())
> +                setFlowAwareLogicalHeight(std::max(flowAwareLogicalHeight(), flowAwareBorderBefore() + flowAwarePaddingBefore() + flowAwareMarginBeforeForChild(child) + maxAscent + maxDescent + flowAwareMarginAfterForChild(child) + flowAwarePaddingAfter() + flowAwareBorderAfter() + scrollbarLogicalHeight()));
> +        } else if (isFlowAwareLogicalHeightAuto())
> +            setFlowAwareLogicalHeight(std::max(flowAwareLogicalHeight(), flowAwareBorderBefore() + flowAwarePaddingBefore() + flowAwareMarginBeforeForChild(child) + flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child) + flowAwarePaddingAfter() + flowAwareBorderAfter() + scrollbarLogicalHeight()));

Can you instead move flowAwareBorderAndPaddingHeight and flowAwareMarginHeightForChild into helper functions? That would make this a lot easier to read and avoid code duplication here and above.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:540
> +void RenderFlexibleBox::alignChildrenBlockDirection(FlexOrderIterator& iterator, LayoutUnit maxAscent, LayoutUnit maxDescent)

You don't use maxDescent. Did you not get a compiler warning?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:544
> +        LayoutUnit currentChildHeight = flowAwareMarginBeforeForChild(child) + flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child);

You compute this on line 503 as well. Move into a helper function? flowAwareLogicalMarginBoxHeightForChild? lol.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:555
> +        case AlignStart:
> +            break;

Could you check the flexAlign as the beginning of the function and early return? Then here you could ASSERT_NOT_REACHED? I guess that's a premature optimization, but a FIXME at least would be good.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:566
> +            LayoutUnit ascent = child->firstLineBoxBaseline();
> +            if (ascent == -1)
> +                ascent = flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child);
> +            ascent += flowAwareMarginBeforeForChild(child);

This chunk of code is exactly the same as in layoutAndPlaceChildrenInlineDirection. Pull it out into a helper function?

[Tony Chang]

Created attachment 110574 [details]
Patch

[Tony Chang]

Created attachment 110575 [details]
Patch

[Tony Chang]

(In reply to comment #2)
> (From update of attachment 110454 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110454&action=review
> 
> > LayoutTests/css3/flexbox/flex-align.html:105
> > +  <div data-expected-height="100" data-offset-y="0" style="width: -webkit-flex(1 0 0); height: 100px;"></div>
> 
> Would be good to have a test-case for items with different top/bottom margins.

Done.

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:512
> > +            if (isFlowAwareLogicalHeightAuto())
> > +                setFlowAwareLogicalHeight(std::max(flowAwareLogicalHeight(), flowAwareBorderBefore() + flowAwarePaddingBefore() + flowAwareMarginBeforeForChild(child) + maxAscent + maxDescent + flowAwareMarginAfterForChild(child) + flowAwarePaddingAfter() + flowAwareBorderAfter() + scrollbarLogicalHeight()));
> > +        } else if (isFlowAwareLogicalHeightAuto())
> > +            setFlowAwareLogicalHeight(std::max(flowAwareLogicalHeight(), flowAwareBorderBefore() + flowAwarePaddingBefore() + flowAwareMarginBeforeForChild(child) + flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child) + flowAwarePaddingAfter() + flowAwareBorderAfter() + scrollbarLogicalHeight()));
> 
> Can you instead move flowAwareBorderAndPaddingHeight and flowAwareMarginHeightForChild into helper functions? That would make this a lot easier to read and avoid code duplication here and above.

Done.

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:540
> > +void RenderFlexibleBox::alignChildrenBlockDirection(FlexOrderIterator& iterator, LayoutUnit maxAscent, LayoutUnit maxDescent)
> 
> You don't use maxDescent. Did you not get a compiler warning?

Removed.

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:544
> > +        LayoutUnit currentChildHeight = flowAwareMarginBeforeForChild(child) + flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child);
> 
> You compute this on line 503 as well. Move into a helper function? flowAwareLogicalMarginBoxHeightForChild? lol.

Switched to use flowAwareMarginLogicalHeightForChild().

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:555
> > +        case AlignStart:
> > +            break;
> 
> Could you check the flexAlign as the beginning of the function and early return? Then here you could ASSERT_NOT_REACHED? I guess that's a premature optimization, but a FIXME at least would be good.

I added availableLogicalHeightForChild to avoid the extra computation.  An early return wouldn't have worked since this is applied to each child.

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:566
> > +            LayoutUnit ascent = child->firstLineBoxBaseline();
> > +            if (ascent == -1)
> > +                ascent = flowAwareLogicalHeightForChild(child) + flowAwareMarginAfterForChild(child);
> > +            ascent += flowAwareMarginBeforeForChild(child);
> 
> This chunk of code is exactly the same as in layoutAndPlaceChildrenInlineDirection. Pull it out into a helper function?

Done.

[Ojan Vafai]

Comment on attachment 110575 [details]
Patch

Patch looks good to me. Would like Hyatt to do a pass just to make sure we're not doing anything egregiously wrong.

[Dave Hyatt]

Comment on attachment 110575 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:571
> +        case AlignStretch: {
> +            Length height = isHorizontalFlow() ? child->style()->height() : child->style()->width();
> +            if (height.isAuto())
> +                child->setLogicalHeight(child->logicalHeight() + RenderFlexibleBox::availableLogicalHeightForChild(child));
> +            break;

This is wrong if the child is a perpendicular writing mode. You need setLogicalHeightForChild and to use logicalHeightForChild, or you could just use physical setters.

[Tony Chang]

Created attachment 110738 [details]
Patch

[Tony Chang]

(In reply to comment #7)
> (From update of attachment 110575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110575&action=review
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:571
> > +        case AlignStretch: {
> > +            Length height = isHorizontalFlow() ? child->style()->height() : child->style()->width();
> > +            if (height.isAuto())
> > +                child->setLogicalHeight(child->logicalHeight() + RenderFlexibleBox::availableLogicalHeightForChild(child));
> > +            break;
> 
> This is wrong if the child is a perpendicular writing mode. You need setLogicalHeightForChild and to use logicalHeightForChild, or you could just use physical setters.

Updated to use physical setters and logicalHeightForChild.  Also added a test case.

[Ojan Vafai]

Comment on attachment 110738 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:578
> +                    child->setHeight(logicalHeightForChild(child) + RenderFlexibleBox::availableLogicalHeightForChild(child));
> +                else
> +                    child->setWidth(logicalHeightForChild(child) + RenderFlexibleBox::availableLogicalHeightForChild(child));

nit: move logicalHeightForChild(child) + RenderFlexibleBox::availableLogicalHeightForChild(child) into a local variable (e.g. newLogicalHeight) so you don't have to repeat the code?

[Tony Chang]

Created attachment 110749 [details]
Patch

[Dave Hyatt]

Comment on attachment 110749 [details]
Patch

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

r=me

> Source/WebCore/rendering/RenderFlexibleBox.cpp:573
> +            Length height = isHorizontalFlow() ? child->style()->height() : child->style()->width();

RenderStyle does have helpers for margins but not for logical height. If you wanted to add a logicalHeightUsing method to RenderStyle helper so you could say child->style()->logcialHeightUsing(style()) I would be fine with that. Not required though.

Also note you don't handle maxHeight clamping here. Not sure if you have to or not. I'd put a FIXME: in asking and not worry about it for now.

[Tony Chang]

Created attachment 110754 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 110754 [details]
Patch for landing

Clearing flags on attachment: 110754

Committed r97313: <http://trac.webkit.org/changeset/97313>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.