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

Prevent simplifying translate: none; and scale: none; #703

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RobinMalfait
Copy link
Contributor

@RobinMalfait RobinMalfait commented Mar 20, 2024

This PR fixes a subtle bug where translate: none; was simplified to translate: 0;. Similarly scale: none; was simplified to scale: 1;.

These simplifications are unsafe because translate: 0; and scale: 1; create a stacking context where translate: none; and scale: none; do not.

The implementation currently changes the Translate and Scale structs to an enum with an explicit None variant for the "none" case, and an explicit XYZ variant for the x, y, and z values.

I considerd adding a more generic Keyword variant to the Translate and Scale enums, but I think that would be a bit overkill for this specific use case. Either way, I'm open to suggestions for the chosen APIs.

Here is a jsfiddle playground (https://jsfiddle.net/2krswu8e/) to show the issue. Notice how the red boxes 5 and 9 are on top of the blue boxes. This is not the case for the safe translate: none; and scale: none; values.

pub z: Length,
pub enum Translate {
/// The "none" keyword.
None,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also use Keyword(&'a str) but not sure if it's worth it to do that honestly especially since keywords could all be known upfront and the 'none' value is the only one we care about right now.

None,

/// The x, y, and z translations.
XYZ {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use a better name, but I don't hate it. I also noticed there are some structs like this in the colors.rs file. E.g.: XYZd50

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine. Could you run yarn build-ast to re-generate the typescript types? Technically I guess this would be a breaking AST change... Perhaps there is a way to avoid that, but lets see what it generates first.

@RobinMalfait
Copy link
Contributor Author

Will try to do that soon, currently running into an issue that I would have to debug to know what's going on. Just sharing it here in case you've seen this issue before:

image

@mischnic
Copy link
Member

Maybe you haven't run yarn in a while? For me yarn build-ast works fine with your branch

@RobinMalfait
Copy link
Contributor Author

Hmm I did run yarn. Can you share which version of yarn and which version of node you are using?

@mischnic
Copy link
Member

Yarn 1.22.21
Node 20.11.1
macOS 13.6.1

@RobinMalfait
Copy link
Contributor Author

Hmm, thanks! Will keep debugging.

image

@RobinMalfait
Copy link
Contributor Author

Not sure what, but there is something wrong when trying to yarn install (even after completely removing node_modules and running yarn cache clean. Eventually used bun install (nothing committed about that) which figured out the correct dependencies and then I could run the script.

Funky.

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.

None yet

3 participants