Re: [css-grid] Grid Layout Algorithm Rewritten!

On Fri, Apr 25, 2014 at 10:18 AM, Simon Sapin <simon.sapin@exyr.org> wrote:
> This looks much nicer, thanks! A few comments / questions:
>
>
> In §10.3 Initialize Track Sizes:
>
>> Initialize each track’s base size and growth limit. For each track,
>> if the track’s min track sizing function is:
>>
>> * For fixed track sizes, resolve to an absolute length and use that
>> size.
>
>
> Use that size for both the initial base size and the initial growth limit?

This section appears to have gotten mangled during some bit of our
editing.  We've restored to a previous version and fixed things up a
bit.

> Also, the "If XX is: / For YY, …" wording doesn’t really work. Maybe
> "Depending on XX: / For YY, …" or "If XX is: / A YY, …"

Kept "If...", but switched to a DL with better wording.

>> * For intrinsic track sizes, use an initial base size of zero and an
>> initial growth limit of infinity.
>> * For flexible track sizes, use an initial base size of infinity.
>>
>> A track with a flexible minimum sizing function is treated exactly as
>> if it had a fixed min track sizing function of zero; except when the
>> grid container is being sized under a min-content constraint, in
>> which case it is treated exactly as min-content.
>
>
> Isn’t that paragraph in contradiction with the last bullet point above?

The previous bullet points were nonsense.  Fixed now.

> Do "<em>minimum</em> <i>sizing function</i>" and "<i>min track sizing
> function</i>" (in Bikeshed markup) refer to the same thing? If so, it may be
> preferable to use the same markup and cross-reference links.

Yes, fixed.  (And reworded that whole paragraph.)


> In §10.4 Resolve Content-Based Track Sizing Functions:
>
>> Note: […] This algorithm may be updated in the future to take into
>> account more advanced heuristics as they are identified.
>
> Hopefully I don’t need to, but I’ll point out that once web content starts
> to depend on this algorithm, we may not be able to change it without
> breaking something.

Of course.

> In the "Increase sizes to accommodate spanning items" sub-algorithm, the
> first sub-step has a bold label "For min-content minimums", but then affects
> tracks with either min-content or max-content minimums.
>
> Same for the third sub-step "For min-content maximums".
>
> It seems that it’s the labels that should be fixed, since the list is
> ordered and the order only matters if the sets of affected tracks are not
> disjoint.

Yes, fixed.

>> (This prevents the size increases from becoming order-dependent.)
>
> Is this referring to the order of the grid items?

Kinda sorta. It's possible, if you do this algo wrong, for the order
in which you process things to affect the outcome.  Do you think this
needs any more clarification?

>> Distribute the space equally to the planned increase of each spanned
>> track with an affected size, freezing tracks as their planned size
>> reaches their growth limits (and continuing to grow the unfrozen
>> tracks as needed).
>
> What does "their planned size reaches their growth limits" means when the
> affected size *is* the growth limit?

It means that nothing happens, because the affected size is already at
the growth limit, by definition. ^_^  I've added a clarifying note.

> Also, I don’t quite understand what this freezing is about. The "as […]
> reaches" and "continuing" wording implies a flow of time, while CSS is
> stateless. I can guess what is intended by imagining pouring a fixed amount
> of liquid (extra space) into a number of buckets (tracks) at an equal rate,
> and stopping with a given bucket when it’s full (thus increasing the amount
> of remaining space-liquid for each of the remaining tracks-buckets.)

CSS is stateless, but whenever we use an algorithm to express its
constraints, by the nature of having ordered steps that algorithm has
a notion of time.

> But I shouldn’t have to guess, and the spec perhaps shouldn’t use my liquid
> metaphor :)
>
> I suppose the spec should spell out an iterative algorithm rather than try
> to condense it in one sentence with "freezing" tracks. It would possibly
> look like: [pseudo-code]
>
> The above is just my guess and may be wrong. And I’m sure it can be
> expressed more nicely :)

The spec previously (in the MS version of the algo) had an explicit
iterative algorithm.  It was *extremely confusing and unclear*, and
took us a significant amount of time to figure out what it was doing.
We came up with the liquid metaphor as the clearest way we could
invoke the desired behavior without being overly verbose.  If you have
a clearer way to phrase it, we'd love to see it.

(We had thought about doing a multiple-round freezing/distributing
thing, like Flexbox does, but it seemed like overkill since this was
so much simpler.)

>> If a track was marked as infinitely growable for this phase, treat
>> its growth limit as infinite for this calculation (and then unmark
>> it).
>
> The earlier occurrence of "infinitely growable" should be a <dfn>, and this
> one should link to it.

Good catch.  Done.

>> (If the growth limit is infinite, set it to the track’s base size
>> plus the calculated increase.)
>
> Does this apply always, or only when the affected size is a growth limit?

Either way works, since base sizes can't be infinite.  To avoid
confusion, we've switched the term to "affected size".

>> At this point, all intrinsic base sizes and growth limits will have
>> been resolved to absolute lengths.
>
> Is "this point" the end of a single invocation of "distribute extra space",
> or the end of "Resolve Content-Based Track Sizing Function"?

The latter. We've clarified that remark a bit, and made it properly a note.

> In §10.5 Grow All Tracks To Their Max
>>
>> If the free space is positive, distribute it equally to all tracks,
>> freezing tracks as they reach their growth limits (and continuing to
>> grow the unfrozen tracks as needed).
>
> Same comment about "freezing" than above. Perhaps this could be a named
> sub-algorithm used in two places.

Same response as above.  (Though if we do make the previous one more
complicated, then we would handle it as a named sub-algo.)

> In §10.6.1 Find the Size of an fr
>>
>> Let leftover space be the space to fill minus the base sizes of all
>> the non-flexible grid tracks.
>
> Does "all the non-flexible grid tracks" mean all in the grid, or all in the
> input to this sub-algorithm? I suppose only the latter makes sense, but
> "all" is misleading. Perhaps just removing "all" is enough.
>
> (Same for two occurrences of "all flexible tracks".)

Yes, the latter.  Fixed as you suggested.

>> For all flexible tracks, if the product of the hypothetical flex
>> fraction and the track’s flex factor is less than the track’s base
>> size, restart this algorithm treating all such tracks as having a
>> flex factor of zero.
>
> Restart if *any* or *all* of the flexible tracks have a greater product than
> base size? Does "such" refer to all flexible tracks, or only those with a
> greater product?

Rephrased to:

# If the product of the <a>hypothetical flex fraction</a> and a
flexible track's <a>flex factor</a>
# is less than the track's base size,
# restart this algorithm
# treating all such tracks as inflexible.

Better?

~TJ and fantasai

Received on Monday, 7 July 2014 16:39:24 UTC