bug: pseudo class selectors fails to parse class_names on attribute selectors

[mohamedmansour]

Did you check existing issues?

• I have read all the tree-sitter docs if it relates to using the parser
• I have searched the existing issues of tree-sitter-css

Tree-Sitter CLI Version, if relevant (output of tree-sitter --version)

tree-sitter 0.24.3 (bdfe32402e85673bbc693216f0a6ef72c98bb665)

Describe the bug

All attribute selectors in CSS are allowed to have class name selectors attached to it. For example, adding a :hover. But the parser fails with ERROR nodes.

It seems any selector after the attribute selector [a="b"] if it is within a pseudo_class_selector :host(....) fails. That should be pass.

This is needed to support HTML Web Components, where :host and :host-context are used to decorate the components. These two are missing from tree-sitter-css which accept selectors as parameters.

Steps To Reproduce/Bad Parse Tree

(stylesheet [0, 0] - [1, 1]
  (rule_set [0, 0] - [1, 1]
    (selectors [0, 0] - [0, 20]
      (pseudo_class_selector [0, 0] - [0, 20]
        (class_name [0, 1] - [0, 5])
        (arguments [0, 5] - [0, 20]
          (grid_value [0, 6] - [0, 13]
            (plain_value [0, 7] - [0, 12]))
          (ERROR [0, 13] - [0, 14])
          (plain_value [0, 14] - [0, 19]))))
    (block [0, 21] - [1, 1])))

Expected Behavior/Parse Tree

I expect a class_name to be attached after the attribute selector [a="b"].

(stylesheet [0, 0] - [5, 1]
  (rule_set [0, 0] - [2, 1]
    (selectors [0, 0] - [0, 14]
      (pseudo_class_selector [0, 0] - [0, 14]
        (class_name [0, 1] - [0, 5])
        (arguments [0, 5] - [0, 14]
          (grid_value [0, 6] - [0, 13]
            (plain_value [0, 7] - [0, 12])))))
          (class_name [3, 8] - [3, 13])))

Repro

:host([a="b"]:hover) {
}

[mohamedmansour]

When looking at the grammar for pseudo_class_selector it seems the pseudo classes are defined manually, :has, :not, :is, :where. When I added :host to that CHOICE it worked.

But there are many pseudo class selectors, I am not familiar with tree-sitter, but should we support any string?

https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-classes

A

:active
:any-link
:autofill
B

:blank Experimental
C

:checked
:current
D

:default
:defined
:dir()
:disabled
E

:empty
:enabled
F

:first
:first-child
:first-of-type
:focus
:focus-visible
:focus-within
:fullscreen
:future Experimental
H

:has() Experimental
:host
:host()
:host-context() Experimental
:hover
I

:indeterminate
:in-range
:invalid
:is()
L

:lang()
:last-child
:last-of-type
:left
:link
:local-link
M

:modal
N

:not()
:nth-child()
:nth-last-child()
:nth-last-of-type()
:nth-of-type()
O

:only-child
:only-of-type
:optional
:out-of-range
P

:past
:paused
:picture-in-picture
:placeholder-shown
:playing
:popover-open
R

:read-only
:read-write
:required
:right
:root
S

:scope
:state()
T

:target
:target-within Experimental
U

:user-invalid
V

:valid
:visited
W

:where()

[mohamedmansour]

@amaanq what is the recommendation for supporting all those pseudo class selectors?

[mohamedmansour]

Hey @savetheclocktower do you think this is related to your last change?

[savetheclocktower]

Hey @savetheclocktower do you think this is related to your last change?

Almost certainly, but it also tells us what the fix should be.

The problem was that, inside of a pseudoclass that takes parameters, the parser incorrectly preferred to parse some selectors as plain_values . The full explanation is here.

My fix was to split pseudoclasses with parameters into two buckets — those that expect a selector and those that don't. That fixes the problem, but creates a different problem of maintenance: whenever new pseudoclasses are added that take a selector as a parameter, they have to be added to the list.

I suppose I missed this one when I submitted the fix — I thought the four I named were the only ones that took a selector. You're right that the quickest fix is to add :host to the list.

---

This cries out for a more thorough fix, but I'm not sure what form it would take. Pseudoclasses vary wildly in what sorts of parameters they allow. Here's what you'd want to do:

• Reduce the list in your earlier comment to only the ones that accept parameters
• Describe valid input for each pseudoclass
• See if you can come up with an unambiguous rule that can parse all acceptable input for all different types of pseudoclasses

The last item sounds hard, but it might not be! The problem is that the pseudo_class_arguments rule accepts a number of things including plain_value — which is rather broad and will always be preferred even when we should lex something as a selector. It's possible that pseudo_class_arguments can be rewritten in a way that doesn't use plain_value and instead uses something more restrictive (and is maybe aliased to plain_value).

But pseudoclass parameters are an anything-goes nightmare! I just recently realized that :nth-child and :nth-last-child pseudoclasses can be “qualified” with a selector as follows:

div > :nth-child(1 of p.active) {
  // The first child of a `div` among those that match the selector `p.active`
}

This has the same problem that my change was trying to solve: of is parsed as a plain_value (which is fine), but p.active is as well.

So I think the right answer is still to be more restrictive than we currently are in how we parse all pseudoclass arguments, whether or not we maintain a special “selector-only” list. The _value rule has lots of the things we'd want to identify in that context, but is too broad.

[savetheclocktower]

Anyway, if you wanted to submit a quick fix, you'd just need to add host and host-context to the list of pseudoclasses that take only selectors as parameters. Never mind, did it myself!

[savetheclocktower]

But there are many pseudo class selectors, I am not familiar with tree-sitter, but should we support any string?

To be clear: all others are supported as well, even those that aren't specifically mentioned. You could write p:foo(bar) and it would be parsed correctly, even though that's not a valid pseudoclass right now.

The ones that are mentioned by name are those that need special handling for the reasons mentioned above.

[mohamedmansour]

Great work @savetheclocktower! I just verified all the pseudo selectors are now handled after your PR. Really appreciate the level of detail with your posts! Hopefully that gets merged soon!