1373678 - The CSS Grid Inspector Line Numbers (being displayed to authors) are wrong, because they don't take the implicit grid and negative numbers into account

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P1)

Description

[Jen Simmons [:jensimmons]]

Look at this demo:
https://igalia.github.io/css-grid-layout/grid-placement.html

It shows how the explicit placement of Grid items can trigger the creation of an implicit grid. If you place something using negative numbers, you can trigger some implicit grid creation above and to the left of the explicit grid (assuming a horizontal-tb writing mode). Line 1, 2, etc does not move. The first explicit grid line gets the number of 1. Any implicit grid lines before 1 get negative numbers, but do not get any positivity numbers.

Meanwhile, our tool renumbers the lines to always start with 1. Which is wrong.

See how the Inspector tool numbers do not match the numbers on this demo. Ours are wrong. (This demo was created by Igalia, who implemented Grid in Blink & Webkit. They used this page to test their implementation.) 

https://monosnap.com/file/aM55VZWWdSYTFgYiOFp1xjjLY8Tgyt.png

My guess is that our API is wrong. It does not represent a full understanding of how positive and negative numbers work in Grid. :/

Comment 1

[Jen Simmons [:jensimmons]]

Here is a more simple example showing this problem: https://s.codepen.io/jensimmons/debug/GENbzL

With the code: https://codepen.io/jensimmons/pen/GENbzL?editors=1100

Here is the correction that's needed: https://monosnap.com/file/htG1YbfLegYxdtR3gfyiHgNP7PemSV.png

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

Brad, it seems that the platform API should returns the right `number` value in those scenario.

Based on https://monosnap.com/file/htG1YbfLegYxdtR3gfyiHgNP7PemSV.png I think that if the `number` can returns the proper offset (so, 0 instead of 1 in the example), from DevTools side we can extrapolate the negative numbers (the orange ones).

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

I've just talked with Jen at cssday conf, for her this is a blocker for the shipping, I would raise the priority to P1.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

I have already brought this to Brad's attention and we should be expecting a fix soon.

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1373678 Part 1: Reduce grid line numbers by count of leading implicit lines, minimum 0.

Review commit: https://reviewboard.mozilla.org/r/149920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149920/

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1373678 Part 2: Convert tabs to spaces in grid tests.

Review commit: https://reviewboard.mozilla.org/r/149922/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149922/

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1373678 Part 3: Add line number checks to test_grid_implicit.html.

Review commit: https://reviewboard.mozilla.org/r/149924/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149924/

Comment 8

[Brad Werth [:bradwerth]]

Proposed patch is uploaded. Mats can likely review early next week. If we need it faster, let me know and I'll find another reviewer.

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

Notice that after the platform change lands; we still need to update our highlighter. I can take care of that.

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Brad Werth [:bradwerth] from comment #8)
> Proposed patch is uploaded. Mats can likely review early next week. If we
> need it faster, let me know and I'll find another reviewer.

Brad, just to be sure we're on the same page: taking this example:

https://monosnap.com/file/htG1YbfLegYxdtR3gfyiHgNP7PemSV.png

The `number` property should start from 0. But, if the grid-column was -6 instead of -5, for example, the `number` for the column should start from -1. If it was -7, should be start from -2, and so on.

Comment 11

