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] Substitution of invalid variables into other variables #5370

Closed
JaneOri opened this issue Jul 28, 2020 · 22 comments · Fixed by #6006
Closed

[css-variables] Substitution of invalid variables into other variables #5370

JaneOri opened this issue Jul 28, 2020 · 22 comments · Fixed by #6006
Labels
Closed Accepted by Editor Discretion Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-variables-1 Tested Memory aid - issue has WPT tests

Comments

@JaneOri
Copy link

JaneOri commented Jul 28, 2020

In the spec it says to treat css variables as "unset" instead of "initial" if any compute results in the value becoming 'initial'.
(if the variable is directly set to initial without a compute, it behaves as initial)

https://drafts.csswg.org/css-variables/#invalid-variables

Until the most recent stable release of FF and Chrome, both browsers were ignoring this. #4075


I would like to have this wording changed to match the previous behavior of FF and Chrome:

A declaration can be invalid at computed-value time if it contains a var() that references a custom property with the guaranteed-invalid value, as explained above, or if it uses a valid custom property, but the property value, after substituting its var() functions, is invalid. When this happens, the computed value of the property is the initial value, guaranteeing the Guaranteed-Invalid behavior.

for the following reasons:

  1. Standard properties with inheritance treat initial as sort of unset, except the behavior is "unset since the root". For example, see this demonstration on the inherited color property: https://developer.mozilla.org/en-US/docs/Web/CSS/initial

  2. the spec specifically says that the cascade value should be thrown out by the time it's become invalid at computed-value time:

Note: The invalid at computed-value time concept exists because variables can’t "fail early" like other syntax errors can, so by the time the user agent realizes a property value is invalid, it’s already thrown away the other cascaded values.

  1. The behavior is confusing on the surface:
  code {
    --color: red;
  }
  i {
    --foo: initial;
    --color: var(--foo);
    background: var(--color, lime);
  }

Background of i is red if it's nested inside of code, the expectation was lime

  1. Since initial is the guaranteed invalid behavior

