Bug 73504

Summary: Need to implement flex-flow: column-reverse
Product: WebKit Reporter: Tony Chang <tony>
Component: Layout and RenderingAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 62048    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Tony Chang
Reported 2011-11-30 17:15:48 PST
It's not currently implemented. I'm splitting this off from row reverse because it's considerably more complicated due to needing to repaint above the start of the box.
Attachments
Patch (29.68 KB, patch)
2011-12-01 15:22 PST, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch (30.05 KB, patch)
2011-12-01 15:26 PST, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch (32.28 KB, patch)
2011-12-02 13:57 PST, Tony Chang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Tony Chang
Comment 1 2011-12-01 15:22:19 PST
Created attachment 117501 [details] Patch
Tony Chang
Comment 2 2011-12-01 15:26:20 PST
Created attachment 117502 [details] Patch
Ojan Vafai
Comment 3 2011-12-01 18:57:41 PST
Comment on attachment 117502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117502&action=review > Source/WebCore/rendering/RenderFlexibleBox.cpp:635 > + computeLogicalHeight(); If you move this out of the if-statement, you can remove the last computeLogicalHeight() call in layoutBlock(), right? > Source/WebCore/rendering/RenderFlexibleBox.cpp:646 > + // This is similar to the logic in layoutAndPlaceChildren, except we place the children > + // starting from the end of the flexbox. We also don't need to layout anything since we're > + // just moving the children to a new position. Do we really need to duplicate all this logic? It is not sufficient to just flip the children?
Tony Chang
Comment 4 2011-12-02 13:57:52 PST
Created attachment 117685 [details] Patch
Tony Chang
Comment 5 2011-12-02 13:59:05 PST
(In reply to comment #3) > (From update of attachment 117502 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117502&action=review > > > Source/WebCore/rendering/RenderFlexibleBox.cpp:635 > > + computeLogicalHeight(); > > If you move this out of the if-statement, you can remove the last computeLogicalHeight() call in layoutBlock(), right? If we do that, then in the non-column-reverse case, we lose the overflow (clientLogicalBottom() will be called after we set the height to the computed height). > > Source/WebCore/rendering/RenderFlexibleBox.cpp:646 > > + // This is similar to the logic in layoutAndPlaceChildren, except we place the children > > + // starting from the end of the flexbox. We also don't need to layout anything since we're > > + // just moving the children to a new position. > > Do we really need to duplicate all this logic? It is not sufficient to just flip the children? It's a bit complicated because of how the padding and margins are applied. Based on offline discussion, I consolidated the flex-pack logic into some helper functions to cut down on duplicate code.
Ojan Vafai
Comment 6 2011-12-02 14:13:12 PST
Comment on attachment 117685 [details] Patch LGTM. I'll leave it for Hyatt to r+ though.
Dave Hyatt
Comment 7 2011-12-02 14:23:42 PST
Comment on attachment 117685 [details] Patch r=me
WebKit Review Bot
Comment 8 2011-12-02 19:50:41 PST
Comment on attachment 117685 [details] Patch Clearing flags on attachment: 117685 Committed r101897: <http://trac.webkit.org/changeset/101897>
WebKit Review Bot
Comment 9 2011-12-02 19:50:46 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.