apply cross axis constraints before aligning children in flexbox

[Tony Chang]

apply cross axis constraints before aligning children in flexbox

[Tony Chang]

Created attachment 133883 [details]
Patch

[Ojan Vafai]

Comment on attachment 133883 [details]
Patch

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

Yay! This looks great!

> Source/WebCore/rendering/RenderFlexibleBox.cpp:274
> +    if (!isMultiline() && lineContexts.size() == 1)

Should this also have ASSERT(lineContexts.size() <= 1)? Can we have a non-1 size if it's not multiline other than the case of no flex items? Meh. Maybe it's overkill.

This maybe deserves a comment? I had to read a bunch of code to piece together that we need to reset crossAxisExtent *after* computeLogicalHeight.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:285
> +        lineContexts[0].crossAxisExtent = crossAxisContentExtent();
> +    alignChildren(flexIterator, lineContexts);
> +
> +    if (style()->flexWrap() == FlexWrapReverse) {
> +        if (isHorizontalFlow())
> +            oldClientAfterEdge = clientLogicalBottom();
> +        flipForWrapReverse(flexIterator, lineContexts);
> +    }
> +
> +    // direction:rtl + flex-direction:column means the cross-axis direction is flipped.
> +    flipForRightToLeftColumn(flexIterator);

Would make the code a bit more readable to move this into a function IMO. repositionLogicalHeightDependentFlexItems? It's wordy, but it make the logic of computeLogicalWidth-->layoutItems-->computeLogicalHeight-->resposition more clear, e.g. someone modifying this code will be more likely to understand that they can only reposition things in repositionHeightDependentFlexItems.

I guess the confusing thing there is that alignChildren isn't necessarily height-dependent, but I think that's OK.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:929
>          layoutColumnReverse(children, childSizes, crossAxisOffset, availableFreeSpace);

Should we move this to after the computLogicalHeight call in layoutBlock? Then we would just have the 1 computeLogicalHeight call in all of flexbox layout.

[Tony Chang]

Created attachment 133896 [details]
Patch for landing

[Tony Chang]

(In reply to comment #2)
> (From update of attachment 133883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133883&action=review
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:274
> > +    if (!isMultiline() && lineContexts.size() == 1)
> 
> Should this also have ASSERT(lineContexts.size() <= 1)? Can we have a non-1 size if it's not multiline other than the case of no flex items? Meh. Maybe it's overkill.
> 
> This maybe deserves a comment? I had to read a bunch of code to piece together that we need to reset crossAxisExtent *after* computeLogicalHeight.

I added a comment. Yes, the size == 1 is to make sure we don't have 0 items. Doesn't seem worth an ASSERT.

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:285
> > +        lineContexts[0].crossAxisExtent = crossAxisContentExtent();
> > +    alignChildren(flexIterator, lineContexts);
> > +
> > +    if (style()->flexWrap() == FlexWrapReverse) {
> > +        if (isHorizontalFlow())
> > +            oldClientAfterEdge = clientLogicalBottom();
> > +        flipForWrapReverse(flexIterator, lineContexts);
> > +    }
> > +
> > +    // direction:rtl + flex-direction:column means the cross-axis direction is flipped.
> > +    flipForRightToLeftColumn(flexIterator);
> 
> Would make the code a bit more readable to move this into a function IMO. repositionLogicalHeightDependentFlexItems? It's wordy, but it make the logic of computeLogicalWidth-->layoutItems-->computeLogicalHeight-->resposition more clear, e.g. someone modifying this code will be more likely to understand that they can only reposition things in repositionHeightDependentFlexItems.

Added a helper function.

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:929
> >          layoutColumnReverse(children, childSizes, crossAxisOffset, availableFreeSpace);
> 
> Should we move this to after the computLogicalHeight call in layoutBlock? Then we would just have the 1 computeLogicalHeight call in all of flexbox layout.

Nope, then we would get the wrong bottom edge when calling clientLogicalBottom (when we reposition, there should be no bottom overhang). I could move it, but then I would have to do similar fixup like I do before flipForWrapReverse.

[WebKit Review Bot]

Comment on attachment 133896 [details]
Patch for landing

Clearing flags on attachment: 133896

Committed r112154: <http://trac.webkit.org/changeset/112154>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

(In reply to comment #6)
> All reviewed patches have been landed.  Closing bug.

Looks like this broke on clang, but mitz fixed it.  Thanks!