Closed Bug 1005660 Opened 8 years ago Closed 8 years ago

Freeze flex items early, if their base size already violates their [max|min]-size (if we're [growing|shrinking]), to reduce discontinuities

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 2 obsolete files)

Per https://dvcs.w3.org/hg/csswg/rev/b6cb0efb158c , Tab's fixing this in the spec by adding an "early freeze" step.

The idea is: if we're using flex-grow, and we have unfrozen items whose base-size is already greater than their max-size, then we can immediately clamp those items. And we initialize the "original free space" *after* this step.

This ensures that the free-space value *must* approach 0 on each loop of the algorithm, and I believe it'll prevent the sort of discontinuities that prompted me to file this bug. So, I think we want to add an "early-freeze" step like this.

(The above is all symmetric for flex-shrink, of course.)
Summary: Allow "original free space" to grow in flexbox resolve-flexible-widths algorithm, to remove/reduce discontinuities → Freeze flex items early, if their base size already violates their [max|min]-size (if we're [growing|shrinking]), to reduce discontinuities
Attached patch wip (obsolete) — Splinter Review
Attached patch fix v1 (obsolete) — Splinter Review
Here's the fix. A few notes:
 - I clarified the documentation for SetFlexBaseSizeAndMainSize() to indicate that it sets our main size to the Hypothetical Main Size (which we end up using in FreezeItemsEarly(), the new function added in this patch).

 - This patch changes behavior in the flexibilities<1 case (to make things more continuous), because the early-freeze affects how much "initial free space" we think we have.  (so when we treat flexibilities as percentages, they'll now be a percentage of a different value, in some cases.)  The patch updates the testcases from bug 985304 accordingly, so that they still pass.
Attachment #8419811 - Attachment is obsolete: true
Attachment #8423477 - Flags: review?(matspal)
(In reply to Daniel Holbert [:dholbert] from comment #1)
> This ensures that the free-space value *must* approach 0 on each loop of the
> algorithm

SIDE NOTE: As implied by my comment quoted here^, I'm 95% sure that this bug's fix will mean that we don't need the "if the sign of free space matches the type of flexing that we're doing" check[1] anymore. If the free space is nonzero, I think we can now assume that it *must* match our type of flexing. It can't be negative if we're growing, for example.

This check is still encoded into the spec language though -- current language is e.g. "If using the flex grow factor and the remaining free space is positive ". I'm going to email www-style to see if I'm missing anything and if it makes sense to simplify the spec language, and I'm filing a separate bug on changing this in our code.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?rev=b37e7eabd9ef&mark=1705-1708#1705
Blocks: 1011311
Attached patch fix v2Splinter Review
No code changes in this new version -- just changed 2 non-code things:

(1) Previous patch had the wrong bug number - fixed that now.

(2) Previous patch had crashtest orange on Try, from fixing an assertion. The test was previously annotated as asserts(4-8) since it hits 4 assertions per reflow, and it occasionally reflows twice, hence 4-8.  Now that we've prevented 1 assertion, it can be labeled as 3-6 instead.  (The assertion in question is about a flex item that was previously being left frozen longer than expected, due in part to nscoord overflow. This bug's early-freeze keeps that from happening.)

New Try run: https://tbpl.mozilla.org/?tree=Try&rev=f523e22d1495
Attachment #8424159 - Flags: review?(matspal)
Attachment #8423477 - Attachment is obsolete: true
Attachment #8423477 - Flags: review?(matspal)
Comment on attachment 8424159 [details] [diff] [review]
fix v2

Looks good, r=mats
Attachment #8424159 - Flags: review?(matspal) → review+
https://hg.mozilla.org/mozilla-central/rev/4845c0d526e8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.