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

RESOLVED FIXED in Firefox 45

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({testcase})

unspecified
mozilla45
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8696123 [details]
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)
(Assignee)

Comment 1

3 years ago
Created attachment 8696148 [details] [diff] [review]
fix
Attachment #8696148 - Flags: review?(dholbert)
(Assignee)

Comment 2

3 years ago
Created attachment 8696149 [details] [diff] [review]
reftests
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?
> 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")
(Assignee)

Comment 7

3 years ago
Created attachment 8696240 [details] [diff] [review]
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.
Attachment #8696148 - Attachment is obsolete: true
Attachment #8696148 - Flags: review?(dholbert)
Attachment #8696240 - Flags: review?(dholbert)
(Assignee)

Comment 8

3 years ago
the two lines *of* common code
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
Attachment #8696240 - Flags: review?(dholbert) → review+
(Assignee)

Updated

3 years ago
Flags: in-testsuite+

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/55e55fdb727c
https://hg.mozilla.org/mozilla-central/rev/3b487da5a6d4
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.