Refactor flexbox layout logic some more, to make pagination more straightforward

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla27
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
I split out some of my flexbox-pagination code into some simpler minor-refactoring patches, which my pagination patches stack on top of.

(This is much like bug 885424, just with a few new patches.)
(Assignee)

Comment 1

5 years ago
Created attachment 805768 [details] [diff] [review]
part 1: do flex layout in container's content-box space

So right now, nsFlexboxFrame runs the flexbox layout algorithm using coordinates that are relative to its border-box.

This turned out to make things a bit tricky for pagination patches, because it means there are several places where we need GetSkipSides() checks to find out whether the container's border/padding should be discounted.

SO: This patch makes things simpler by doing flexbox layout in content-box space -- i.e. *ignoring* borders and padding on the container -- and then adding them in at the last minute, when we give each child their final reflow.
Attachment #805768 - Flags: review?(matspal)
(Assignee)

Comment 2

5 years ago
(Note: The two "EnterMargin(ReflowState.mComputedBorderPadding)" calls removed in Part 1 are what currently move our PositionTracker "current position" representation past the flex container's border/padding, so that flex items end up inside of the border/padding instead of on top of it.  Since the patch makes us ignore the container's border/padding until the very end, it can also drop those calls.)
(Assignee)

Comment 3

5 years ago
Created attachment 805771 [details] [diff] [review]
part 2: Make PhysicalPositionFromLogicalPosition() take content-box size and flip polarity if necessary

This patch abstracts some logic into PhysicalPositionFromLogicalPosition().

If we have a right-to-left flexbox that's 200px wide, and we end up with a flex item at main-axis-position 50px, we need to convert that "50px from right edge" into "x-position of 200px-50px = 150px" to get x-position where we should reflow the child.

This patch just pushes that polarity-flipping code into PhysicalPositionFromLogicalPosition(), to make Reflow a little cleaner/clearer.
Attachment #805771 - Flags: review?(matspal)
(Assignee)

Comment 4

5 years ago
Created attachment 805772 [details] [diff] [review]
part 3: move some code up a little

This patch just moves some code up a bit -- in particular, it makes us compute a flex item's "physical" (x,y) position in its container *before* we create its nsReflowState.

Right now, it doesn't matter where we do this computation, but it'll be needed in the earlier spot for pagination support, because when we switch to giving the flex item's reflow state a meaningful availableHeight, we'll need to have its physical y-position in order to compute the right available height to give it.
Attachment #805772 - Flags: review?(matspal)
(Assignee)

Comment 5

5 years ago
Green try run w/ these 3 patches: https://tbpl.mozilla.org/?tree=Try&rev=9410251ffa4a
Comment on attachment 805768 [details] [diff] [review]
part 1: do flex layout in container's content-box space

These changes looks good to me.  r=mats
Attachment #805768 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #805771 - Flags: review?(matspal) → review+

Updated

5 years ago
Attachment #805772 - Flags: review?(matspal) → review+
https://hg.mozilla.org/mozilla-central/rev/c3a3f570a81e
https://hg.mozilla.org/mozilla-central/rev/9e76e17797e8
https://hg.mozilla.org/mozilla-central/rev/67cf1e541846
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.