1237805 - [css-grid] Make 'auto-fit' to remove all empty (repeat) tracks, not just those at the end.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

https://lists.w3.org/Archives/Public/www-style/2016Jan/0031.html
"
  - The potential options for how to drop repeated grid tracks with
      auto-fit came down to:
          1. Drop all empty columns
          2. Drop empty columns from either end
          3. Drop empty columns from end end.
      - Several people were against 3 because it removed the
          symmetry of the property.
      - The group didn't have a preference between 1 and 2, so
          decided to stay with 1 since that's what's in the spec now.
"

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Comment 2

[Mats Palmgren (inactive)]

Attachment: reftests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fbf2f0a658a&selectedJob=15194856

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8705639 [details] [diff] [review]
fix

Review of attachment 8705639 [details] [diff] [review]:
-----------------------------------------------------------------

holding off on r+ for the moment, due to the n^2 algorithm issue (see below)

::: layout/generic/nsGridContainerFrame.cpp
@@ +2140,5 @@
>    }
>  
> +  // Count empty 'auto-fit' tracks in the repeat() range.
> +  // |colAdjust| will have a count for each line in the grid of how many
> +  // tracks were removed before that line.

s/"were removed"/"were empty"/.  (We're not removing yet -- we're just counting empty tracks.)

At first, I interpreted "before" here to mean "directly before". But you mean before-going-all-the-way-back-to-the-grid-start.

To avoid that ambiguity, consider a rewording like:
  s/before that line/between the start of the grid and that line/

@@ +2158,5 @@
> +          colAdjust->SetLength(numColLines);
> +          PodZero(colAdjust->Elements(), colAdjust->Length());
> +        }
> +        for (uint32_t i = col + 1, len = colAdjust->Length(); i < len; ++i) {
> +          ++(*colAdjust)[i];

This looks like an n^2 algorithm -- for each empty track, we walk up to the end of the grid. That could get expensive, if there are a lot of empty tracks.

Can you restructure this to just use a single loop, over all of the columns, which tracks numEmptyCols up to the current point & sets colAdjust[i] to the current value of numEmptyCols?

(Same applies to the rows loop below this.)

::: layout/generic/nsGridContainerFrame.h
@@ +192,5 @@
>      /**
>       * Translate the lines to account for (empty) removed tracks.  This method
>       * is only for grid items and should only be called after placement.
> +     * aNumRemovedTracks says how many tracks were removed before each line
> +     * in the grid.

As noted elsewhere, "before" is ambiguous here. Consider rewording, or adding a clarifying parenthetical like "(between the start of the grid and that line)" at the end.

@@ +200,5 @@
>        MOZ_ASSERT(mStart != kAutoLine, "invalid resolved line for a grid item");
>        MOZ_ASSERT(mEnd != kAutoLine, "invalid resolved line for a grid item");
> +      uint32_t numRemovedTracks = aNumRemovedTracks[mStart];
> +      mStart -= numRemovedTracks;
> +      mEnd -= numRemovedTracks;

Could you assert here that no tracks were removed *between* mStart and mEnd?

I think that's equivalent to asserting that aNumRemovedTracks[mStart] == aNumRemovedTracks[mEnd - 1], but I'm not 100% sure...

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix

(In reply to Daniel Holbert [:dholbert] from comment #3)
> > +  // Count empty 'auto-fit' tracks in the repeat() range.
> > +  // |colAdjust| will have a count for each line in the grid of how many
> > +  // tracks were removed before that line.
> At first, I interpreted "before" here to mean "directly before". But you
> mean before-going-all-the-way-back-to-the-grid-start.

That seems like a nonsensical interpretation to me since a "directly before"
count could only be 0 or 1 which contradicts "count ... tracks".

> To avoid that ambiguity, consider a rewording like:
>   s/before that line/between the start of the grid and that line/

Sure, that's more precise.

> This looks like an n^2 algorithm -- for each empty track, we walk up to the
> end of the grid. That could get expensive, if there are a lot of empty
> tracks.

Ah, good catch.  Fixed.

> ::: layout/generic/nsGridContainerFrame.h
> > +      uint32_t numRemovedTracks = aNumRemovedTracks[mStart];
> > +      mStart -= numRemovedTracks;
> > +      mEnd -= numRemovedTracks;
> 
> Could you assert here that no tracks were removed *between* mStart and mEnd?

Sure, aNumRemovedTracks[mStart] == aNumRemovedTracks[mEnd] should hold.
(in fact all aNumRemovedTracks[mStart..mEnd] should be equal but the
above seems like a good enough check)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8705868 [details] [diff] [review]
fix

Review of attachment 8705868 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good! r=me

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba193524a8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceccb1580caa

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ba193524a8b
https://hg.mozilla.org/mozilla-central/rev/ceccb1580caa