1229999 - [css-grid] Assertion failure: "invalid start line" with grid, rel & abs pos

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: aStart >= nsStyleGridLine::kMinLine && aStart <= nsStyleGridLine::kMaxLine (invalid start line), at layout/generic/nsGridContainerFrame.h:139

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Mats Palmgren (inactive)]

Attachment: fix

The problem is that ResolveAutoPosition is a bit too permissive:
http://hg.mozilla.org/mozilla-central/annotate/e02b17a2b5b8/layout/generic/nsGridContainerFrame.h#l169

The kTranslatedMaxLine there is large enough that even with a maximum
number of implicit tracks before line 1, it still allows auto-placed
items up to kMaxLine in the untranslated grid.  The problem is that
with no, or just a few, implicit lines before line 1, it allows
lines that would be > kMaxLine in the untranslated grid.

This isn't much of a problem - we can deal with kTranslatedMaxLine
lines just fine, and the layout is correct.  It's just that we clamp
definite lines numbers and auto-line numbers a bit differently.

The assertion occurs because before we resolve lines for abs.pos.
children (after auto-placement) we untranslate the grid and that leads
to some lines being > kMaxLine which when we create the LineRange
for the abs.pos. item in this case asserts in the ctor:
http://hg.mozilla.org/mozilla-central/annotate/e02b17a2b5b8/layout/generic/nsGridContainerFrame.h#l129

This patch seems like the most correct solution since it treats auto-placed
items equally with definite-placed items w.r.t clamping.  There's a small
runtime cost here though.

An alternative solution would be to just relax this assertion, but only
for abs.pos. children.

Comment 3

[Mats Palmgren (inactive)]

Attachment: reftest

Comment 4

[Daniel Holbert [:dholbert]]

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

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

Commit message nit:
> Clamp auto-placed lines the where kMaxLine is in the translated grid. r=dholbert

"the where" seems like a typo.

::: layout/generic/nsGridContainerFrame.h
@@ +166,5 @@
>        MOZ_ASSERT(IsAuto(), "Why call me?");
>        mStart = aStart;
>        mEnd += aStart;
>        // Clamping per http://dev.w3.org/csswg/css-grid/#overlarge-grids :
> +      uint32_t translatedMax = aExplicitGridOffset + nsStyleGridLine::kMaxLine;

Can you extend the comment slightly to explain the new behavior? (Right now the comment sounds like one can find an explanation for this aExplicitGridOffset addition in the #overlarge-grids spec section; but I don't think it's really clear.)

Maybe just add a parenthetical, so we end up with e.g.:
  // Clamping to kMaxLine (in the explicit grid), per
  // http://dev.w3.org/csswg/css-grid/#overlarge-grids :

Also: might be worth indicating in this function's documentation that |aStart| is a line-index in the *implicit* (untranslated) grid. (It is, right?)  This would help make the aExplicitGridOffset parameter & addition more obviously-correct, too.  (and the old kTranslatedMaxLine comparison obviously-bogus, since it's from a different coordinate space)

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef6fa56c99ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01ad4aa08d5

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ef6fa56c99ca
https://hg.mozilla.org/mozilla-central/rev/d01ad4aa08d5