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

Change selector-pseudo-*-allowed-list not to include vendor prefixes automatically #7542

Open
Tracked by #7396
carlosjeurissen opened this issue Feb 26, 2024 · 9 comments
Labels
status: agreed but held is agreed on but implementation is held type: bug a problem with a feature or rule
Milestone

Comments

@carlosjeurissen
Copy link
Contributor

carlosjeurissen commented Feb 26, 2024

What is the problem you're trying to solve?

Currently, when just hover is specified in the allowlist, -webkit-hover will be allowed as well even tho :-webkit-hover is invalid. This is pretty unexpected for pseudo classes / elements. Considering:

  • most of them do not have vendor prefixes
  • you can not join them in one selector as this could invalidate the whole selector
  • the vendor-prefix is named differently (:-moz-focusring, :-webkit-full-screen)

What solution would you like to see?

It would make more sense to explicitly add desired vendor prefixed pseudo elements/classes to the allowlist.

This can potentially be a breaking change. However:

  • usage of allowlists in pseudo elements / classes seems rare
  • rarely an equivalent vendor prefix ever existed (I could only found any-link, for which one would use :visited and :link as fallback)
  • this can be specified in the stylelint changelog
  • migration is fairly easy
  • no breaking issues will occur, just a false positive
@ybiquitous ybiquitous added the status: needs discussion triage needs further discussion label Feb 26, 2024
@ybiquitous ybiquitous changed the title "selector-pseudo-*-allowed-list" automatically allows vendor prefixed equivalents. selector-pseudo-*-allowed-list automatically allows vendor prefixed equivalents Feb 26, 2024
@ybiquitous
Copy link
Member

@carlosjeurissen Thanks for opening the discussion. The suggestion seems to make sense to me. Vendor prefixes have been gradually less used, and people can easily migrate their config to keep the current behavior if they want. E.g.

{
  "selector-pseudo-class-allowed-list": [/^(-webkit-|-moz-)?hover$/]
}

But indeed, this can be a breaking change, even if it is a false positive. So, I think it's better to change in the next major release.

@ybiquitous
Copy link
Member

Ref

## Major release
Likely to break your lint build:
- a change in the documented behavior of an existing rule results in Stylelint reporting more errors by default

@carlosjeurissen
Copy link
Contributor Author

carlosjeurissen commented Feb 26, 2024

@ybiquitous if we want to be extra careful we can indeed wait until a major release for this change to be implemented. However I do not see a single use-case of automatically allow-listing vendor prefixes. If you have one please let me know! Even the any-link example makes no sense as their is :link and :visited as fallbacks.

If we really want to wait, it could make sense to offer an option to the pseudo-allowed-list rules named "allowVendorPrefixes", which would be true by default until a major release.

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

The selector-pseudo-class-whitelist rule (before selector-pseudo-class-allowed-list) was added in 2c4bfa9 and published in https://github.com/stylelint/stylelint/releases/tag/7.1.0. That was 2016, 8 years ago. The rule initially supported vendor prefixes. Probably, I imagine including vendor prefixes by default was more important at that time.

if (matchesStringOrRegExp(vendor.unprefixed(name).toLowerCase(), whitelist)) { return }

I also don't have a use case, but I'm still concerned about the compatibility. Even if adding an option like includeVendorPrefixed, the option would become useless in the next major version, right?

@Mouvedia
Copy link
Contributor

If we are going the option way, I would prefer something like strict: true or lax: false because

or ":last-node"

wouldn't be expected to not be permitted for

"includeVendorPrefixed": false

You can't change the statu quo default boolean value before a major release though.

@carlosjeurissen
Copy link
Contributor Author

or ":last-node" wouldn't be expected to not be permitted for

@Mouvedia This would be expected if the allowlist only contains "-moz-last-node".

@ybiquitous Fair. The option in a next version would indeed be pretty useless unless people like it as a feature. Yet again I do not see a use case for this. Lets be safe and wait for a next major version and skip having an option for this.

Let me know if I can provide a PR and what the process for a delayed PR would be.

@ybiquitous ybiquitous added status: agreed but held is agreed on but implementation is held type: bug a problem with a feature or rule and removed status: needs discussion triage needs further discussion labels Feb 27, 2024
@ybiquitous ybiquitous changed the title selector-pseudo-*-allowed-list automatically allows vendor prefixed equivalents Change selector-pseudo-*-allowed-list not to include vendor prefixes automatically Feb 28, 2024
@ybiquitous ybiquitous mentioned this issue Feb 28, 2024
5 tasks
@ybiquitous
Copy link
Member

I added this issue to the list in #7396, preparing 17.0.0.

@Mouvedia Mouvedia added this to the future-major milestone Feb 28, 2024
@Mouvedia
Copy link
Contributor

Mouvedia commented Apr 18, 2024

related: #7624
i.e. to ignore -webkit-box you gotta add box instead

@Mouvedia
Copy link
Contributor

Mouvedia commented Apr 26, 2024

see also

…and add option noVendorIgnore

checkPrefixed

i.e. the name chosen will have to be consistent for all affected rules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: agreed but held is agreed on but implementation is held type: bug a problem with a feature or rule
Development

No branches or pull requests

3 participants