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

Redundant css vendor prefix for letter-spacing in v3 #223

Closed
aszx87410 opened this issue May 29, 2020 · 5 comments
Closed

Redundant css vendor prefix for letter-spacing in v3 #223

aszx87410 opened this issue May 29, 2020 · 5 comments

Comments

@aszx87410
Copy link

Reproduce

  1. Open this codesandbox project: https://codesandbox.io/s/amazing-ramanujan-doked?file=/src/index.js
  2. See the screen and you will found letter-spacing with vendor prefix which is unnecessary.

螢幕快照 2020-05-30 上午12 16 57

version: 3.5.4

code:

import stylis from "stylis";

document.getElementById("app").innerHTML = `
${stylis("#id", "letter-spacing: 1px;")}
`;

Explanation

By rights, letter-spacing need no vendor prefix, so it's a bug, and it's because of hash collision!

In v3, the hash function is quite simple: var hash = (first*2) + (second*3) + (third*4), so for the string letter-spcing, the result is 983, which happen to have the same value as user-select.

https://github.com/thysultan/stylis.js/blob/v3.5.4/stylis.js#L1079

So letter-spacing has been wrongly transformed and add vendor prefix.

This problem is resolve in v4 because of the stronger hash function, I still submit this issue because both @emotion and styled-component uses stylis under the hood, and they have the same issue.

I already opened an issue for styled-component: styled-components/styled-components#3157. For emotion I am aware that you are working on upgrade stylis to v4 so I didn't submit an issue for it, and for emotion it's fine, because it's planning to upgrade to v4 with your help. But for styled component it's a different story because it depends on @emotion/stylis.

I am not sure what is the best way to solve this issue for other projects like styled-component and emotion which depends on stylis, ask them to upgrade may make a breaking change for them as well and may take some times, so maybe fix this issue and publish 3.5.5 is a better solution?

Anyway, I searched for keyword letter-spacing and find nothing so I submit this issue and want to bring this to you. Please let me know if there is anything unclear, thanks!

@thysultan
Copy link
Owner

Does this actually break the css on the page?

@aszx87410
Copy link
Author

Nope, add unnecessary vendor prefix won't break CSS on the page, it's totally fine. But there are two other disadvantages I can think of.

First, it makes CSS bundle size bigger, but after gzipped maybe it's negligible.

The second one is, it may makes people confusing. When I saw this prefix I thought "wow, I don't know letter-spacing also needs vendor prefix, I learned a new thing today", but later on I found that it's just a bug and it's not needed to add any prefix to letter-spacing.

@thysultan
Copy link
Owner

3.5.4 was published 2 years ago so i doubt this is a problem in practice. You can lobby styled-components to upgrade. It can be a non-breaking upgrade if they didn't directly expose the previous styleis plugin system.

@aszx87410
Copy link
Author

Got it, thanks for the promptly response, I am going to close the issue then.

@Andarist
Copy link
Collaborator

Just a note - they have exposed the plugin system in their latest major version so it would be a breaking change for them.

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

No branches or pull requests

3 participants