1230695 - [css-grid] abs.pos. child spanning from an 'auto' start position to an end line should end on the start side of the gutter

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase #1

This is probably a regression from adding gutters.

See the testcase:
The green items shows where the tracks are, these start at line 1,2,3,4
and have "span 1".  There's a grid-column-gap:100px between each.

The black area is an abs.pos. with grid-column: 2/4.
The pink area is a grid item with grid-column: 2/4.
Both of these have correct layout.
The red area is an abs.pos. with grid-column: auto/4.
I believe it should end in the same horizontal position as the former two.

For comparison,
the blue area is an abs.pos. with grid-column: 4/auto.
the cyan area is a grid item with grid-column: 4/6.
I believe both of these have correct layout.

(Chrome has wrong layout for both the black and red areas, fwiw)

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Comment 2

[Mats Palmgren (inactive)]

Attachment: reftests

Comment 3

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49345fdc7b38

Comment 4

[Mats Palmgren (inactive)]

I filed a Chrome bug:  https://code.google.com/p/chromium/issues/detail?id=566265

Comment 5

[Daniel Holbert [:dholbert]]

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

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

So the real problem here is that "GridLinePosition" is misleading, now that we live in a world with gutters.  It thinks of a line as having 0 thickness, but they don't necessarily have 0 thickness anymore, if there's a gutter.  So its current behavior -- returning the start-edge of track N -- still ends up being correct for mStart, but not for mEnd.

IMO, as part of this fix, we should tweak GridLinePosition to still be correct. I think the replacement code you're adding in this patch should really be of GridLinePosition(), and GridLinePosition() should take a flag (or an enum) indicating whether we care about the grid line as a track-starter (for mStart) or as a track-ender (for mEnd).  How does that sound?

(I expect the two scenarios should look pretty similar -- the only difference is whether or not we include the size of a particular track, I think -- where N is the number we pass to GridLinePosition).

Also, GridLinePosition might want to be renamed to GridTrackEdgePosition, or something like that, to avoid giving the impression that there is a singular "grid line position".

How does that sound?

Comment 6

[Daniel Holbert [:dholbert]]

> where N is the number we pass to GridLinePosition).

(ignore this, sorry; this was a vestigial afterthought from an earlier version of this comment where I was using "N")

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix

Yeah, that's more readable, thanks.

FYI, I'm intentionally not trying to share the two lines if common code
between the eBefore/After blocks by making a more complicated if-condition.
I bet the compiler will inline this and eliminate the block that doesn't
apply given that aSide is a literal value in both calls.

Comment 8

[Mats Palmgren (inactive)]

the two lines *of* common code

Comment 9

[Daniel Holbert [:dholbert]]

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

>+++ b/layout/generic/nsGridContainerFrame.cpp
>+  if (MOZ_UNLIKELY(aTrackSizes.IsEmpty())) {
>     // https://drafts.csswg.org/css-grid/#grid-definition
>     // "... the explicit grid still contains one grid line in each axis."
>     MOZ_ASSERT(aLine == 0, "We should only resolve line 1 in an empty grid");
>     return nscoord(0);
>   }

Observation: we're not checking |aSide| at all in this empty-grid scenario, which means that an empty grid's singleton-line must always have 0 thickness. (regardless of gutter properties)

At first I wasn't sure if this made sense, but after some thought, I think it's reasonable, since it's a logical extension of the fact that nonempty grids don't have gutters at their extreme edges either. (And this singleton line is the extreme edge(s) of an empty grid.)

Probably worth calling out this special handling (and its justification) with a brief comment, though -- e.g. consider adding this after your existing comment:
    // Disregard aSide -- this line is at grid edge, so it has no gutter.

>+  if (aSide == GridLineSide::eBeforeGridGap) {
>+    if (MOZ_LIKELY(aLine > 0)) {
>+      const TrackSize& sz = aTrackSizes[aLine - 1];
>+      return sz.mPosition + sz.mBase;
>+    }
>+    return aTrackSizes[0].mPosition;
>+  }

Two nits on this chunk:
 (1) Please add a comment to explain what we're doing here. (See suggested comment in code below.)

 (2) IMO this chunk would make more sense to me with the logic flipped, so that the special-case is first, like so:

  if (aSide == GridLineSide::eBeforeGridGap) {
    // Before-edge of Nth grid line is at end of previous track
    // (except for 0th line, which has no previous track & no thickness)
    if (MOZ_UNLIKELY(aLine == 0)) {
      return aTrackSizes[0].mPosition;
    }
    const TrackSize& sz = aTrackSizes[aLine - 1];
    return sz.mPosition + sz.mBase;

Reasons I'd prefer this ordering:
 a) This makes this code easier to compare to the clause immediately below it, since the common code ends up at the same indentation level, instead of being offset from each other.
 b) The "== 0" check (instead of >0) makes it clearer that this is *specifically* a special case for 0 (the first line in our grid), not for negative numbers.  (I know aLine is unsigned, so negative numbers are impossible here. Still, just looking at this chunk in isolation, a "==0" check is easier to immediately grok as a one-off boundary condition, whereas >0 looks ambiguous.)
 c) This structure -- with the boundary case handled before the common case -- is consistent with the code directly after this (for the "after-gap" edge).


>   if (aLine == aTrackSizes.Length()) {
>     const TrackSize& sz = aTrackSizes[aLine - 1];
>     return sz.mPosition + sz.mBase;
>   }
>   return aTrackSizes[aLine].mPosition;

Two things on this chunk:
 (1) For symmetry/consistency with the 0 check above, should we label the "aLine == aTrackSizes.Length()" comparison as MOZ_UNLIKELY?

 (2) As above, a comment for this ^^ code (the "after-gap" code) would help. Suggested comment, to go before the Length() comparison:
    // After-edge of Nth grid line is at start of the track that follows it
    // (except for final line, which has no such track & no thickness).

r=me with the above addressed to your liking

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55e55fdb727c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b487da5a6d4

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/55e55fdb727c
https://hg.mozilla.org/mozilla-central/rev/3b487da5a6d4