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

[css-variables][cssom] Empty value doesn't round-trip #9847

Open
Loirooriol opened this issue Jan 24, 2024 · 6 comments
Open

[css-variables][cssom] Empty value doesn't round-trip #9847

Loirooriol opened this issue Jan 24, 2024 · 6 comments

Comments

@Loirooriol
Copy link
Contributor

<!DOCTYPE html>
<div style="--a: ; width: var(--a) 100px; height: 100px; background: cyan;"></div>
<script>
var el = document.querySelector("div");
el.style.setProperty("--a", getComputedStyle(el).getPropertyValue("--a"));
</script>

I would expect the JS to be no-op as per the round-tripping principle.

However, an empty string has a special meaning in setProperty: it behaves as removeProperty, even though it's valid for custom properties.

Some reasonable possibilities:

  • Don't redirect setProperty to removeProperty for custom properties, since empty string is a valid value. Authors could explicitly use removeProperty if they want to remove. I suspect this may not be web compatible at this point.
  • Serialize empty values as " " instead of "" so that they can be safely used in setProperty, as @prjnt proposed in [css-syntax] Trim whitespace around declarations? #774 (comment)
@Loirooriol Loirooriol added css-variables-1 Current Work cssom-1 Current Work labels Jan 24, 2024
@bkardell
Copy link
Contributor

The latter seems better, right?

@cdoublev
Copy link
Collaborator

cdoublev commented May 3, 2024

The first seems more appropriate and easier to implement.

I suspect this may not be web compatible at this point.

Why authors would use .setProperty() instead of .removeProperty() to remove a declaration?

@Loirooriol
Copy link
Contributor Author

@cdoublev I imagine authors using jQuery like

$(".foo").css({
  display: "block",
  "--foo": "",
})

which I think ends up using https://github.com/jquery/jquery/blob/527fb3dcf0dcde69302a741dfc61cbfa58e99eb0/src/css.js#L253C1-L257C6

if ( isCustomProp ) {
  style.setProperty( name, value );
} else {
  style[ name ] = value;
}

@cdoublev
Copy link
Collaborator

Ok. They expect property values that include var(--foo) to become invalid. I cannot say how many would remain invalid if it was substituted with an empty string.

@tabatkins
Copy link
Member

I agree that we should change getProperty() to serialize an empty custom property to " ".

I was at first concerned that this could break things that are depending on the custom property actually being empty - CSS usually doesn't care about whitespace, just token separation, but we do have a few places that care, like in selectors. But then I remembered that parsing is already specified to strip leading/trailing whitespace, so there's actually no behavior difference between "" and " " as the value.

I do think changing setProperty()'s behavior on custom properties would be better, but I suspect it's not web-compatible, and I don't think it's even possible to tell the compat impact without making the change and seeing what breaks.

Serializing to other values, like "/**/", might be slightly more technically correct, but is more likely, imo, to break scripts that might .strip() the value and test for emptiness.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-variables][cssom] Empty value doesn't round-trip, and agreed to the following:

  • RESOLVED: Serialize custom properties with empty value as a single space
The full IRC log of that discussion <fantasai> oriol: In the past, we didn't have any property that would accept empty value as valid
<fantasai> oriol: but we changed custom properties to trim whitespace and accept empty values
<fantasai> oriol: problem is with CSSOM, because in setProperty if you provide an empty string, then instead of trying to set an empty value it behaves as removeProperty
<fantasai> oriol: so if you serialize a custom property that's empty string, it won't round-trip
<fantasai> oriol: Some possible improvements
<fantasai> oriol: 1. For custom properties we could stop doing removeProperty for assigning the empty string.
<fantasai> oriol: but might have some compat problems at this point
<fantasai> oriol: 2. Instead of serializing empty values as empty string, serialize a single space. This will round-trip.
<dbaron> option 2 sounds good to me
<fantasai> oriol: Could also use a comment instead of space, but space seems preferable
<kizu> +1 to a space
<ChrisL> single space seems a good way, to me
<fantasai> +1 to space
<fantasai> astearns: has anyone tried implementing?
<fantasai> oriol: no
<emilio> q+
<astearns> ack fantasai
<emilio> fantasai: I think there's a benefit with being consistent in the way we remove properties
<astearns> ack emilio
<fantasai> emilio: +1 to that. Serializing to space is simpler and less inconsistent
<fantasai> astearns: any opinions against?
<fantasai> RESOLVED: Serialize custom properties with empty value as a single space

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Regular agenda items
Development

No branches or pull requests

6 participants