Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow -n in an-b, allow comments within +n and -n #1498

Closed
wants to merge 1 commit into from

Conversation

Manishearth
Copy link
Member

Currently the grammar doesn't allow for -n within :nth-child, it only deals with +n. However, -n (and things like -n+5) are accepted by browsers.

This fixes the discrepancy.

Additionally, the parsing here is a bit tricky -- while the +/- and n are separate tokens, the spec does not allow for spaces between them. Chrome follows this, and as of https://bugzilla.mozilla.org/show_bug.cgi?id=1364009, firefox does too. However, both allow comments. While this is probably implicit, I'm clarifying this within the spec.

See also: servo/rust-cssparser#153,

r? @tabatkins @SimonSapin

@tabatkins
Copy link
Member

tabatkins commented Jun 5, 2017

The grammar does allow for -n and -n+5. Every single "group" of clauses has the first line handle a no-sign initial, the second handle an initial +, and the third handle an initial -.

What it doesn't allow is - and n to be separate tokens; since we're already requiring no spaces between the sign and the n, the only reason to parse them as separate tokens on purpose is to specifically allow comments between them. We can't avoid that for +n, because that parses as two tokens no matter what (and so +/**/n is automatically allowed), but -n naturally parses as one token, and there's no particular reason to allow -/**/n to work - it's just more effort for literally zero payoff.

Chrome does not allow this, by the way:

<!DOCTYPE html>
<p>foo<p>foo<p>foo
<style>p:nth-child( -/**/n + 2 ) { color: red; }</style>

doesn't turn anything red, while +/**/n + 2 does (correctly). It looks like Firefox does allow it, giving the same effect as -n + 2, which I assume is because they don't follow the spec and instead just reparse the source text with a specialized an+b parser.

@tabatkins tabatkins closed this Jun 5, 2017
@tabatkins
Copy link
Member

Oh, and note that comments are allowed anywhere between two tokens. This is implicit in "consume a token" - whenever you ask for the next token, it automatically consumes (and silently discards) any comments as well. In Serialization, it's specified that impls are allowed to remember where comments were, and re-insert them during serialization if they wish, but this information must have no effect on parsing - as far as CSS parsing and grammars are concerned, comments don't exist.

@SimonSapin
Copy link
Contributor

The initial message of this PR is misleading. This is specifically about the -/**/n case.

@tabatkins
Copy link
Member

Sure, but I addressed that specifically in my response, too. Chrome does not allow -/**/n + 2 as far as I can tell, so the premise is flawed.

@Manishearth
Copy link
Member Author

Yeah, I hadn't misread the spec -- but I felt it should be symmetric.

For some reason it seemed like Chrome accepted this? Maybe it was a separate case, I was doing a lot of testing to make Firefox match Chrome and the spec.

Will remove this behavior from Firefox then.

@tabatkins
Copy link
Member

Theoretical purity (symmetry) doesn't apply when we're dealing with "what random-ass places in the middle of expressions can people put comments in?". ^_^ There's a lot of things like this that are just artifacts of CSS tokenization, not something meaningful.

@Manishearth
Copy link
Member Author

Fair. Firefox has a bajillion tests for "what random-ass places can folks put comments in", but I'm just going to remove the ones that test for this behavior and fix up the parsing to match this.

@tabatkins
Copy link
Member

Testing for those things is good; people will put them anywhere they're allowed, because users tessellate the possibility space of an API. But trying to make decisions about where to allow them based on aesthetics is not. ^_^

@Manishearth
Copy link
Member Author

Agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants