Summary: | apply cross axis constraints before aligning children in flexbox | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tony Chang <tony> | ||||||
Component: | New Bugs | Assignee: | Tony Chang <tony> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | mitz, ojan, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Tony Chang
2012-03-26 13:43:52 PDT
Created attachment 133883 [details]
Patch
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. Created attachment 133896 [details]
Patch for landing
(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. Comment on attachment 133896 [details] Patch for landing Clearing flags on attachment: 133896 Committed r112154: <http://trac.webkit.org/changeset/112154> All reviewed patches have been landed. Closing bug. (In reply to comment #6) > All reviewed patches have been landed. Closing bug. Looks like this broke on clang, but mitz fixed it. Thanks! |