Closed Bug 1265001 Opened 8 years ago Closed 8 years ago

nth- parser fails with uppercase N followed by negative number

Categories

(Core :: CSS Parsing and Computation, defect, P3)

45 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: algorithmicimperative, Assigned: MatsPalmgren_bugz)

Details

(Keywords: testcase)

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160309193552

Steps to reproduce:

In either CSS or JS (querySelector), use an `:nth-child` selector that includes a capital `N` followed by a negative number.

This does not happen with lowercase `n-1` or with a positive number `N+1`.

DEMO: https://jsfiddle.net/ajd6Lo5c/1/

CSS:
```
p:nth-child(N-1) {
  color: red
}
```

JS:
```
document.querySelector(":nth-child(N-1)")
```


Actual results:

In CSS, the selector fails so the rule is ignored, and the color is not set to `red`.

In JS, `querySelector` throws a DOM Exception.


Expected results:

In CSS, the selector ought to succeed and style the appropriate elements

In JS, the querySelector ought to fetch appropriate elements.
Component: Untriaged → DOM
Product: Firefox → Core
Component: DOM → CSS Parsing and Computation
Right, the spec is clear that the 'n' should be case-insensitive:
https://drafts.csswg.org/css-syntax/#the-anb-type
Assignee: nobody → mats
Severity: normal → minor
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: testcase
Priority: -- → P3
Attached patch fixSplinter Review
Attachment #8742144 - Flags: review?(cam)
Comment on attachment 8742144 [details] [diff] [review]
fix

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

Please add a test that uses an uppercase :nth-child(...N...).

::: layout/style/nsCSSParser.cpp
@@ +6279,5 @@
> +  const nsASCIICaseInsensitiveStringComparator ciComparator;
> +  auto TokenBeginsWith = [&] (const nsLiteralString& aStr) {
> +    return StringBeginsWith(mToken.mIdent, aStr, ciComparator);
> +  };
> +

I don't think it's necessary to factor out the creation of the comparator here.  It is cheap to construct nsASCIICaseInsensitiveStringComparator() to pass in to StringBeginsWith directly at the call site.

(It would be nice if we had a LowerCaseBeginsWithLiteral, so that we could avoid lowercasing the string we pass in...)
Attachment #8742144 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #4)
> Please add a test that uses an uppercase :nth-child(...N...).

I didn't see the other attachment, but it looks OK.  Would be nice to have a reftest that just verifies we correctly match :nth-child(...N...), too.
Comment on attachment 8742144 [details] [diff] [review]
fix

>+  auto TokenBeginsWith = [&] (const nsLiteralString& aStr) {
>+    return StringBeginsWith(mToken.mIdent, aStr, ciComparator);
>+  };

This violates the coding style guideline at:
https://developer.mozilla.org/en-US/docs/Mozilla/C++_Portability_Guide#Use_C_lambdas_but_with_care
which says:

> We recommend explicitly listing out the variables that you capture in the lambda, both for documentation purposes, and to double-check that you're only capturing what you expect to capture.

Please explicitly list the captured variables (but still capture by reference!).
(I think that would be "[&mToken]".)
> (I think that would be "[&mToken]".)

That fails to compile unfortunately.
What we implicitly capture here is 'this'.
I used [this] instead to make it explicit.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/1157791f8ec1
https://hg.mozilla.org/mozilla-central/rev/4f139d664ccc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.