postcss / postcss-selector-parser

A CSS selector parser, integrates with postcss but does not require it.
MIT License
203 stars 48 forks source link

"combinators" found in `nth-child` expressions #14

Open davidtheclark opened 9 years ago

davidtheclark commented 9 years ago

Another nth-child problem ...

Given the selector .foo:nth-child(2n + 1), the parser thinks that plus sign is a combinator.

Given the selector .foo:nth-child(2n - 1), the parser thinks the spaces around the minus signs are combinators.

ben-eb commented 9 years ago

This is a tradeoff between the parser being generic enough to support custom pseudo element syntax, whilst also being specific enough to understand common CSS grammar such as the combinator. Are we sure we are not just duplicating the discussion in https://github.com/postcss/postcss-selector-parser/issues/13?

If we add the detection of this case to the parser we have to go down the road of differentiating between pseudo element syntax. The current approach prefers that the user performs her own checking for these expressions, which I prefer as it means that the parser is faster.

davidtheclark commented 9 years ago

Yeah, I see the flexibility argument. However, this is faulty parsing of valid CSS --- I'm guess I'm not going to agree that that's not a problem.

Would it be sufficient to say that anything wrapped in parentheses is an "expression" and therefore plays by different rules? Maybe you don't go to the trouble of really parsing :nth-* expressions. I just can't think of a case where the parser would actually need to find combinators (or even selectors) within parentheses.

ben-eb commented 9 years ago

The main use case is in pseudo elements that take a selector list, for example the negation selector:

h1:not(.main-title, .article-title) {}
davidtheclark commented 9 years ago

Makes sense. But that does mean you're privileging :not() or :nth-*, right --- if :not() will be parsed correctly at the expense of :nth-*?

If user was told that their pseudo-selector :not() contained the expression .main-title, .article-title, their parsed results are not incorrect, they're just incomplete, and the user could run the expression through the parser again to parse those inner selectors. But the :nth-* results are actually incorrect.

*Please understand I'm not trying to be pushy, just trying to troubleshoot with you :)

ben-eb commented 9 years ago

Yes, the :not() case is prioritised. It enables the not so likely but plausible parsing of multiple levels of pseudo selectors, such as:

h1:not(h2:not(h3:not(h4:not(h5:not(h6))))) {}

As I remarked earlier, this is a tradeoff. I wrote it like this because I thought that it was a good idea to allow people to map over nested pseudo selectors easily. If it is not done inside the parser, we must write some user-land code to evaluate the result of that expression through another run of the parser, which I felt was a bit wasteful.

davidtheclark commented 9 years ago

Ok. I'm unconvinced that returning an inaccurate AST on valid selectors is worth this, but I'll close the issue.

ben-eb commented 9 years ago

Actually, I think both approaches are valid. If I had implemented your method in the first place (which was one of the very first considerations) then I may have had someone asking of me the way I have implemented it now.

Would you be happier with a flag for not parsing pseudo selector parameters?

davidtheclark commented 9 years ago

I do think that would be a good addition.

Either way there should probably be some note in the README about the issue; and it would look better if rather than just having to state "The parser will return an inaccurate AST if input CSS contains :nth-* selectors" you could instead qualify that with "By default" and add "The alternative is to ..."

ben-eb commented 9 years ago

Let's say I will accept a PR for an optional flag for the parser to not parse pseudo parameters. I can potentially handle this myself but it would be lower down on the priority list.

clentfort commented 8 years ago

FYI:

Negations may not be nested; :not(:not(...)) is invalid.

See 1

clentfort commented 8 years ago

@davidtheclark I'm currently working on a selector parser that supports plugins for function calls, so it can differentiate between a call to :not, :nth-child etc. It's a bit more heavy weight than this parser but should be easier to extend. So far its only a parser, so only an AST is generated and it does not provide ways to manipulate the AST (or to reprint it to CSS). You can take a look at it here: https://github.com/clentfort/another-selector-parser

davidtheclark commented 8 years ago

Thanks for the heads up, @clentfort.

chriseppstein commented 6 years ago

the :nth-XXX() pseudo classes have a microsyntax https://drafts.csswg.org/css-syntax/#anb-microsyntax.

The general syntax is An + B of S where A is a signed or unsigned integer and B is an unsigned integer, where the + can also be a -. and S is a selector list as is currently expected by the parser.

I suggest that any psuedo with parens starting with :nth be parsed according to this microsyntax.

I think we should add a counter attribute to psuedos to capture the An+B expression. The counter will have a repeat and an offset and the original string value will be stored as raws.counter. If an attribute has a counter and child nodes those will be separated by of unless raws.counterSeparator is set to a different value.

counter: {
  repeat: number;
  offset: number;
}
raws: {
   counter: string;
   counterSeparator?: string;
};