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

Remove text-decoration hack #1474

Closed
wants to merge 2 commits into from
Closed

Remove text-decoration hack #1474

wants to merge 2 commits into from

Conversation

h3rmanj
Copy link

@h3rmanj h3rmanj commented Sep 7, 2022

Fixes #1473

@romainmenke
Copy link
Contributor

romainmenke commented Sep 12, 2022

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
Copy link
Author

h3rmanj commented Sep 12, 2022

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
Copy link
Contributor

romainmenke commented Sep 12, 2022

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 : #1473 (comment)

@h3rmanj
Copy link
Author

h3rmanj commented Sep 12, 2022

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
Copy link
Contributor

Two things are need to resolve the linked issue :

  • remove the hack
  • add the right browser support data for text-decoration as a shorthand

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

@romainmenke
Copy link
Contributor

@h3rmanj #1478 was merged but I was unable to avoid conflicts with your changes. Hopefully not to much hassle to resolve these 🤞

@ai ai closed this Sep 14, 2022
@h3rmanj
Copy link
Author

h3rmanj commented Sep 14, 2022

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

@ai
Copy link
Member

ai commented Sep 14, 2022

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

@h3rmanj
Copy link
Author

h3rmanj commented Sep 14, 2022

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
Copy link
Contributor

romainmenke commented Sep 14, 2022

@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
Copy link
Member

ai commented Sep 14, 2022

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
Copy link
Author

h3rmanj commented Sep 14, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

text-decoration prefixing inconsistent
3 participants