1238294 - [css-grid] Tweak abs.pos. area that ends in line 1, or start in the last line

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase

STR
1. load the attached testcase in a Nightly build

ACTUAL RESULTS
The black areas are grid-aligned abs.pos. children that spans from the padding
edge to a definite line.  In the grid at the top, the areas have auto/1, auto/2
etc. and in the grid below they have 1/auto, 2/auto etc.

As you can see, the first abs.pos. in the top grid and the last in the bottom
grid appear a bit asymmetric to the other ones.

I tend to think we should change this so that they appear symmetrical.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Expected result

Here's the desired rendering: each abs.pos. is now one gap longer than
the one before it ("gap" here meaning the width of a grey area, i.e.
the distance between two tracks).  Notably, both the aforementioned
problematic abs.pos. areas now just covers the padding area (which
also implies they will have zero width if the padding is removed).

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Make GridLineEdge() a method on the Tracks class rather than a static function (idempotent change).

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Treat any gaps at the grid edges as "line thickness" so they behave the same as gaps between tracks for positioning areas.

So here's the actual fix.  It makes "eBeforeGridGap" of the first line be
at the content area start edge, and "eAfterGridGap" of the last line be at
the content area end edge.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Add/tweak reftests for new behavior of gaps at the grid edges.

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

Comment 5

[Daniel Holbert [:dholbert]]

I agree that your expected-behavior here makes sense, and I think it matches the current spec text. In particular:
 * The gutters spec-text says "the grid track between two grid lines is the *space between* the gutters that represent them" https://drafts.csswg.org/css-grid/#gutters
 * For abspos, "an auto value ... contributes a special line ... whose position is that of the corresponding padding edge of the grid container". https://drafts.csswg.org/css-grid/#abspos-items

So IIUC, this means an abspos item with auto/1 should span the space *between* the special padding-edge-line & the gutter for line 1. In particular, it should *not* overlap the gutter for line 1, as it does in current mozilla-central.

Point being:
 - I agree this is a bug.
 - I don't think this needs a spec change.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8706044 [details] [diff] [review]
part 1 - [css-grid] Make GridLineEdge() a method on the Tracks class rather than a static function (idempotent change).

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

r=me on part 1

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8706045 [details] [diff] [review]
part 2 - [css-grid] Treat any gaps at the grid edges as "line thickness" so they behave the same as gaps between tracks for positioning areas.

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

r=me on part 2

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8706046 [details] [diff] [review]
part 3 - [css-grid] Add/tweak reftests for new behavior of gaps at the grid edges.

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

Two notes on the reftests:

(1) If you don't already have something like this, consider adding a testcase for the "interesting" (IMO) scenario where the last grid-line is effectively one giant gutter, due to having "justify-content: start" or "align-content:start" which smooshes all the tracks to the start edge.

e.g. a grid like the following diagram, with "justify-content:start":

line idx:  1      2      3-is-a-big-gutter-3 auto
        +-------------------------------------+
        |  (padding)                          |
        |  +-------------------------------+  |
        |  |      |      |                 |  |
        |  |track1|track2|                 |  |
        |  |      |      |                 |  |
        |  |      |      | (packing space, |  |
        |  |      |      |  i.e. gutter)   |  |
        |  |      |      |                 |  |
        |  +-------------------------------+  |
        |                                     |
        +-------------------------------------+
...with an abspos element (not shown) that has grid-column:3/auto. In current trunk, IIUC, this abspos element would span all the way back to track2, but now it'll just span the right padding-space.

(Maybe your existing tests provide enough coverage for this scenario; if so, great.)

::: layout/reftests/css-grid/grid-abspos-items-013.html
@@ +31,5 @@
>    float: left;
>    min-width: 70px;
>    margin-right: 2px;
>  }
> +.c.s { 

(2) Nit: drop end-of-line space here.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b818db1bd826
https://hg.mozilla.org/integration/mozilla-inbound/rev/642953e50624
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fa8dc9a495f

Comment 10

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> (1) If you don't already have something like this,

grid-abspos-items-013.html covers this case

> (2) Nit: drop end-of-line space here.

Fixed, thanks.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b818db1bd826
https://hg.mozilla.org/mozilla-central/rev/642953e50624
https://hg.mozilla.org/mozilla-central/rev/1fa8dc9a495f