Incorrect tests for specificity and css modules in selector-max-specificity

[romainmenke]

see :

stylelint/lib/rules/selector-max-specificity/__tests__/index.mjs

Line 451 in 382961f

code: ':global(.b, :local(.c)) {}',

These tests are problematic for multiple reasons:

• :global and :local are non-standard and it isn't defined what their specificity is.
• :global and :local don't have any specificity of their own in @csstools/selector-specificity, so ignoring them to reduce their specificity seems weird?
• :global and :local do not support selector lists in implementations
  • lightningcss errors
  • postcss-modules mangles the output (:global(.page, .bar) -> .page .bar)

They also dominate the implementation of selector-max-specificity.

As far as I can tell the ignoreSelectors was also created with css modules too much in mind.
It acts like a hole punch. The AST node that matches doesn't contribute to specificity, but its child nodes do. Which works really well for :global and :local because they happen to be removed during compilation.

But I am not sure if this makes sense for any case other than css modules.
There isn't any test coverage for other pseudo selectors with child nodes, so I can't tell what the exact intention was here.

---

Proposal :

• fix the tests so that :global and :local are used as supported by implementations
• better define how ignoreSelectors works when it matches AST nodes with child nodes

[ybiquitous]

@romainmenke Thanks for the report. ignoreSelectors seemed to be added by PR #2874, quite a while ago (2017)! I'm honestly unsure about the background, but it seems a good opportunity to reconsider CSS Modules selectors (e.g. :global). 👍🏼

In addition, thanks for trying @csstools/selector-resolve-nested with PR #7496!

[Mouvedia]

It has been decided at one point that we wouldn't care about CSS modules selectors.
Nonetheless I think we shouldn't introduce regressions if it can be avoided.

[romainmenke]

Regressions or CSS modules compat are not really my primary concern here :)

CSS modules are supported in @csstools/selector-specificity so there is nothing that needs to happen for that in Stylelint when calculating specificity.

But the tests contain incorrect CSS for CSS modules.
This is just bogus :

:global(.page, .bar)
:global(.page, :local(.bar))

Because these are bogus we can safely assume that no user will be writing this and that changes won't cause regressions.

---

At the same time these test are the only coverage we have for ignoreSelectors in combination with functional pseudo classes.

There is no coverage with standard functional pseudo classes and ignoreSelectors.

I don't know what the calculated specificity should be for these :

• ignoreSelectors -> :is

.foo :is(#bar, input) {} // [1, 1, 0] without ignore

Is that :

• [0, 1, 0] -> :is and all children are ignored
• [1, 1, 0] -> only :is is ignored, but not its children

:is already doesn't have any specificity of its own, it is as specific as its most specific argument.

The same is true for :not, :has, ...

• is there a use case for ignoring only a functional pseudo class but not its children?
• how do other ignore flags work? do they ignore the entire AST subtree?

---

This is different from non-functional pseudo classes.

.foo :focus {} // [0, 2, 0] without ignore

Ignoring :focus does meaningfully affect specificity -> [0, 1, 0]

[ybiquitous]

I agree that we should correct the current weird behavior of ignoreSelectors. But as @Mouvedia is concerned, I think we should ensure the compatibility until the next major version, since this option was added a long time ago.

[ybiquitous]

Probably, I guess the following behavior would be correct: ⬇️

---

Given:

{"ignoreSelectors": [":global"]}

Then:

:global {} /* ignored */
:global(.foo) {} /* not ignored */

---

Given:

{"ignoreSelectors": ["/:global\\(.+\\)/"]}

Then:

:global {} /* not ignored */
:global(.foo) {} /* ignored */

[romainmenke]

Would it be ok to remove the bogus tests and to replace them with tests for standard functional pseudo classes?

Then at least there is coverage for the current implementation in a way that makes it clear what the expectations are.

[Mouvedia]

<talking about :global and :local>
At the same time these test are the only coverage we have for ignoreSelectors in combination with functional pseudo classes.

If I had to guess it was to cover a real use case; this seems reasonable to me.

better define how ignoreSelectors works when it matches AST nodes with child nodes

If that's a documentation update please check #4105 as well.

:global and :local do not support selector lists in implementations
[…]
fix the tests so that :global and :local are used as supported by implementations

If true, 👍

[jeddy3]

Our end goal is to modernise and make our behaviour around specificity and nesting spec-compliant, which will benefit the majority of our users.

As such, I think we can remove barriers that'll hamper us in achieving this for our next major release, including removing options that may no longer be relevant.

I suggest we remove the ignoreSelectors option (and related tests) as part of #7496. It may well be that everyone's use case is covered by:

:global and :local don't have any specificity of their own in @csstools/selector-specificity

If not, a person can open a new issue with their use case, and we'll decide on the most modern way to cater to it.

how do other ignore flags work? do they ignore the entire AST subtree?

We've typically avoided broad ignore options like ignoreSelectors because of these types of problems. We've had better success with more focused ignore options like:

• ignorePseudoClasses
• ignoreContextFunctionalPseudoClasses

We can reach for similar options to cater to specific use cases.

[ybiquitous]

I agree with replacing ignoreSelectors with more specific options. 👍🏼

Perhaps, I assume that CSS Modules selectors like :global or :local might be used in most use cases of ignoreSelectors. As we can regard them as functional pseudo classes, providing ignoreFunctionalPseudoClasses instead of ignoreSelectors makes sense to me in order to keep compatibility as much as possible.

E.g.

{
  "selector-max-specificity": ["0,2,0", {"ignoreFunctionalPseudoClasses": [":global", ":local"]}]
}

.foo .bar.baz {} /* '0,3,0' => reported */

:global(.foo) .bar.baz {} /* '0,2,0' => not reported */

So, I suggest the following steps:

1. Deprecate ignoreSelectors with the next minor version (16.x.0)
2. Add ignoreFunctionalPseudoClasses with the next minor version (16.x.0)
3. Remove ignoreSelectors with the next major version (17.0.0)

[Mouvedia]

So, I suggest the following steps:

1. Deprecate ignoreSelectors with the next minor version (16.x.0)
2. Add ignoreFunctionalPseudoClasses with the next minor version (16.x.0)
3. Remove ignoreSelectors with the next major version (17.0.0)

Sounds reasonable.

In the meantime

fix the tests so that :global and :local are used as supported by implementations

shouldn't hinder that plan.
@romainmenke if you are planning to fix it yourself, either emend, remove or replace the 6 tests that are affected. We can discuss the details during the review.

---

The documentation update could be skipped since we are planning to replace the option.