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

Browser/Spec bug: Failing test for capturing css shorthand with confounding variable #1322

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eoghanmurray
Copy link
Contributor

This PR contains a new test which shows the failing behaviour;
I can't currently see a way around this, so this is a bug report and maybe a place to link to any bugs in browser version trackers.

#1268 PR (as reported in #1246) is very similar however this version occurs while serializing the static stylesheet, rather than as part of a mutation (where we can work around it)

Problem:

#one {
	animation: var(--animation-fade-in);
	animation-delay: 0.2s;
}

in a stylesheet gets serialized as

#one {
  animation-duration: ; 
  animation-timing-function: ; 
  animation-iteration-count: ; 
  animation-direction: ; 
  animation-fill-mode: ; 
  animation-play-state: ; 
  animation-name: ; 
  animation-timeline: ; 
  animation-range-start: ; 
  animation-range-end: ; 
  animation-delay: 0.2s; 
}

I presume a similar thing might happen for e.g. background: var(--mycolor); background-image: url('x.png');

@changeset-bot
Copy link

changeset-bot bot commented Oct 10, 2023

⚠️ No Changeset found

Latest commit: 2d21d1e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@eoghanmurray eoghanmurray changed the title Browser bug? Failing test for capturing css shorthand with confounding variable Browser/Spec bug: Failing test for capturing css shorthand with confounding variable Oct 10, 2023
@eoghanmurray
Copy link
Contributor Author

I think this is the relevant spec issue:

w3c/csswg-drafts#2515

@eoghanmurray
Copy link
Contributor Author

Similarly the following would exhibit the problem:

.gradient {
  background: rgb(var(--color-background));
  background: var(--gradient-background);
  background-attachment: fixed;
}

@eoghanmurray
Copy link
Contributor Author

eoghanmurray commented Jun 4, 2024

There is a possibility to fix this when we have access to the raw .innerText of the style;

  • we could construct an artificial Stylesheet using CSSStyleSheet
  • then compare the rules with the derived rules from .sheet

If they are the same, that means we can trust the .innerText as being up-to-date (not mutated by .insertRule etc)
In that case we just use the .innerText as the serialization.

For external stylesheets (<link> elements) we can fetch the raw contents and do the comparison that way (fetch should be from cache).

This will all be possible in an async manner after #1475

Hat-tip: CSSStylesheet idea was from https://github.com/rrweb-io/rrweb/pull/1357/files#diff-c61c2c2a3a84e2ee30a603d8363a994acce4caef8827931eb5b7317f08b53cb1R588

@Juice10
Copy link
Contributor

Juice10 commented Jul 9, 2024

This also happens to padding and margin shorthands.
More info: https://issues.chromium.org/issues/40771479#comment13
And for both comedy and even more information, you get to see me getting yelled at here: https://issues.chromium.org/issues/40771479#comment14

Edit: Looks like part of this is fixed, the rest is related to w3c/csswg-drafts#2515 as mentioned above.

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.

2 participants