search

postcss / autoprefixer

Parse CSS and add vendor prefixes to rules by Can I Use
https://twitter.com/autoprefixer
MIT License
21.66k stars 1.26k forks source link

Remove text-decoration hack #1474

Closed h3rmanj closed 2 years ago

h3rmanj commented 2 years ago

Fixes #1473

romainmenke commented 2 years ago

For this to work as expected autoprefixer needs to add -webkit-text-decoration when the browerslist includes a Safari version that doesn't support test-decoration as a shorthand. (at the moment no Safari version supports this)

From the changes alone I could not determine if this was the case.

Testing shows that only Safari <= 12 currently receives prefixes for text-decoration. To fully fix the issue another change is needed to track : https://caniuse.com/mdn-css_properties_text-decoration_shorthand

h3rmanj commented 2 years ago

This PR won't change the behavior of shorthand text-decoration, it will still prefix it. The hack was there to not prefix basic values however, which is causing issues with safari. I removed it so that all values of text-decoration will be prefixed. See the issue for more context.

romainmenke commented 2 years ago

This PR won't change the behavior of shorthand text-decoration, it will still prefix it.

I think it needs to change to get the intended effect. The fix in this pull request is currently only applied if your browserslist is targeting Safari 12 or older.

However text-decoration should be prefixed in all Safari versions to fully resolve the case described in the linked issue.

see my comment : https://github.com/postcss/autoprefixer/issues/1473#issuecomment-1243370592

h3rmanj commented 2 years ago

I'm not sure I follow, I think that would be a separate issue (probably in browserlist?), as this PR only looks to prefix all values of text-decoration.

romainmenke commented 2 years ago

Two things are need to resolve the linked issue :

Maybe best to update the PR comment so that merging this PR doesn't resolve the linked issue?

romainmenke commented 2 years ago

@h3rmanj https://github.com/postcss/autoprefixer/pull/1478 was merged but I was unable to avoid conflicts with your changes. Hopefully not to much hassle to resolve these 🤞

h3rmanj commented 2 years ago

I could take a look at it tomorrow if ai wants this fix, but it doesn't seem so 🤷

ai commented 2 years ago

Yes, I really afraid to change old prefixes. I afraid that with the this fix we will break another cases.

h3rmanj commented 2 years ago

I see. The bug still remains though. Maybe we could find the version of safari or Mozilla you used when implementing it, to see if that gives us a hint as to why you didn't prefix single values?

romainmenke commented 2 years ago

@ai Is your concern that people might depend on the current behaviour or that it is difficult to validate this change?

If it is the former I agree that this could be a breaking change for some users of autoprefixer.

ai commented 2 years ago

My concerns go from:

  1. Yes, “people might depend on the current behavior”
  2. That we created a fix based on one use case rather than analysis of many possible cases
  3. We have a few properties like text-decoration. We should have the common policy for all of them.
h3rmanj commented 2 years ago

I'm probably blind to the other cases from my point of view, so it's probably a breaking change. I still consider this the correct fix though, as if Safari were to remove the prefix, that would leave the same behavior as this PR introduces.

But I'm still lost as to why the check is there in the first place. text-decoration as a shorthand still supports a single basic value as @romainmenke pointed out in the issue.

I'm removing my interest in this issue as we've had to include a rule to never use text-decoration in our stylesheet, since autofixer would break it, and waiting for a major release wasn't an option.