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

Rejected -webkit-background-clip with text value #7294

Closed
brunodccarvalho opened this issue Nov 3, 2023 · 11 comments
Closed

Rejected -webkit-background-clip with text value #7294

brunodccarvalho opened this issue Nov 3, 2023 · 11 comments
Labels
status: needs discussion triage needs further discussion

Comments

@brunodccarvalho
Copy link

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

.gradient {
  -webkit-background-clip: text;
  background-clip: text;
  -webkit-text-fill-color: transparent;
  -moz-text-fill-color: transparent;
}

What minimal configuration is needed to reproduce the bug?

{
  "defaultSeverity": "error",
  "rules": { "property-no-vendor-prefix": true }
}

How did you run Stylelint?

npx stylelint --config=.stylelintrc.json test.css

Which Stylelint-related dependencies are you using?

15.11.0

What did you expect to happen?

Since the rule does not delete the prefixed text-fill-color properties I expect no problems to be reported as -webkit-background-clip: text also seems to have very widespread support.

postcss/autoprefixer has a 5 year old test to accept this here, see background-clip.css

What actually happened?

test.css
 2:3  ✖  Unexpected vendor-prefix "-webkit-background-clip"  property-no-vendor-prefix

1 problem (1 error, 0 warnings)

and it is autocorrectable, so it gets replaced with background-clip: text if --fix is given

Do you have a proposal to fix the bug?

No response

@brunodccarvalho
Copy link
Author

Apologies, I should clarify that the unprefixed property is not equivalent as background-clip: text does not currently work on chrome or edge, as shown on the caniuse page.

@Mouvedia Mouvedia added the status: needs discussion triage needs further discussion label Nov 3, 2023
@Mouvedia
Copy link
Member

Mouvedia commented Nov 3, 2023

Why aren't you using ignoreProperties?

@brunodccarvalho
Copy link
Author

I am expecting stylelint to not transform correct to incorrect code silently, especially for css constructs and features that have essentially complete browser support. If it reports an error and refuses to fix the property I would consider that fine. Is this an unreasonable expectation?

@Mouvedia
Copy link
Member

Mouvedia commented Nov 3, 2023

I am expecting stylelint to not transform correct to incorrect code silently

Hypothetically if we were targeting only Firefox 49–121, Edge 15–18 and Safari 14–17.2—which all support background-clip: text;—wouldn't the error be regarded as expected and desirable?
It seems that you are taking a subjective take on this; the rule doesn't discriminate based on browser support.
IMHO using this rule without exceptions spells trouble: it has to be constantly maintained.

@brunodccarvalho
Copy link
Author

The rule is enabled by default in stylelint-config-standard with no exceptions.

When it comes to the css, as you can see in the caniuse page, the -webkit-background-clip name for this property value has been supported for a decade, the webkit browsers seem to have no interest in migrating to background-clip, and the non-webkit browsers support the -webkit- prefix. Browser targeting is largely irrelevant in this case when the unprefixed version has 20% support, the prefixed one has 95%, the latter is a superset of the former, and there seems to be no interest in changing this. The prefixed version is de facto standard.

Looking around in lib/utils/isAutoprefixable.js, #5312, #4971 and postcss/autoprefixer/lib/hacks, this looks like it was missed in #5312.

In isAutoprefixable.js a manual whitelist is used to list target properties, rather than a blacklist or something dynamic. This suggests the rule is meant to be conservative and safe, and remove only historically superfluous prefixes like -moz-columns and -webkit-transform. With this implementation approach I expect the user should not need to worry with this rule breaking conforming up-to-date css. I think stylelint-config-standard would agree.

Scanning postcss/autoprefixer/lib/hacks I found similar hacks for mask-border, which seems to be handled correctly here, and another for background-size, which seems to be handled unsafely according to wg-compat#28. I suggest removing it from the whitelist.

In #5312 -ms-interpolation-mode is a "positive" exception, we should do the same here, this is a "negative" exception, we'll need to know the value in the declaration like the autoprefixer hack does. I'll submit the short PR in a minute.

@ybiquitous
Copy link
Member

@brunodccarvalho Thanks for the explanation. I understand your motivation well. But I think stylelint-config-standard should handle this case, instead of the Stylelint core logic. Reasons:

Any thoughts?

@Mouvedia
Copy link
Member

Mouvedia commented Nov 4, 2023

But I think stylelint-config-standard should handle this case, instead of the Stylelint core logic.
Creating a breaking change […] is much easier in stylelint-config-standard than stylelint.

I think that's the right course of action. @brunodccarvalho were you using the standard config?
If so this issue can be transferred to stylelint/stylelint-config-standard.

@brunodccarvalho
Copy link
Author

background-clip: text is not currently supported in the CSS Background Level 3 spec.

That you present this as a serious argument demonstrates the project targets the idealized web rather than the de facto web, which I do not consider reasonable for a tool like this. I'll add my exceptions to background-clip and background-size internally and end my contribution to this matter here.

@Mouvedia
Copy link
Member

Mouvedia commented Nov 4, 2023

@ybiquitous I think we should propose to add exceptions here.

@brunodccarvalho thanks for the feedback but stylelint/stylelint is just not the right place for this change.
The existing exception is a bad precedent IMHO but it's too late to revert now.

@Mouvedia Mouvedia closed this as completed Nov 4, 2023
@ybiquitous
Copy link
Member

I think we should propose to add exceptions here.

Agree. Feel free to open a Pull Request.

The existing exception is a bad precedent IMHO but it's too late to revert now.

Yes, we should not have added this exception.

@Mouvedia
Copy link
Member

-webkit-background-clip is part of the legacy name aliases list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs discussion triage needs further discussion
Development

Successfully merging a pull request may close this issue.

3 participants