anandthakker / doiuse

:bomb: Lint CSS for browser support against caniuse database.
MIT License
1.23k stars 50 forks source link

css-not-sel-list matches on leading/trailing whitespace #168

Closed edg2s closed 10 months ago

edg2s commented 1 year ago

The following is a simple not selector, it's not a list:

a:not( .hidden ) {}

but as the regex considers whitespace anywhere to potentially be part of a complex selector, e.g. as in:

a:not(.foo .bar) {}
          ^

we get a false positive.

edg2s commented 1 year ago

I note that other rules account for surrounding whitespace, e.g. https://github.com/anandthakker/doiuse/blob/75ed0d7fc176f3956452be87f3cc1adf67108f46/data/features/css-nth-child-of.js#L12 however it may be worth auditing these newly created rules for other whitespace issues.

clshortfuse commented 1 year ago

Thanks for filing the issue as well as the fix.

It seems you're right about the improper regex. Spec states:

Also as usual, white space is allowed around the arguments inside the parentheses of a functional pseudo-class unless otherwise specified.

https://www.w3.org/TR/selectors-4/#pseudo-classes

I'll work on reviewing the PR and seeing if there are any other related issues around the codebase.

RJWadley commented 12 months ago

I went through all the new features looking for similar issues. I'm not sure whether or not these ones are spec compliant:

svg-css might have false positives

css-rrggbbaa might not catch all 4-length colors

And for these ones I'm not familiar with the spec, so they might be wrong in subtle ways: css-relative-colors css-nth-child-of css-case-insensitive

clshortfuse commented 12 months ago

@RJWadley 👋

Mentioning of the colors reminds me of work I had done on clean-css to fix some improper regex there. So we can probably just reuse their Regex instead of trying to figure it out again:

https://github.com/clean-css/clean-css/blob/master/lib/optimizer/level-1/value-optimizers/color.js

I haven't fully checked it, but I did write a thorough color parsing regex as seen here with the compiler source here. Looking back there are notes on how you don't need to close parenthesis according to CSS spec.

Functions aren't the same as selectors in spec, but worth noting:

A functional notation is a type of component value that can represent more complex types or invoke special processing. The syntax starts with the name of the function immediately followed by a left parenthesis (i.e. a ) followed by the argument(s) to the notation followed by a right parenthesis. White space is allowed, but optional, immediately inside the parentheses. Functions can take multiple arguments, which are formatted similarly to a CSS property value.

Some legacy functional notations, such as rgba(), use commas unnecessarily, but generally commas are only used to separate items in a list, or pieces of a grammar that would be ambiguous otherwise. If a comma is used to separate arguments, white space is optional before and after the comma.

https://www.w3.org/TR/css-values-3/#functional-notations

It sounds like some functions can be comma separated. We don't want to validate the CSS, just detect with the most basic parsing. That means even if somebody decides to incorrectly add commas to something like calc(), we shouldn't care.


Summed up, functional notation and CSS selectors can share the same regex if we're just checking by name (rgba, calc, :is). If checking the parameters themselves, we should first ignore whitespace near the parentheses, then do whatever extra if required for the check (eg: counting values, checking for commas, checking for lists, etc.)

Edit: Looking for closing parenthesis and any preceding whitespace isn't required, but I'd be fine with requiring it if it simplifies the check.