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 selector-pseudo-*-allowed-list false positives for vendor prefixes #7524

Closed
carlosjeurissen opened this issue Feb 17, 2024 · 7 comments · Fixed by #7525
Closed

Fix selector-pseudo-*-allowed-list false positives for vendor prefixes #7524

carlosjeurissen opened this issue Feb 17, 2024 · 7 comments · Fixed by #7525
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule

Comments

@carlosjeurissen
Copy link
Contributor

carlosjeurissen commented Feb 17, 2024

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

button::-moz-focus-inner {
  display: none;
}

What minimal configuration is needed to reproduce the bug?

{
  "rules": {
    "selector-pseudo-element-allowed-list": ["-moz-focus-inner"]
  }
}

How did you run Stylelint?

CLI

Which Stylelint-related dependencies are you using?

{
  "stylelint": "16.2.1"
}

What did you expect to happen?

For it to pass without any errors.

What actually happened?

It rejects the pseudo element since "focus-inner" is not in the allowed-list.

Do you have a proposal to fix the bug?

Yes: #7525

@carlosjeurissen
Copy link
Contributor Author

In general one might wonder why we even ignore the vendor prefix in the first place.
If "something" is in the allowed-list, why should "-webkit-something" be accepted?

@ybiquitous ybiquitous added status: wip is being worked on by someone type: bug a problem with a feature or rule labels Feb 19, 2024
@ybiquitous
Copy link
Member

@carlosjeurissen Thanks for the report and pull request. Right, this is a bug.

We also can see the reproducible demo:

button:-moz-focus-inner,
button::-moz-focus-inner {
  display: none;
}
Unexpected pseudo-class ":-moz-focus-inner" (selector-pseudo-class-allowed-list)
Unexpected pseudo-element "::-moz-focus-inner" (selector-pseudo-element-allowed-list)

@ybiquitous ybiquitous changed the title Vendor prefixes in pseudo allowed-list are currently a noop Fix selector-pseudo-*-allowed-list false positives for vendor prefixed values Feb 19, 2024
@ybiquitous ybiquitous changed the title Fix selector-pseudo-*-allowed-list false positives for vendor prefixed values Fix selector-pseudo-*-allowed-list false positives for vendor prefixes Feb 19, 2024
@carlosjeurissen
Copy link
Contributor Author

@ybiquitous Thanks for merging! What are your thoughts on accepting / rejecting vendor prefixes when they are not included in the allowlist?

Currently, when just "box-shadow" is specified in the allowlist, "-webkit-box-shadow" will be allowed as well. Which seems odd.

Do you feel it makes sense to change this? Alternatively we can add an option to the pseudo-allowed-list rules named "allowVendorPrefixes", which would be true by default. I can make a PR for this.

@ybiquitous
Copy link
Member

Currently, when just "box-shadow" is specified in the allowlist, "-webkit-box-shadow" will be allowed as well. Which seems odd.

This is a demo. I feel that makes sense because we usually use a vendor prefix with a non-prefixed value.

Is there a case when you want to exclude only a vendor prefix?

@carlosjeurissen
Copy link
Contributor Author

carlosjeurissen commented Feb 23, 2024

@ybiquitous Personally I want to explicitly allowlist specific vendor-prefixes when it comes to pseudo classes and elements as the vendor prefixes often do not have a non-prefixed version or the non prefixed version works differently than the prefixed version.

What I hear is you like the default to stay as it is. Would you have any objections in a PR which adds an option allowing explicit specifications of vendor prefixes?

So that:

"selector-pseudo-class-allowed-list": [[
  "hover", "-moz-last-node"
], {
  "includeVendorPrefixed": false
}]

Would not allow ":-moz-hover" or ":last-node" but would allow ""-moz-last-node" and "hover"

@ybiquitous
Copy link
Member

@carlosjeurissen Although you might want to use the selector-no-vendor-prefix rule for your use case, explicitly including vendor prefixes seems to make sense for me.

Probably, I feel like it would be better not to include vendor prefixes by default (without any options), but this idea leads to breaking the rule's backward compatibility.

Anyway, can you open a new issue about this topic? Let's discuss it there.

@carlosjeurissen
Copy link
Contributor Author

@ybiquitous Sure. That makes a lot of sense. Created a new issue:
#7542

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.

2 participants