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

"combinators" found in nth-child expressions #14

Open
davidtheclark opened this issue Jul 23, 2015 · 13 comments
Open

"combinators" found in nth-child expressions #14

davidtheclark opened this issue Jul 23, 2015 · 13 comments

Comments

@davidtheclark
Copy link
Contributor

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
Copy link
Member

ben-eb commented Jul 23, 2015

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 #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
Copy link
Contributor Author

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
Copy link
Member

ben-eb commented Jul 23, 2015

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
Copy link
Contributor Author

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
Copy link
Member

ben-eb commented Jul 23, 2015

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
Copy link
Contributor Author

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

@ben-eb
Copy link
Member

ben-eb commented Jul 23, 2015

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
Copy link
Contributor Author

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
Copy link
Member

ben-eb commented Jul 23, 2015

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
Copy link

FYI:

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

See 1

@clentfort
Copy link

@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
Copy link
Contributor Author

Thanks for the heads up, @clentfort.

@chriseppstein
Copy link
Contributor

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;
};

@chriseppstein chriseppstein added this to In progress in 4.0 Release Mar 27, 2018
@chriseppstein chriseppstein removed this from In progress in 4.0 Release Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants