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 rule-selector-property-disallowed-list false positives for nesting selectors #7488

Closed
emmacharp opened this issue Jan 21, 2024 · 13 comments · Fixed by #7558
Closed

Fix rule-selector-property-disallowed-list false positives for nesting selectors #7488

emmacharp opened this issue Jan 21, 2024 · 13 comments · Fixed by #7558
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule

Comments

@emmacharp
Copy link
Contributor

emmacharp commented Jan 21, 2024

What minimal example or steps are needed to reproduce the bug?

.search {
   & input {
      width: 100%;
   }
}

What minimal configuration is needed to reproduce the bug?

{ 
  "rules": { 
    "rule-selector-property-disallowed-list": [{ "/^((?!input).)*$/": "width" }]
  }
}

How did you run Stylelint?

stylelint *.css

Online demo

Which Stylelint-related dependencies are you using?

{
   "stylelint": "16.2.0"
}

What did you expect to happen?

The nested block should not generate an error.

What actually happened?

.search is evaluated and an error is thrown.

Do you have a proposal to fix the bug?

Don't evaluate nested selectors... though I'm not sure about collateral damage.

@ybiquitous
Copy link
Member

@emmacharp Thanks for the report with a reproduction. To clarify, do you want to disallow a width prop within all input selectors?

I created the online demo, which doesn't seem to be a problem. See the config:

{
  "rules": {
    "rule-selector-property-disallowed-list": [{ "input": "width" }]
  }
}

@ybiquitous ybiquitous added the status: needs clarification triage needs clarification from author label Jan 22, 2024
@emmacharp
Copy link
Contributor Author

The contrary, actually. The example is about disallowing the width property within any selector not containing input. But since the input selector is nested inside a .search selector, an error results.

My regex in the example was bunk, sorry. If you try your demo with this one instead, you should see what I mean:
/^((?!input).)*$/

As always, thanks for the help!

@ybiquitous
Copy link
Member

ybiquitous commented Jan 22, 2024

Ah, I understand it now. See the new demo.

.search {
  & input {
    width: 100%; /* should be ok, but actually reported */
 /* ^^^^^ Unexpected property "width" for selector ".search" */
  }
}

.search input {
  width: 100%; /* not reported, ok */
}

.search form {
  width: 100%; /* reported, ok */
/*^^^^^ Unexpected property "width" for selector ".search form" */
}
{
  "rules": {
    "rule-selector-property-disallowed-list": [{ "/^((?!input).)*$/": "width" }]
  }
}

This should be a bug of rule-selector-property-disallowed-list for nested rules.

I've labeled the issue as ready to implement. Please consider contributing if you have time.

There are steps on how to fix a bug in a rule in the Developer guide.

@ybiquitous ybiquitous added status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule and removed status: needs clarification triage needs clarification from author labels Jan 22, 2024
@ybiquitous ybiquitous changed the title rule-selector-property-disallowed-list considers nested blocks Fix rule-selector-property-disallowed-list false positives for nesting selectors Jan 22, 2024
@ybiquitous
Copy link
Member

I've updated this issue body to include the correct reproduction and the online demo.

@emmacharp
Copy link
Contributor Author

Thanks for rephrasing in a standard way. I'll see what I can do today & tomorrow.

@ybiquitous
Copy link
Member

We can use postcss-resolve-nested-selector to resolve a nested selector, which is installed already.

Otherwise, we may be able to install and use @csstools/selector-resolve-nested instead. The package follows the CSS Nesting spec, while postcss-resolve-nested-selector doesn't.

For details, see #6234 (comment)

@emmacharp
Copy link
Contributor Author

Well...

I have never worked with Postcss before and the truth is I'm more a designer than a programmer. Heh. So figuring out what's going on is proving a bit challenging.

The way I understand it, we need to prevent walking through child nodes' rules. I'm not sure how the plugins mentioned above can help doing that. Am i missing something?

Not quite sure about collateral damage either. Do you all think about something that could be broken by preventing the rule to walk through child nodes in the AST?

@ybiquitous
Copy link
Member

Ah, I see. Don't mind. Adding @csstools/selector-resolve-nested was started with PR #7496, so let's wait for the job. This issue should be addressed more easily if the job is done.

Copy link
Contributor

This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant.

@github-actions github-actions bot added status: ask to implement ask before implementing as may no longer be relevant and removed status: ready to implement is ready to be worked on by someone labels Feb 26, 2024
@emmacharp
Copy link
Contributor Author

@ybiquitous, do we still wait PR #7496 to be closed to progress here? If not, I'd try to see what I can do for it to be included in 16.3.0

Thanks!

@ybiquitous
Copy link
Member

In this issue, I favor installing @csstools/selector-resolve-nested because #7496 tries replacing postcss-resolve-nested-selector with @csstools/selector-resolve-nested but rule-selector-property-disallowed-list doesn't rely on postcss-resolve-nested-selector. There is no breaking change when using @csstools/selector-resolve-nested.

CC: @romainmenke if you notice something, feel free to comment.

If not, I'd try to see what I can do for it to be included in 16.3.0

Honestly, I'd like to release 16.3.0 as soon as possible because the version will include some bug fixes.

So, you don't need to rush, we can release 16.3.1 soon when resolving this issue.

@ybiquitous ybiquitous added status: ready to implement is ready to be worked on by someone and removed status: ask to implement ask before implementing as may no longer be relevant labels Mar 12, 2024
@romainmenke
Copy link
Member

I think that actual issue might simpler to resolve :)
But maybe not.

I'll open a PR with a proposed fix.

@romainmenke
Copy link
Member

It is already quite late here, so I will pick this up again tomorrow.
But if anyone has thoughts : #7558

@Mouvedia Mouvedia mentioned this issue Mar 13, 2024
6 tasks
@ybiquitous ybiquitous added status: wip is being worked on by someone and removed status: ready to implement is ready to be worked on by someone labels Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule
Development

Successfully merging a pull request may close this issue.

3 participants