Closed Bug 1226697 Opened 6 years ago Closed 6 years ago

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


(Core :: Layout, defect)

45 Branch
Not set



Tracking Status
firefox46 --- fixed


(Reporter: eric, Assigned: MatsPalmgren_bugz)


(Blocks 1 open bug)


(Keywords: DevAdvocacy, testcase)


(2 files)

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:  See 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.
Blocks: css-grid
Build ID: 20151122030230
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:45.0) Gecko/20100101 Firefox/45.0


I tested this and I can reproduce the problem.
Component: Untriaged → CSS Parsing and Computation
Ever confirmed: true
OS: Unspecified → Mac OS X
Product: Firefox → Core
Assignee: nobody → mats
Component: CSS Parsing and Computation → Layout
Keywords: testcase
OS: Mac OS X → All
Summary: grid-column-start failure with 'span' named value → [css-grid] grid-column-start failure with 'span' named value
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. :-(
Attachment #8696522 - Flags: review?(dholbert)
Attached patch reftestsSplinter Review
Lot's and lot's of additional tests.  I think I've captured all possible
scenarios now. :-)
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];
Thanks for the correction, Mats—fixed!
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.)
Attachment #8696522 - Flags: review?(dholbert) → review+
Fixed those comment nits - thanks!
Flags: in-testsuite+
sorry had to back this out for test failures like starting with this changes
Flags: needinfo?(mats)
(In reply to Carsten Book [:Tomcat] from comment #9)
> sorry had to back this out for test failures like
> 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:
Flags: needinfo?(mats)
Yeah, comment 11 seems to be the problem.
The SVG reftest succeeds without my new reftests:
and I confirmed that it's merely adding new entries in
layout/reftests/css-grid/reftest.list that makes it fail:
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).
Flags: in-testsuite+ → in-testsuite?
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.