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

Fix adjacent logical selectors #138

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

KindaOK
Copy link
Contributor

@KindaOK KindaOK commented Dec 2, 2024

Previously, adjacent logical selectors such as :is(*):is(*) would crash because the regex would incorrectly match *):is(*

Replacing the naive match contents with something that matches balanced parentheses correctly handles these situations.

Resolves #137

Previously, adjacent logical selectors such as `:is(*):is(*)` would
crash because the regex would incorrectly match *):is(*

Replacing the naive match contents with something that matches balanced
parentheses correctly handles these situations.
@KindaOK
Copy link
Contributor Author

KindaOK commented Dec 2, 2024

Per #137, no regressions on the following wpt tests

has basic
has relative
not complex
is where basic
is nested

I don't have the full test setup locally, so that will need to be checked still. Also will need a performance check for tuning depth. A depth of 3 seems pretty good in terms of covering real scenarios, but if it doesn't hurt to go higher, then increasing it to something like 4-5 would be nice.

@KindaOK
Copy link
Contributor Author

KindaOK commented Dec 2, 2024

This method also does not work for all logical selectors. Sample failing test cases below. It seems like it's related to commas or number of characters between inner parentheses.

// fails with SyntaxError: unknown pseudo-class selector
:is(:is(*, *), *)
:has(> :is(*), *)
:has(:is(*),*)

This PR will not fix #127

Might be fixable by modifying the regex to allow more characters in between nested parens?

@KindaOK
Copy link
Contributor Author

KindaOK commented Dec 3, 2024

Funnily enough, :is(*,:is(*)) works.

@KindaOK
Copy link
Contributor Author

KindaOK commented Dec 3, 2024

Oh found it. The problem is that commas are interpreted as different selectors sometimes within the context of logical selectors in the initial parse in match

https://github.com/dperini/nwsapi/blob/master/src/nwsapi.js#L1534

@dperini
Copy link
Owner

dperini commented Dec 3, 2024

@KindaOK
great help from you on these isssues, I agree that your commit for issue #137 is the best we can have at the moment,maybe it can be improved in the future.
For the weekend I wish to have it tested and do the commits for a new release.
I will start merging your pull request today to have the time to execute all the necessary tests.
Again thank you for your time and for the correct suggestions, I really appreciated your help.

@dperini
Copy link
Owner

dperini commented Dec 3, 2024

@KindaOK
two posiby related things to watch out that could be adversely affecting current status:

  1. the :has( ... ) pseudo-class cannot be nested which is the contrary for other logical selectors
  2. the parser should accept mangled selectors, selectors missing a closed quote or double quote or brackets/squares

Just a note for when performing the tests.

@KindaOK
Copy link
Contributor Author

KindaOK commented Dec 4, 2024

  1. the :has( ... ) pseudo-class cannot be nested which is the contrary for other logical selectors

There's no special behavior in place to prevent this at the moment neither in the current version of nwsapi nor in my branch. I think it's hard but doable with regex using a balanced parentheses matcher. Something like "(?!:has\(([^()]*|" + createBalancedParensMatcher(...) + "):has\(". Obviously not complete since parentheses could be put in attribute string comparisons, but that goes back to just using a parser.

  1. the parser should accept mangled selectors, selectors missing a closed quote or double quote or brackets/squares

My PR doesn't support that, but mangled selector support already has been gone for a bit. It looks like e60b058 was the commit that removed it. I don't think it can be easily thoroughly readded without a parser setup. The simple case for missing end parenthesis could be resolved by replacing the end \x29 with (?:\x29|$) I suppose though

@dperini
Copy link
Owner

dperini commented Dec 4, 2024

@KindaOK
well let's apply the changes for these new pseudo-classes and see what the other developers have to say about these new functionalities, try to improve the parsing and the resolvers and getting their reviews and suggestions.
Thank you for your continuous help and research.

@bmm6o
Copy link

bmm6o commented Jan 15, 2025

Is there a plan for a release that includes this fix? I don't see anything in the wiki about release targets or cadences, but it looks like you've been releasing ~bimonthly recently? For us, this is a blocker for Svelte 5.

@dperini
Copy link
Owner

dperini commented Jan 16, 2025

@bmm6o
I am sorry if this blocks any other development or project.
However you should avoid depending on edge functionalities like these
or at least have different backup solutions to ensure your project works despite
wrong or faulty nwsapi implementations that were not part of the listed functionalities.

I will do my best to increase the releases frequencies related to the :has() pseudo-class but to be honest
I still do not have this clear nor I have a definitive strategy on how to write the correct resolver that can keep up
with the speed of other resolver. The good thing is that this pseudo-class already reached 90% positive resolution
coverage of the available wpt tests.

Next testing strategy candidate will be using "compareDocumentPosition"
and see if we can reach resolution on more cases an still keep decent speed.

Stay tuned, reviews, suggestions and testing are welcome and appreciated.

@bmm6o
Copy link

bmm6o commented Jan 17, 2025

Sounds good. And it might be helpful if I add some context. The dependency chain for us is Vite/jsdom/nwsapi, and what surfaced the bug is that Svelte 5 seems to generate more complex css selectors than Svelte 4 did. This is on the same codebase, so it's not selectors that we are intentionally building. I expect others from the Svelte community will run into this soon if they haven't already, which is what I meant by "us" - I hope I didn't sound too rude earlier.

We are investigating alternatives, which I think would be to replace jsdom.

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.

Logical Selector regex does not handle some nested + chained selectors correctly
3 participants