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

Add support for pseudo elements #40

Merged
merged 3 commits into from Sep 30, 2019
Merged

Add support for pseudo elements #40

merged 3 commits into from Sep 30, 2019

Conversation

benoitkugler
Copy link
Contributor

Hi again,
This PR adds support for pseudo elements.

The approach taken boils down to adding a method PseudoElement on Sel, even for simple selectors, where the implementation is "empty".

@andybalholm
Copy link
Owner

Somehow we need to make sure that a pseudo-element selector doesn't match anything at all unless the caller explicitly supports pseudo-elements.

Would it be better to have a MatchPseudoElement(n *html.Node) (match bool, pseudo string)? It would return true, "" on a non-pseudo match and true, "first-line" (or whatever) on a pseudo match.

@benoitkugler
Copy link
Contributor Author

Why not, but I'm not sure to understand the need for this, since pseudo-elements can't change the node selected. They are only linked to a selector, and can be retrieved on matching, but don't have any influence on the matching (specificity aside).
Or am I missing something ?

@andybalholm
Copy link
Owner

A selector like button::before does not match any real elements. It only matches pseudo elements. Try entering document.querySelectorAll("button::before") in the developer console for this page. You'll see that it returns an empty node list, even though there are buttons on the page.

@benoitkugler
Copy link
Contributor Author

Ah, you're right !
Well, from what I understand, it's up to the user to validate the pseudo-element, that's why a browser won't give you anything for an invalid pseudo-element.

@andybalholm
Copy link
Owner

If somebody enters a selector with a pseudo-element into a program that doesn't support pseudo-elements, they should either get an error, or it should do nothing. So we need to come up with an API that works this way for programs that don't specifically ask for pseudo-element support.

@benoitkugler
Copy link
Contributor Author

benoitkugler commented Sep 30, 2019

In this case, we could add a boolean parameter withPseudoElements to Parse() : when false, parsing a pseudo-element return an error; when true it behaves like in this PR (that is "button::before" match every button).
The Compile() function could call Parse() with withPseudoElements = false to maintain backward compatibility.

@andybalholm
Copy link
Owner

I think you're on the right track. But I don't want to clutter up the common use case (no pseudo elements) with an extra parameter. I suppose that means we'd need to add a ParseWithPseudoElements method or something.

I noticed you're working on porting a project from Python. Does it use a general-purpose selector library? If so, how does it handle this issue?

@benoitkugler
Copy link
Contributor Author

They have built their own selector library cssselect2.
Actually, this PR is directly inspired from their API : for example, the selector button::before applied on the html <button></button>, does match, returning the specificity and the pseudo element before.
I guess the support of pseudo elements was a goal from the beginning.

@benoitkugler
Copy link
Contributor Author

So, as you suggested, I added a method ParseGroupWithPseudoElements. The default behavior for Parse and ParseGroup is left unchanged : selector with pseudo-element error at parse time.

@andybalholm
Copy link
Owner

Your doc comment mentions ParseWithPseudoElements, but you didn't implement that, just ParseGroupWithPseudoElements.

@benoitkugler
Copy link
Contributor Author

Added ParseWithPseudoElement alongside ParseGroupWithPseudoElements

@andybalholm andybalholm merged commit dfaf055 into andybalholm:master Sep 30, 2019
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