If, for whatever reason, one wants to manually reset a variable to the guaranteed-invalid value, using the keyword initial will do this.

  computing to initial to override a variable (and use fallbacks when referenced) should still be an option but is no longer possible. Now the guaranteed-invalid value is only guaranteed to behave the initial invalid value at the :root and is more of an edge-case than a guarantee we could rely on.

  1. Independent components can no longer be nested (especially if they're two instances of the same component) if they use the same variables and rely on fallback behavior of var references var(.., UNREACHABLE). (This is arguably the single most important reason the previous FF and Chrome behavior was very useful)

^ number 5 there should be a deal breaker for anyone wanting to make css-variables part of their library, even though the decision was recently made to follow spec, the decision had arguments on both sides, ultimately landing in a spot that removes a lot of possibility and readability from CSS variables.


Understanding the reasons listed here, especially num 5, I believe both @andruud and @emilio would agree that the behavior as it was before should be established as correct in the Spec and the browser changes be reverted. (& Safari should come along too)

Thank you for your consideration

@andruud
Copy link
Member

andruud commented Jul 29, 2020

I personally don't agree with that, I think what we currently have now is the most consistent behavior. (Yes, there are arguments in both directions).

Since FF, Safari, Chrome and the spec now all agree, this ship has sailed in my opinion. Even if the spec did change, I'd be reluctant to change the behavior in Chrome (again); we shouldn't flip-flop between behaviors constantly.

If you want to avoid inheriting the (outer) value into some component, then explicitly set the custom property to initial at the component boundary. This is no different from other properties that inherit by default.

@JaneOri
Copy link
Author

JaneOri commented Jul 29, 2020

The behavior is now inconsistent with standard properties actually.

If you want to avoid inheriting the (outer) value into some component, then explicitly set the custom property to initial at the component boundary. This is no different from other properties that inherit by default.

color inherits by default and does not inherit above the component when initial is used:
https://jsbin.com/rokoyamixo/1/edit?html,output

initial makes color behave as if it were unset in the entire ancestry, inheriting its unset value from the perspective of :root

This is what computed-time-value of initial used to do for css variables. (and it's extremely useful)

@emilio
Copy link
Collaborator

emilio commented Jul 29, 2020

So this is about https://bugzilla.mozilla.org/show_bug.cgi?id=1623396, right? Which landed in Firefox 76, which is most definitely not the most recent stable release (that was released mid-may, so we've been shipping this for more than two months).

As I said in #4075 I'm not particularly a fan of the current behavior (I slightly prefer the previous one). That being said, I see arguments both ways, and switching behaviors back and forth is really unfortunate, as @andruud said.

If the spec changes I'd be ok changing behavior in Gecko again, though given the discussion in #4075 and the fact that all browsers now agree on the behavior I think the value of the spec change needs to be huge.

Independent components can no longer be nested (especially if they're two instances of the same component) if they use the same variables and rely on fallback behavior of var references var(.., UNREACHABLE). (This is arguably the single most important reason the previous FF and Chrome behavior was very useful)

Can you clarify? Your example with code and i needs an extra variable indirection to produce red, but an author could pretty much use --color: initial to get the fallback behavior on the background image. Or they could use the fallback for color like: --color: var(--foo, lime), right?

cc @tabatkins

@JaneOri
Copy link
Author

JaneOri commented Jul 29, 2020

@emilio Acknowledged on all points, thank you for the corrections.

Here is minimal var component example:
https://jsbin.com/hulesopayu/1/edit?html,css,output
small change to reflect a more contained component pattern:
https://jsbin.com/hekilegoba/1/edit?html,css,output

The inner most --smaller-input, and any other instances nested within, will always use --bigger-input behavior now because no fallback behavior is possible with initial at computed-value-time now. (unless it's on the root or has no parents with the same var name)

The library I'm working on is of course much more complex than that and has many many layers of "internal" variables that may compute to initial in any number of ways so I could use fallback behavior, all based on a small number of end-user input options.


@andruud Please know I respect your opinion here even though I disagree with the conclusion. It wouldn't be a reasonable request for me to ask this if we can't coordinate with all 3 browsers to move forward. Perhaps an alternative path to allow the previous computed-value-time initial behavior could be added independently? I haven't given much thought to that but would you be more comfortable going in that direction?

I think everyone will agree with both of you that it's really nice to have all 3 browsers on the same page (and agreeing with the current spec).

@andruud
Copy link
Member

andruud commented Jul 29, 2020

"Internal" variables, huh? 🙂

You can use @property to register the properties as non-inherited. https://jsbin.com/bosufutixa/edit?html,css,output (Chrome 85+ only!)

@andruud
Copy link
Member

andruud commented Jul 29, 2020

(Thinking about it some more:)

Perhaps an alternative path to allow the previous computed-value-time initial behavior could be added independently?

Yeah, that could be the way to go. It does seem useful to be able to control the IACVT behavior in your example (thanks for that BTW 👍). Not sure how though. Maybe it's an aspect of the property itself (in which case it's @property territory), or it's an aspect of the value, e.g. a new function which takes any value, and falls back to something else if the main value is IACVT (for example --bigger: validate(calc(var(--other) * 5), initial). (Almost like catching an exception ...)

Some of this reminds me of #5055, though that's not quite going to work here as-is. (Linking anyway for inspiration).

On the other hand, it seems easy to solve this problem in practice by just having the outer element of the component reset all the custom properties that shouldn't cross the boundary, and then an inner element can (attempt) to set the actual values for those properties.

@JaneOri
Copy link
Author

JaneOri commented Jul 29, 2020

No worries! I'm glad the example was helpful

Catching an exception sounds like the right track and I love the idea of an inline customizable fallback...
Perhaps --bigger: try(calc(var(--other) * 5, initial); ?
edit: Just realized it would be impossible to parse this semantically unless the fallback here was required and had to be a , followed by a single token... which is still very useful

On the other hand, it seems easy to solve this problem in practice by just having the outer element of the component reset all the custom properties that shouldn't cross the boundary, and then an inner element can (attempt) to set the actual values for those properties.

Yep, this is how I've worked around it in my library. It may be easy for small libraries but the real-world cost of my project is:

  1. 107 lines of CSS in a new [data-mylibrary-reset] selector, using the initial keyword on every conceptually-internal variable that may compute to initial.
  2. End user pain point and maintenance concern that they must use an additional DOM layer to achieve nesting
  3. Added complexity in documentation and tests
  4. Had to remove several features where pseudo element overrides were previously possible (this can be simulated with 2 additional DOM elements now though, but that's a much bigger ask for an end user and the library developer)

@property is a good call out too though. We (library developers) may have to register hundreds of them to prevent the behavior, and deliberately inherit what's needed on pseudo children, but it is a path that solve the problem, just in a different and somewhat expensive way. (assuming inherit doesn't trigger this behavior and stops at the parent)

That solution with the inline fallback in a validate/try that you came up with though would be an incredibly useful addition. I'm all for that

@tabatkins
Copy link
Member

tabatkins commented Jul 30, 2020

Copied over from #4075:

It sounds like what you're wanting is for the guaranteed-invalid value to be preserved as it's substituted thru custom properties, so that the iacvt behavior of making the value unset only triggers on "real" properties (or perhaps on registered custom properties as well), and you can trigger fallback at any point in a variable chain (rather than only at the exact point where the guaranteed-invalid value is introduced). Is this right?

This isn't an unreasonable request! In w3c/css-houdini-drafts#994 (comment) Amelia asks for the ability to explicitly allow a registered property to still assume the guaranteed-invalid value, which is currently only possible when you have a completely unrestricted grammar. Implicitly, this is recognizing that the guaranteed-invalid value actually is "valid" in some circumstances; it's guaranteed to never match the grammar of any predefined CSS property, but it's part of the value space (and thus, matches the grammar) of unregistered custom properties, or registered ones with unrestricted grammar.

This is a different choice than what we've currently made in the spec, but it's not a wrong one. And I think I've talked myself into preferring it. It would be a minor behavior change, tho. @andruud, any thoughts?

@JaneOri
Copy link
Author

JaneOri commented Jul 31, 2020

Hello @tabatkins,

... trigger fallback at any point in a variable chain (rather than only at the exact point where the guaranteed-invalid value is introduced). Is this right?

Yes, you nailed it, succinct too. 😄 That definition of the behavior enables modular/independent/nestable css variable components

Thank you!

@andruud
Copy link
Member

andruud commented Jul 31, 2020

Implicitly, this is recognizing that the guaranteed-invalid value actually is "valid" in some circumstances; it's guaranteed to never match the grammar of any predefined CSS property, but it's part of the value space (and thus, matches the grammar) of unregistered custom properties, or registered ones with unrestricted grammar.

When you put it like that, it makes sense to me too.

It would mean that any property that is IACVT becomes:

  • guaranteed-invalid if that value is permitted by the grammar of the property, or
  • unset in all other cases

If we are recognizing guaranteed-invalid as "valid" in some cases, we must IMO also recognize that it's not permissible in all other cases. This has some consequences for cyclic at computed value time. We currently assign guaranteed-invalid to all custom properties in a cycle, regardless of their grammar. With this new way of thinking about guaranteed-invalid, we could only assign this value to custom properties which support it. In other words, non-universal registered custom properties in a cycle would become unset, which is a change from how it currently works. Basically I think we could just remove the cyclic at computed value time definition, and merge it with IACVT.

What bugged me initially in #4075 was the inconsistency that standard properties behaved in one way and custom propeties in another for no apparent reason. But the new model would "explain" the difference via what is allowed by the grammars, which seems satisfactory to me. (So even a standard property could in theory become guaranteed-invalid if it explicitly allowed it for some reason).

I'm not sure how easy it is to make this change given that all browsers currently agree on the behavior. I'm also not thrilled about doing a 180. But since it seems to be what everyone wants, and I'm warming up to the idea myself, I suppose we could try. 🤷‍♂️

@tabatkins
Copy link
Member

tabatkins commented Jul 31, 2020

While it's a near-reversal in behavior, we're not just swapping our opinion back, we're actually going for a new argument that happens to look similar to the older behavior. I'm okay with that. ^_^

I'll get some edits in soon.

@andruud
Copy link
Member

andruud commented Aug 1, 2020

Since we are leaving a state of "perfect interop", we should make sure Apple agrees with the change before making the edit. I don't think this is valuable unless all browsers are willing to change their behavior. @tabatkins who on the Webkit team would be a good person to comment on this?

@tabatkins
Copy link
Member

tabatkins commented Aug 5, 2020

Hm, not sure who precisely who is working on this sort of thing over there. @hober @smfr who should we be consulting?

@smfr
Copy link
Contributor

smfr commented Aug 6, 2020

I think we'd be willing to follow if other browsers think there isn't a compat concern.

@andruud
Copy link
Member

andruud commented Aug 6, 2020

@smfr Thanks, I can investigate the compat situation by adding a use-counter in Chrome.

@JaneOri
Copy link
Author

JaneOri commented Aug 6, 2020

I'm not sure how specific use-counter can get but it should only count when the unset/inheritance happens and does not resolve to initial since libraries like https://propjockey.github.io/css-media-vars/ rely heavily on the IACVT fallback and has recently become popular - all the properties are set on :root so this specific modularity issue does not happen.

Plus, others were at least experimenting with the previous chrome/firefox behavior too: https://twitter.com/gavinmcfarland/status/1289010777651847168 so use-counter would almost have to determine intention if it's just out there and now broken somewhere.


Tangentially, If the change moves forward, the current-spec-behavior can be reproduced when #5325 is resolved by using inherit in the fallback. Which is a really good alternative.

If the change doesn't move forward, there is no easy path alternative to reach the IACVT fallback in the nested/modular situation unless your new validate #issuecomment-665864719 idea moves forward

@JaneOri
Copy link
Author

JaneOri commented Aug 6, 2020

Since the issue was re-opened in Chromium bug tracker specifically to implement the use-counter, my point may not have been clear... @andruud @emilio @smfr (please correct me if I am wrong)


Problem: A use-counter cannot measure compatibility with the developer's intention unless you also measured before the change a few months ago and subtract those.

Reason: Any CSS causing IACVT that now specifically inherits a non-initial value from an ancestor (based the spec's definition) was exclusively either:

  1. written in the last few months and doing this intentionally
    OR
  2. written specifically and only for Safari browsers any time since Q1 2016
    OR
  3. written as early as Q1 2016, before the spec-aligned behavior started a few months ago in FF&Chrome, and would therefore now be a false-positive for the same use-counter. (and is likely causing problems anywhere it's happening)

All other behavioral branches in the IACVT cases are identical in an end-result behavior between the two implementations and are therefore also not a measurement for compatibility with the author's intention.


The most likely branch, # 3, is the reason it hit my radar, and also this person's: https://twitter.com/gavinmcfarland/status/1289010777651847168

blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Aug 13, 2020
There is a desire to change how this works (again) [1], but since
all browsers agree on the current behavior, we have to be careful
with changes. Hence this CL adds a detailed use-counter.

The behavior we want to change can be illustrated with the current
example:

  <style>
    #outer { --x: foo; }
    #inner { --x: var(--unknown); }
  </style>
  <div id=outer>
    <div id=inner></div>
  <div>

Current behavior: the computed value of --x on #inner behaves as
'unset', and hence becomes ' foo'.

Desired behavior: the computed value of --x on #inner becomes the
guaranteed-invalid value [2].

More generally, any property whose grammar "supports" the guaranteed-
invalid value shall become guaranteed-invalid if it's invalid at
computed-value time (IACVT) [3]. Properties which currently supports
this are: unregistered custom properties, and registered custom
properties with universal syntax.

This CL avoids counting cases where the new behavior wouldn't make
a difference:

 - If the property doesn't support <guaranteed-invalid>
 - If the property is non-inherited without an initial value
 - If the property would anyway inherit <guaranteed-invalid>

[1] w3c/csswg-drafts#5370
[2] https://drafts.csswg.org/css-variables/#guaranteed-invalid-value
[3] https://drafts.csswg.org/css-variables/#invalid-at-computed-value-time

Bug: 1110188
Change-Id: I1a230e03b59ee3490166e92e1e83586a681301a0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2352871
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797612}
@andruud
Copy link
Member

andruud commented Aug 14, 2020

I've added the use-counter in Chrome, I'll report back in October when I have some data.

@JaneOri
Copy link
Author

JaneOri commented Nov 26, 2020

Hey guys, sorry to bump but it's been a while past October now, is there an update on this?

@Loirooriol
Copy link
Collaborator

Loirooriol commented Nov 26, 2020

I think the counter is CSSInvalidVariableUnset, so 0.016%

@JaneOri
Copy link
Author

JaneOri commented Jan 5, 2021

Hello @tabatkins @emilio @andruud

May we continue this discussion please?

Thank you,
// James

edit: Thank you! https://groups.google.com/a/chromium.org/g/blink-dev/c/0xrbzYe_vxU/m/52xVJHTICQAJ

@andruud
Copy link
Member

andruud commented Feb 16, 2021

I now have approval to ship this change in Chrome, for what that's worth. This is contingent on the spec change happening first, and other vendors still being on board with the change. (@emilio @smfr)

I think we'd be willing to follow if other browsers think there isn't a compat concern.

The Chrome use-counter is at ~0.02% of page loads, and I couldn't find any breakage among that sites that showed up related to the use-counter. (See I2S post for details). So it doesn't appear to be a huge compat concern. (For Chrome that is).

andruud added a commit to andruud/css-houdini-drafts that referenced this issue Feb 16, 2021
 - Export "registered custom property".
 - Export "universal syntax definition".
 - Fix a "the the".
 - When parsing against the syntax, we probably want invalid at
   computed-value time, and not guaranteed-invalid?

Somewhat related to: w3c/csswg-drafts#5370
tabatkins pushed a commit to w3c/css-houdini-drafts that referenced this issue Feb 26, 2021
…1020)

- Export "registered custom property".
 - Export "universal syntax definition".
 - Fix a "the the".
 - When parsing against the syntax, we probably want invalid at
   computed-value time, and not guaranteed-invalid?

Somewhat related to: w3c/csswg-drafts#5370
@tabatkins tabatkins added Closed Accepted by Editor Discretion Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. Tested Memory aid - issue has WPT tests labels Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by Editor Discretion Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-variables-1 Tested Memory aid - issue has WPT tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants