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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

See bug 985304 comment 28 & bug 985304 comment 29.

Comment 1

[Daniel Holbert [:dholbert]]

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.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: wip

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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.

Comment 4

[Daniel Holbert [:dholbert]]

(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

Comment 5

[Daniel Holbert [:dholbert]]

Try run with patches for this bug & bug 1011311:
 https://tbpl.mozilla.org/?tree=Try&rev=b28d1276946c

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v2

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

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8424159 [details] [diff] [review]
fix v2

Looks good, r=mats

Comment 8

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4845c0d526e8

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4845c0d526e8