1452368 - [css-grid] clamping limit for auto-placed items is off by 1

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Mats Palmgren (inactive)]

The bug is that 'aExplicitGridOffset' is 1-based grid value while
this functions deals with 0-based line number, so there should have
been a additional "- 1" here:
https://searchfox.org/mozilla-central/rev/2ce99e8054b0ff6ed1adf484aeaacacf2fea084c/layout/generic/nsGridContainerFrame.cpp#447

Not a big deal currently, since we handle an extra track just fine,
but I'm using the existing clamping machinery to constrain lines
to be within the subgrid bounds and it's crucial that it's correct
in that case to avoid index-out-of-bounds errors.

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] clamping limit for auto-placed items is off by 1.

I'm now passing in a pre-computed limit as a param instead.  It's
more efficient, but mostly to prepare for upcoming subgrid changes
that needs a varying limit (the subgrid's explicit grid bounds).

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Adjust reftests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7c451982c7691d121f053b315c98cb198814f06&selectedJob=172415043

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8965952 [details] [diff] [review]
part 1 - [css-grid] clamping limit for auto-placed items is off by 1.

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

Bug 1452368 part 1 - [css-grid] clamping limit for auto-placed items is off by 1. r=dholbert

Nit: this commit message should be phrased such that it refers to the change, rather than the problem being addressed.

(e.g. maybe "Fix off-by-1 calculation in line clamping limit for auto-placed items")

::: layout/generic/nsGridContainerFrame.cpp
@@ +434,5 @@
>    auto Range() const { return IntegerRange<uint32_t>(mStart, mEnd); }
>  
>    /**
>     * Resolve this auto range to start at aStart, making it definite.
> +   * @param aClampMaxLine the maximum allowed line number (zero-based)

s/zero-based/zero-based index in the implicit grid/, perhaps?

Or some clarification like that, to make it clearer why we're bothering with this arg instead of just hardcoding kMaxLine here (whose documentation suggests that *it* is the maximum line number):
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#1302-1306

@@ +3276,5 @@
>    const int32_t offsetToRowZero = int32_t(mExplicitGridOffsetRow) - 1;
>    mGridColEnd += offsetToColZero;
>    mGridRowEnd += offsetToRowZero;
> +  clampMaxColLine += offsetToColZero;
> +  clampMaxRowLine += offsetToRowZero;

This seems to be the first place we touch these variables, though they were declared way further up in this patch.  Maybe we should just declare them here? (and even declare them as "const" if you like)  I don't see why their decl/usage is spread out right now.

So maybe this should be:
  const int32_t clampMaxColLine = nsStyleGridLine::kMaxLine + offsetToColZero;
  const int32_t clampMaxRowLine = nsStyleGridLine::kMaxLine + offsetToRowZero;

@@ +3308,5 @@
>        cursors.emplace();
>      }
>      auto placeAutoMinorFunc = isRowOrder ? &Grid::PlaceAutoCol
>                                           : &Grid::PlaceAutoRow;
> +    uint32_t clampMaxLine = isRowOrder ? clampMaxColLine : clampMaxRowLine;

This is assigning an int32_t value into a uint32_t.  Perhaps we should have an assertion somewhere that both clampedMax***Line variables are nonnegative?  (maybe right below their definitions?)

(Or perhaps those variables themselves should be uint32_t? Though they're assigned using the sum of two other int32_t, so that change would just push the problem of type-conversion bounds-checking to that assignment rather than this one.)

Comment 4

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11; use needinfo to be sure I see pings on return) from comment #3)
> Nit: this commit message should be phrased such that it refers to the
> change, rather than the problem being addressed.

OK, changed as suggested.

> > +   * @param aClampMaxLine the maximum allowed line number (zero-based)
> 
> s/zero-based/zero-based index in the implicit grid/, perhaps?

Meh, I think it's reasonably clear as is.

> Or some clarification like that, to make it clearer why we're bothering with
> this arg instead of just hardcoding kMaxLine here (whose documentation
> suggests that *it* is the maximum line number):
> https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.
> h#1302-1306

It should be obvious why we're not doing that once we land
the subgrid patches, since it passes a different value.

> > +  clampMaxColLine += offsetToColZero;
> > +  clampMaxRowLine += offsetToRowZero;
> 
> This seems to be the first place we touch these variables, though they were
> declared way further up in this patch.  Maybe we should just declare them
> here? (and even declare them as "const" if you like)  I don't see why their
> decl/usage is spread out right now.

These patches are mostly just preparing the code for subgrid,
so this will also be clearer once that happens.

> > +    uint32_t clampMaxLine = isRowOrder ? clampMaxColLine : clampMaxRowLine;
> 
> This is assigning an int32_t value into a uint32_t.  Perhaps we should have
> an assertion somewhere that both clampedMax***Line variables are
> nonnegative?  (maybe right below their definitions?)

This method is special in that it starts with a 1-based grid with
line numbers ranging from kMinLine (negative) to kMaxLine and then
translates the whole grid to a zero-based grid with only positive numbers:
https://searchfox.org/mozilla-central/rev/53afcfdbabed96883126d0ebbcac499b358e32f2/layout/generic/nsGridContainerFrame.cpp#3176
(which is what the "+= offsetToColZero" is for above)
I think it's reasonably clear that those variables are then positive
after that point.

I guess we could introduce a separate set of uint32_t variables for
the latter part of the method, or split it up into smaller methods
for each step, but this method follows the Grid spec algorithm pretty
closely so it's nice to have it all in one place.
https://drafts.csswg.org/css-grid/#auto-placement-algo

I'll keep it as is for now, but I'll consider splitting it up after
subgrid lands since it will add more code here so it might be more
motivated then.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19214ffe89a8
part 1 - [css-grid] Fix off-by-1 calculation in line clamping limit for auto-placed items.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d79a9d3be10
part 2 - [css-grid] Adjust reftests.

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/19214ffe89a8
https://hg.mozilla.org/mozilla-central/rev/8d79a9d3be10