1226697 - [css-grid] grid-column-start failure with 'span' named value

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Eric A. Meyer]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151119065326

Steps to reproduce:

I created a testcase based on Example 34 of the CSS Grid Layout Level 1 specification.  You can find the testcase here: http://meyerweb.com/eric/css/tests/grid-level-01/example034.html.  See http://www.w3.org/TR/2015/WD-css-grid-1-20150917/#example-198bb78c for the basis for this test.


Actual results:

All tests passed except for .d05, which is in the wrong place and not the right size.


Expected results:

The .d05 test should stretch from column-line 6 to column-line 9, as noted in the comments.

Comment 1

[ovidiu boca[:Ovidiu]]

Build ID: 20151122030230
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:45.0) Gecko/20100101 Firefox/45.0

Hi,

I tested this and I can reproduce the problem.

Comment 2

[Mats Palmgren (inactive)]

Attachment: [css-grid] Fix off-by-one error when counting lines in reverse.

FindNamedLine's aFromIndex should be the number before the line where it
starts matching, i.e. zero if searching forward from the start of the grid
or aExplicitGridEnd + 1 when searching in reverse from the end.
("before" here meaning in the logical direction of the search)

Or, when searching for a span, the definite line we span from (whether it's
forward or backward doesn't matter since we always want to exclude that line).

I should have caught this in bug 1215957 already, sorry. :-(

Comment 3

[Mats Palmgren (inactive)]

Attachment: reftests

Lot's and lot's of additional tests.  I think I've captured all possible
scenarios now. :-)

Comment 4

[Mats Palmgren (inactive)]

Eric, thanks for the bug report!  FYI, there is a minor problem with your test -
it doesn't really correspond to the one in Example 34 in the spec.  The spec
says "Given a single-row, 8-column grid and the following 9 named lines",
but your example is using: "grid-template-columns: repeat(3, [A] 50px [B] 50px [C] 50px)"
which creates 9 tracks.  The ninth track is clipped by the "width: 401px".
It doesn't really matter much in this case, since none of the items matches
the actual last line (line 10), but this would match Example 34 better:
grid-template-columns: repeat(2, [A] 50px [B] 50px [C] 50px) [A] 50px [B] 50px [C];

Comment 5

[Eric A. Meyer]

Thanks for the correction, Mats—fixed!

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8696522 [details] [diff] [review]
[css-grid] Fix off-by-one error when counting lines in reverse.

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

Sorry for the delay here.

r=me; just a few comment nits.

::: layout/generic/nsGridContainerFrame.cpp
@@ +723,5 @@
>            const nsTArray<nsTArray<nsString>>& aNameList)
>  {
>    MOZ_ASSERT(aNth && *aNth > 0);
> +  if (MOZ_UNLIKELY(aFromIndex == 0)) {
> +    return 0; // There are no named lines before the start of the explicit grid.

The word "before" is ambiguous here. Consider replacing with "beyond" or something else that's clearer.

(It's ambiguous because this is in a reverse-direction-traversal function -- and "before" could refer to traversal-order or numerical-order.  Here, I think you mean "on the start side", i.e. numerical order, but you use the other meaning of "before" in the FindNamedLine documentation when talking about reverse traversal. So it's not immediately clear what it means here.)

Anyway -- "beyond" has the meaning you want, I think, and (to my ear at least) sounds like "more towards the extreme" & hence seems less ambiguous.

@@ +725,5 @@
>    MOZ_ASSERT(aNth && *aNth > 0);
> +  if (MOZ_UNLIKELY(aFromIndex == 0)) {
> +    return 0; // There are no named lines before the start of the explicit grid.
> +  }
> +  --aFromIndex;

Consider adding a comment here, saying e.g.
  // (shift aFromIndex so we can treat it as inclusive)

Otherwise, the code from this point on (which behaves as if |aFromIndex| *is* inclusive) superficially contradicts the FindNamedLine documentation (which says aFromIndex is *not* inclusive).

@@ +748,5 @@
>    return 0;
>  }
>  
> +/**
> + * Find the aNth occurrence of aName, searching forward if aNth positive,

"is positive"

@@ +750,5 @@
>  
> +/**
> + * Find the aNth occurrence of aName, searching forward if aNth positive,
> + * and in reverse if aNth is negative (aNth == 0 is invalid), starting from
> + * aFromIndex (not included), and return a 1-based line number.

I think you mean s/included/inclusive/ here?

("not included" initially made me think "we don't have access to this variable here", which confused me.  "inclusive" is the more standard terminology for range-related edge-case definitions, I think.)

Comment 7

[Mats Palmgren (inactive)]

Fixed those comment nits - thanks!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2391e16acb15
https://hg.mozilla.org/integration/mozilla-inbound/rev/14f29b2683c7

Comment 9

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=18627472&repo=mozilla-inbound starting with this changes

Comment 10

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee89ebbbcae7
https://hg.mozilla.org/integration/mozilla-inbound/rev/095c97530a31

Comment 11

[Mats Palmgren (inactive)]

(In reply to Carsten Book [:Tomcat] from comment #9)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=18627472&repo=mozilla-
> inbound starting with this changes

Hmm, that's weird because this change has nothing to do with SVG and the failing
test (layout/reftests/svg/svg-integration/clipPath-html-06.xhtml) doesn't use
any Grid features at all.  Maybe it's just that adding a few more reftests
made the reftest chunking different which uncovered an existing problem?
I submitted a Try job with only the code changes here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=901b90d0d085

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7639bf1c7f6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0bf39d0e68

Comment 13

[Mats Palmgren (inactive)]

Yeah, comment 11 seems to be the problem.
The SVG reftest succeeds without my new reftests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bab395d91de
and I confirmed that it's merely adding new entries in
layout/reftests/css-grid/reftest.list that makes it fail:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c1d47ba9121
so it's definitely a test chunking problem.

I've filed bug 1233290 about that.
I landed my fix here without the new reftests (for now).

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7639bf1c7f6b
https://hg.mozilla.org/mozilla-central/rev/fb0bf39d0e68

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6deafbb5f171

Comment 16

[Mats Palmgren (inactive)]

I landed the reftests together with bug 1118820 and bug 1151243.
That seems to avoid the reftest chunking problem, at least on Try.
Let's see if it sticks.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6deafbb5f171