[Brad Werth [:bradwerth]]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #10)
> (In reply to Brad Werth [:bradwerth] from comment #8)
> > Proposed patch is uploaded. Mats can likely review early next week. If we
> > need it faster, let me know and I'll find another reviewer.
> 
> Brad, just to be sure we're on the same page: taking this example:
> 
> https://monosnap.com/file/htG1YbfLegYxdtR3gfyiHgNP7PemSV.png
> 
> The `number` property should start from 0. But, if the grid-column was -6
> instead of -5, for example, the `number` for the column should start from
> -1. If it was -7, should be start from -2, and so on.

I think I understand what you're saying, but I think the only useful value to return for the number property is the line number that would be used by a grid placement. For grid-column, -1 refers to the right-most line, no matter how many implicit columns appear on the left. So I can't think of a good reason to return a number of -1, when that can't be used to place a grid item at that line. 0 seems like the only reasonable number to return for a number that can only be reached with a negative, since you'll have to calculate the negative offsets anyway.

Comment 12

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #11)
> (In reply to Matteo Ferretti [:zer0] [:matteo] from comment #10)
> > (In reply to Brad Werth [:bradwerth] from comment #8)
> > > Proposed patch is uploaded. Mats can likely review early next week. If we
> > > need it faster, let me know and I'll find another reviewer.
> > 
> > Brad, just to be sure we're on the same page: taking this example:
> > 
> > https://monosnap.com/file/htG1YbfLegYxdtR3gfyiHgNP7PemSV.png
> > 
> > The `number` property should start from 0. But, if the grid-column was -6
> > instead of -5, for example, the `number` for the column should start from
> > -1. If it was -7, should be start from -2, and so on.
> 
> I think I understand what you're saying, but I think the only useful value
> to return for the number property is the line number that would be used by a
> grid placement. For grid-column, -1 refers to the right-most line, no matter
> how many implicit columns appear on the left. So I can't think of a good
> reason to return a number of -1, when that can't be used to place a grid
> item at that line. 0 seems like the only reasonable number to return for a
> number that can only be reached with a negative, since you'll have to
> calculate the negative offsets anyway.

To clarify my thinking further... I am interpreting this bug to be "provie the correct line numbers", and not "provide line numbers that make it trivial to label lines in the inspector". If the only goal is to label things correctly in the inspector, then you don't need platform changes. You can just iterate the lines array until you find the first "line.type == 'explicit'" line. That's line 1 and you can work everything out from there.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8878614 [details]
Bug 1373678 Part 2: Convert tabs to spaces in grid tests.

https://reviewboard.mozilla.org/r/149922/#review154700

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8878613 [details]
Bug 1373678 Part 1: Reduce grid line numbers by count of leading implicit lines, minimum 0.

https://reviewboard.mozilla.org/r/149920/#review154710

Initial review notes on part 1 (might have a few more coming, haven't fully grokked this code yet):

::: dom/grid/GridLines.cpp:142
(Diff revision 1)
> +      bool isBeforeFirstExplicit = (aTrackInfo->mNumExplicitTracks == 0) ||
> +                                   (i < aTrackInfo->mNumLeadingImplicitTracks);

Per IRC, the first part of this condition seems like overkill. Please remove it, or add a comment to explain.

::: dom/grid/GridLines.cpp:144
(Diff revision 1)
> +      bool isImplicit = isBeforeFirstExplicit ||
> +                        (i > aTrackInfo->mNumLeadingImplicitTracks +
> +                             aTrackInfo->mNumExplicitTracks);

This boolean is only used to decide between GridDeclaration::Implicit vs. ::Explicit, down below.

Maybe eliminate the boolean and just make the Implicit vs. Explicit decision up here?  That would let us jump straight to the type that we care about faster, and it'd make the SetLineValues(...) call a bit shorter & easier to reason about.

E.g.
  GridDeclaration lineType = (...)
    ? GridDeclaration::Implicit
    : GridDeclaration::Explicit;

  line->SetLineValues(..., lineType);

::: dom/grid/GridLines.cpp:152
(Diff revision 1)
> -        line1Index + numAddedLines,
> -        (
> +        (isBeforeFirstExplicit ? 0 :
> +          line1Index - aTrackInfo->mNumLeadingImplicitTracks + numAddedLines),

Please add a comment to explain the 0 usage here.

(Do we just use "0" as a sentinel for all implicit tracks, so that we don't display anything? If so, please note that.)

Also/alternately: this would be more readable/grokkable if you pulled this complex expression (involving ternary statement and arithmetic) out into a helper variable called e.g. "reportedLineNum", say. (or something else similarly descriptive)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8878613 [details]
Bug 1373678 Part 1: Reduce grid line numbers by count of leading implicit lines, minimum 0.

https://reviewboard.mozilla.org/r/149920/#review154718

::: dom/grid/GridLines.cpp:142
(Diff revision 1)
> +      bool isBeforeFirstExplicit = (aTrackInfo->mNumExplicitTracks == 0) ||
> +                                   (i < aTrackInfo->mNumLeadingImplicitTracks);

You might want to do the bounds-check using "line1Index <=" rather than "i <" here.  (Note: line1Index is defined as i+1). That would make it clearer that we're guaranteed not to underflow in the subtraction you're adding a bit further down, "line1Index - aTrackInfo->mNumLeadingImplicitTracks".

(With the current patch, it's non-obvious whether the subtraction could underflow. But I *think* it won't, based on the relationship between i and line1Index and this mNumLeadingImplicitTracks bounds-check. I'd like for the guarantee to be a little easier to see & reason about.)

::: dom/grid/GridLines.cpp:145
(Diff revision 1)
> +                        (i > aTrackInfo->mNumLeadingImplicitTracks +
> +                             aTrackInfo->mNumExplicitTracks);

(If you agree with my above comment about using line1index instead of i, then you might want to make a similar change here, to keep these explicit-grid-bounds-checks consistent?)

::: dom/grid/GridLines.cpp:218
(Diff revision 1)
> -      aTrackInfo->mRepeatFirstTrack + aRepeatIndex + 1,
> +      (aTrackInfo->mRepeatFirstTrack - aTrackInfo->mNumLeadingImplicitTracks +
> +        aRepeatIndex + 1),

Do we have a guarantee that this subtraction won't underflow?  Could you add an assertion (e.g. that mRepeatFirstTrack >= mNumLeadingImplicitTracks) with a hint for why we're justified in assuming this?

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8878615 [details]
Bug 1373678 Part 3: Add line number checks to test_grid_implicit.html.

https://reviewboard.mozilla.org/r/149924/#review154720

::: dom/grid/test/chrome/test_grid_implicit.html:87
(Diff revision 1)
> +    is(grid.rows.lines[0].type, "implicit", "Grid row line 0 is implicit.");
> +    is(grid.rows.lines[0].number, 0, "Grid row line 0 has correct number.");
> +    is(grid.rows.lines[1].type, "explicit", "Grid row line 1 is explicit.");
> +    is(grid.rows.lines[1].number, 1, "Grid row line 1 has correct number.");
> +    is(grid.rows.lines[3].type, "explicit", "Grid row line 3 is explicit.");
> +    is(grid.rows.lines[3].number, 3, "Grid row line 3 has correct number.");

Could you extend this test with a grid item that has "grid-column: -5;" or similar, to place an item clearly in the implicit grid?  (Maybe there's already an item in the implicit grid here, but I don't immediately see which item & why, from looking at the source.)

Also: ideally, it'd we should be testing a scenario (or several scenarios?) with leading *and* trailing implicit grid-lines (both are possible, right?), and  scenario with more than one implicit track on a side (e.g. mNumLeadingImplicitTracks > 1).

Comment 17

[Brad Werth [:bradwerth]]

Comment on attachment 8878613 [details]
Bug 1373678 Part 1: Reduce grid line numbers by count of leading implicit lines, minimum 0.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149920/diff/1-2/

Comment 18

[Brad Werth [:bradwerth]]

Comment on attachment 8878614 [details]
Bug 1373678 Part 2: Convert tabs to spaces in grid tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149922/diff/1-2/

Comment 19

[Brad Werth [:bradwerth]]

Comment on attachment 8878615 [details]
Bug 1373678 Part 3: Add line number checks to test_grid_implicit.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149924/diff/1-2/

Comment 20

[Brad Werth [:bradwerth]]

Comment on attachment 8878613 [details]
Bug 1373678 Part 1: Reduce grid line numbers by count of leading implicit lines, minimum 0.

https://reviewboard.mozilla.org/r/149920/#review154718

> (If you agree with my above comment about using line1index instead of i, then you might want to make a similar change here, to keep these explicit-grid-bounds-checks consistent?)

This change arguably makes the code more ugly -- have to add a "+1" to the right-hand side -- but it keeps all the comparisons using line1Index.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8878613 [details]
Bug 1373678 Part 1: Reduce grid line numbers by count of leading implicit lines, minimum 0.

https://reviewboard.mozilla.org/r/149920/#review154748

r=me with nits:

::: dom/grid/GridLines.cpp:142
(Diff revision 2)
> +      bool isBeforeFirstExplicit =
> +        (line1Index <= aTrackInfo->mNumLeadingImplicitTracks);

Please add an assertion here (for documentation purposes, if nothing else) to say that line1Index > 0.  (We have that guarantee, because its value from way further up in this function is "line1Index = i+1", and i is unsigned, and IIRC we place a reasonably small limit on track number so integer overflow shouldn't even be possible.)

(Here's the reason this assertion is useful: if line1Index and mNumLeadingImplicitTracks could both be zero, we'd be in a paradoxical situation where we have zero implicit tracks and yet we have isBeforeFirstExplicit=true.  I noticed this when looking at this line & it made me question whether I was understanding this code correctly, but I was able to calm myself down when I realized line1Index *must* be at least 1. :))

::: dom/grid/GridLines.cpp:146
(Diff revision 2)
> +      // For implicit lines that appear before line 1, report a number of 0.
> +      // Those lines can still be addressed with negative indices, but that's
> +      // a calculation to be done by the caller.

Let's offer a reminder or hint to the reader about why we're bothering with this slightly-odd behavior for these lines before the first one. How about something like:

s/negative indices/negative indices (indexed from the *end* of the explicit grid, per the css grid spec)/

Otherwise, our insistence on returning 0 for these lines seems silly/arbitrary. :)

::: dom/grid/GridLines.cpp:151
(Diff revision 2)
> +      GridDeclaration lineType = (isBeforeFirstExplicit ||
> +        (line1Index > aTrackInfo->mNumLeadingImplicitTracks +
> +          aTrackInfo->mNumExplicitTracks + 1))
> +        ? GridDeclaration::Implicit
> +        : GridDeclaration::Explicit;

Indentation here needs some tweaking -- inside of a parenthesized condition, Gecko coding style generally avoids deindenting anything to the left of the open-paren's column, unless absolutely necessary. (And it's not really necessary here.)

Let's just wrap after the "=", and for good measure, add a layer of parens around the arithmetic so that the sides of the greater-than comparison are more clearly separated.  Something like this (which matches Gecko style & which I find more readable):

  GridDeclaration lineType =
    (isBeforeFirstExplicit ||
     line1Index > (aTrackInfo->mNumLeadingImplicitTracks +
                   aTrackInfo->mNumExplicitTracks + 1))
     ? GridDeclaration::Implicit
     : GridDeclaration::Explicit;

::: dom/grid/GridLines.cpp:225
(Diff revision 2)
> +    uint32_t lineNumber = (aTrackInfo->mRepeatFirstTrack -
> +      aTrackInfo->mNumLeadingImplicitTracks + aRepeatIndex + 1);

As above, you shouldn't deindent to the left of the paren, within a parenthesized expression.

In this case you can just drop the parens, because they don't add anything here -- and then, your current indentation becomes fine. :)

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8878615 [details]
Bug 1373678 Part 3: Add line number checks to test_grid_implicit.html.

https://reviewboard.mozilla.org/r/149924/#review154750

Nice! The new test additions look good & give me more confidence in the main patch's correctness. :)

r=me

Comment 23

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Brad Werth [:bradwerth] from comment #12)

> To clarify my thinking further... I am interpreting this bug to be "provie
> the correct line numbers", and not "provide line numbers that make it
> trivial to label lines in the inspector".

It's indeed the first one, "provide the correct line number"; in case then of the examples given, could you please summarize what are the expected values for line numbers given by the API?

Comment 24

[Jen Simmons [:jensimmons]]

I just changed to title to hopefully make this bug more clear. I don't have any opinion on how things work in the API or under the hood. I reported the bug because the line numbers which are displayed to the authors are inaccurate. Maybe this is already clear, but some of the comments above made me think there is confusion about it. 

What we want is for the numbers that are displayed when an Author activates "Display numbers on lines" to be the correct numbers even when they've written code that prompts the Grid algorithm to create implicit grid tracks in the negative number direction (before the first explicit tracks).

Comment 25

[Jen Simmons [:jensimmons]]

I created another example. It's even simpler than my previous one. And as requested, it sends a placed item further out into the implicit grid:

On CodePen: https://codepen.io/jensimmons/pen/bRgRJG?editors=1100#0 (see the code)
The running demo in a page by itself: https://s.codepen.io/jensimmons/debug/bRgRJG (good for testing)
An annotated screenshot: https://monosnap.com/file/SvIGLQFMLllSm0SgQGYKwJhWiYjHcN.png

I've left rows alone here, there's nothing special happening row-wise.

It's also a good example of showing why this bug is bad for authors. If I wanted to place another item, having the wrong numbers would be very confusing. I'd tell something to go to column line 2, looking at the DevTool, when I should be telling it to go to line -4.

I hope this helps. Let me know if you want any other examples.

Comment 26

[Brad Werth [:bradwerth]]

Comment on attachment 8878613 [details]
Bug 1373678 Part 1: Reduce grid line numbers by count of leading implicit lines, minimum 0.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149920/diff/2-3/

Comment 27

[Brad Werth [:bradwerth]]

Comment on attachment 8878614 [details]
Bug 1373678 Part 2: Convert tabs to spaces in grid tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149922/diff/2-3/

Comment 28

[Brad Werth [:bradwerth]]

Comment on attachment 8878615 [details]
Bug 1373678 Part 3: Add line number checks to test_grid_implicit.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149924/diff/2-3/

Comment 29

[Brad Werth [:bradwerth]]

Comment on attachment 8878613 [details]
Bug 1373678 Part 1: Reduce grid line numbers by count of leading implicit lines, minimum 0.

https://reviewboard.mozilla.org/r/149920/#review154748

> Please add an assertion here (for documentation purposes, if nothing else) to say that line1Index > 0.  (We have that guarantee, because its value from way further up in this function is "line1Index = i+1", and i is unsigned, and IIRC we place a reasonably small limit on track number so integer overflow shouldn't even be possible.)
> 
> (Here's the reason this assertion is useful: if line1Index and mNumLeadingImplicitTracks could both be zero, we'd be in a paradoxical situation where we have zero implicit tracks and yet we have isBeforeFirstExplicit=true.  I noticed this when looking at this line & it made me question whether I was understanding this code correctly, but I was able to calm myself down when I realized line1Index *must* be at least 1. :))

I also marked line1Index as const, and added an explanatory comment at the point of definition on why we have that value at all -- basically since line indexes are 1-based, it's helpful for some computations to have a 1-based version of the index.

Comment 30

[Brad Werth [:bradwerth]]

(In reply to Jen Simmons [:jensimmons] from comment #24)
> I just changed to title to hopefully make this bug more clear. I don't have
> any opinion on how things work in the API or under the hood. I reported the
> bug because the line numbers which are displayed to the authors are
> inaccurate. Maybe this is already clear, but some of the comments above made
> me think there is confusion about it. 

Thank you. I believe we're all agreeing on the end state. In comment 11, I was clarifying my intentions for the internal platform API that devtools will use to draw the correct line number. The internal platform API returns only one number for each line, and I've chosen to have it return the positive line numbers for lines 1 on up, and 0 for all other lines (which have no positive line numbers in your examples). Devtools can calculate (and draw) the negative line numbers from this information.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1dfee3f71a0b
Part 1: Reduce grid line numbers by count of leading implicit lines, minimum 0. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/18623c4d94f6
Part 2: Convert tabs to spaces in grid tests. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/994033d6e974
Part 3: Add line number checks to test_grid_implicit.html. r=dholbert

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1dfee3f71a0b
https://hg.mozilla.org/mozilla-central/rev/18623c4d94f6
https://hg.mozilla.org/mozilla-central/rev/994033d6e974

Comment 33

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Bug 1373678 - don't display lines' number for value equals to 0;

Review commit: https://reviewboard.mozilla.org/r/150782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150782/

Comment 34

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8879482 [details]
Bug 1373678 - don't display lines' number for value equals to 0;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150782/diff/1-2/

Comment 35

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: new grid lines' numbers

Notice that the patch for the Grid Inspector is intended to avoid to show the lines' number when the number is 0 (following the example added by Jen); it's not intended to implement the negative ones – that's bug 1369942, and also requires a bit of UX / UI consideration.

Also, we could have a slightly issue (see the attachment): now we don't have always the column and row 1 starting at the beginning, and it might be confusing, I'm not sure. Still, it's not the domain of this bug to fix it, but we should probably give some thoughts about it.

Comment 36

[Patrick Brosset <:pbro>]

Comment on attachment 8879482 [details]
Bug 1373678 - don't display lines' number for value equals to 0;

https://reviewboard.mozilla.org/r/150782/#review155566

::: devtools/server/actors/highlighters/css-grid.js:1336
(Diff revision 2)
>  
>      for (let i = 0; i < gridDimension.lines.length; i++) {
>        let line = gridDimension.lines[i];
>        let linePos = line.start;
> +
> +      if (line.number === 0) {

This needs explanations.
On IRC you said "lines with number 0 are lines that are created not by the user but by the grid itself, when you set negative indexes for example".
Could you perhaps elaborate a bit on that here in a comment?

Comment 37

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8879482 [details]
Bug 1373678 - don't display lines' number for value equals to 0;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150782/diff/2-3/

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd207db57798
don't display lines' number for value equals to 0; r=pbro

Comment 39

[Matteo Ferretti [:zer0] [:matteo]]

Sorry, I totally forgot the leave-open flag. :/

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 8879482 [details]
Bug 1373678 - don't display lines' number for value equals to 0;

https://reviewboard.mozilla.org/r/150782/#review155878

A few drive-by requests here (though, d'oh, it looks like I took too long writing them and this already landed. :)  You might consider pushing a followup to address these, though...)

::: devtools/server/actors/highlighters/css-grid.js:1339
(Diff revisions 2 - 3)
> +      // The first explicit grid line gets the number of 1; any implicit grid lines
> +      // before 1 get negative numbers, but do not get any positivity numbers.

Typo: s/positivity numbers/positive numbers/, I think?

Clarity-wise: this comment leaves the impression that grid lines are numbered e.g. "-2, -1, 1, 2" (with negative numbers on the ones below 1).  But that's not the case -- negative-number line numbers are interpreted as offsets *with respect to the final explicit grid line line*.  Might be worth mentioning or hinting at that in the comment here, since that's the specific bit of subtlety which is the reason we don't label them (I think?)

::: devtools/server/actors/highlighters/css-grid.js:1342
(Diff revisions 2 - 3)
> +      // creation above and to the left of the explicit grid (assuming a horizontal-tb
> +      // writing mode).
> +      // The first explicit grid line gets the number of 1; any implicit grid lines
> +      // before 1 get negative numbers, but do not get any positivity numbers.
> +      // Since here we're rendering only the positive line numbers, we have to skip any
> +      // implicit grid lines before the first tha is explicit.

Typo: s/tha/that/

Comment 41

[Jen Simmons [:jensimmons]]

Yes, please do not show lines that have a number '0'. There is no such thing as a line with the number of '0' in CSS Grid. It's no problem for the Firefox API to secretly be saying 'zero, zero, zero' behind the scenes. In a way, that's a mathematical way to say 'null' / this line doesn't have this kind of number here. But from the perspective of the CSS Grid specification, and from the perspective of authors, showing the '0' is all wrong. If an author tries to use that zero to place an item, it won't work. So let's not confuse Authors. Do not show the '0'.

I also agree with Matteo in comment #35. 
> Also, we could have a slightly issue (see the attachment): now we don't have always the column and row 1 starting at the beginning, and it might be confusing, I'm not sure. Still, it's not the domain of this bug to fix it, but we should probably give some thoughts about it.
The fact the number on the corner labels both the first row line and first column line at the same time is a problem. I already drew a better way to place these line numbers on another ticket. I'll find it, or open another issue to talk about placement of numbers. We want to think about where the negative numbers will go at the same time.

I'm very happy the numbers stay correct now, even when the first line is not line 1. Working through this issue has made me realize displaying the negative numbers too is quite important. https://bugzilla.mozilla.org/show_bug.cgi?id=1369942

Comment 42

[Jen Simmons [:jensimmons]]

Oh, this is the drawing I did: https://monosnap.com/file/FJAy02xXnCiwzQs7EFKjOEwem4SEiV.png

It's on the ticket about negative numbers: https://bugzilla.mozilla.org/show_bug.cgi?id=1369942 

Although I now believe the negative and positive numbers should be on the same side. Let's have that debate on that other ticket.

Comment 43

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Daniel Holbert [:dholbert] from comment #40)

Added a reference about addressing those things in bug 1369942, since this part will be modified anyway there.

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd207db57798