-
Notifications
You must be signed in to change notification settings - Fork 696
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-values] Short-circuit if() evaluation #11500
Comments
This would extend to custom functions as well; none of the following functions are in a cycle:
This probably does place
EDIT: Added |
Yes, --x() and --y() should be non-cyclic; --z() has a cycle inside of itself (--unused is cyclic) but the function itself isn't cyclic; and --w() is cyclic. |
This is now fairly easy to do by extending the CycleElem type introduced in CL:6176920. Both attributes (used by attr()) and locals need the AtomicString type; we can distinguish between the two cases using a StrongAlias. Note that this CL follows the behavior described in Issue 11500, not what the spec currently says. Therefore, the test is marked as tentative. w3c/csswg-drafts#11500 Bug: 325504770 Change-Id: I5a00e03175f0446a0ec6e6ba771253b4ea5f48e6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6190327 Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Reviewed-by: Munira Tursunova <moonira@google.com> Cr-Commit-Position: refs/heads/main@{#1415484}
This is now fairly easy to do by extending the CycleElem type introduced in CL:6176920. Both attributes (used by attr()) and locals need the AtomicString type; we can distinguish between the two cases using a StrongAlias. Note that this CL follows the behavior described in Issue 11500, not what the spec currently says. Therefore, the test is marked as tentative. w3c/csswg-drafts#11500 Bug: 325504770 Change-Id: I5a00e03175f0446a0ec6e6ba771253b4ea5f48e6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6190327 Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Reviewed-by: Munira Tursunova <moonira@google.com> Cr-Commit-Position: refs/heads/main@{#1415484}
This is now fairly easy to do by extending the CycleElem type introduced in CL:6176920. Both attributes (used by attr()) and locals need the AtomicString type; we can distinguish between the two cases using a StrongAlias. Note that this CL follows the behavior described in Issue 11500, not what the spec currently says. Therefore, the test is marked as tentative. w3c/csswg-drafts#11500 Bug: 325504770 Change-Id: I5a00e03175f0446a0ec6e6ba771253b4ea5f48e6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6190327 Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Reviewed-by: Munira Tursunova <moonira@google.com> Cr-Commit-Position: refs/heads/main@{#1415484}
…variables, a=testonly Automatic update from web-platform-tests [functions] Detect cycles between local variables This is now fairly easy to do by extending the CycleElem type introduced in CL:6176920. Both attributes (used by attr()) and locals need the AtomicString type; we can distinguish between the two cases using a StrongAlias. Note that this CL follows the behavior described in Issue 11500, not what the spec currently says. Therefore, the test is marked as tentative. w3c/csswg-drafts#11500 Bug: 325504770 Change-Id: I5a00e03175f0446a0ec6e6ba771253b4ea5f48e6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6190327 Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Reviewed-by: Munira Tursunova <moonira@google.com> Cr-Commit-Position: refs/heads/main@{#1415484} -- wpt-commits: c5dda19e7580e26e7f5e538af095fdff241c9301 wpt-pr: 50483
…variables, a=testonly Automatic update from web-platform-tests [functions] Detect cycles between local variables This is now fairly easy to do by extending the CycleElem type introduced in CL:6176920. Both attributes (used by attr()) and locals need the AtomicString type; we can distinguish between the two cases using a StrongAlias. Note that this CL follows the behavior described in Issue 11500, not what the spec currently says. Therefore, the test is marked as tentative. w3c/csswg-drafts#11500 Bug: 325504770 Change-Id: I5a00e03175f0446a0ec6e6ba771253b4ea5f48e6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6190327 Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Reviewed-by: Munira Tursunova <moonira@google.com> Cr-Commit-Position: refs/heads/main@{#1415484} -- wpt-commits: c5dda19e7580e26e7f5e538af095fdff241c9301 wpt-pr: 50483
Had a big discussion with @andruud about this offline, and pulled in a few related issues (notably, #11144). Here are a few of the issues we wanted to solve, in no particular order:
So, new proposal that solves this issue, #11144, and the "authors will forget to use
This has the downside that using a substitution function that evaluates to a comma-separated list acts differently depending on whether it's inside a normal function or another substitution function. (As said above, There are a few other downstream consequences, which I think are okay. For example, |
This comment has been minimized.
This comment has been minimized.
I like the idea of early and later grammars, especially if this allows us to do
According to the https://www.w3.org/TR/css-variables-1/#cycles,
Note the “including in the fallback argument”. All browsers comply with this part, which led to me writing this article, where I found a use for this fact: https://kizu.dev/indirect-cyclic-conditions/ That said, the technique in this article is purely for conditionals, and could be retired once we will have |
Assuming that you would have written, say,
Yeah, we'd ideally like to change that. ^_^ Since we're doing |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thanks @tabatkins. It looks like that plan is going to give authors the power they need w.r.t. variables in the "conditional parts", while also making it possible for impls to reason about the branches to do short-circuiting. 👍
@kizu Does that mean you'd like to keep this behavior even if the web-compat situation allows it? |
Not a strong opinion, but yes. And, probably, only for the regular variable fallback case: for things like branches inside I would be curious to learn which anticipated edge cases led to the fallbacks being considered as a part of the cycle even when not used: were there actual circularity cases that could happen, and we did not know how to handle (and know what to do now)? And if we change this behavior: what is the benefit of that change, which use cases does it unlock then? |
I wasn't involved then, but I suspect it was done that way to make it possible to handle cycles without doing any evaluation? But things have gotten significantly more complicated (dynamic) since then. For example,
More or less the same as |
The CSS Working Group just discussed
The full IRC log of that discussion<emilio> TabAtkins: so... problem that showed up when we started thinking about if() but it applies to other substitutions<emilio> ... in most programming languages, the not taken branch isn't ran <emilio> ... and has no effect and gets ignored <emilio> q+ <emilio> TabAtkins: as currently spec'd that's not true for if() <emilio> ... it's expected to fully resolve and then choose <emilio> ... it's not great <emilio> ... invalid variables, expensive things like custom functions <emilio> ... having to run all of the branches to throw all but one of them seems bad <emilio> ... so we'd like to find a way to allow deferred evaluation <emilio> ... so that it also applies to fallbacks and so on <emilio> ... the proposal is to change how substitutions are parsed <emilio> ... two grammars, early and late <emilio> ... early grammar is super wide (<decl>) <emilio> ... and punctuation to separate the structure <emilio> ... so in if() you'd get : and ; to separate values <emilio> ... that's what the initial parse uses <emilio> ... not evaluating or applying any grammar to it <emilio> ... then each function defines how it's executed with it's late grammar <emilio> ... so if it's true it resolves the condition and the if body etc <emilio> ... same would apply to other functions <bradk> I have to leave for another meeting. Just wanted to say I am impressed with Zoom’s automatic (AI) captions and transcripts. <emilio> ... so attr() we could avoid evaluating the fallback <emilio> ... I'd like to apply this universally <emilio> ... maybe var() needs to unconditionally evaluate <emilio> ... so might be compat fallout <emilio> ... but for new functions I'd like to specify in this way <emilio> ... so the gross structure is in the early parse and the function controls how it's parsed <emilio> ... that means vars can no longer supply the punctuation, e.g. a custom function would not be able to expand into arguments <emilio> ... so we'd add syntax like the ... so that we expand it during early parse <emilio> ... by default variables would not resolve until requested <astearns> q+ <emilio> ... so if you have a function like --foo(var(--var-1), var(--var-2)) <emilio> ... you know there are two args <emilio> ... specially for custom functions there are two arguments if there are commas <kizu> q+ <emilio> ... authors need to deffensively wrap in {} <emilio> ... no longer the case now, authors can depend on a var that's 1 argument being 1 argument <emilio> ... this actually solves a footgun that we were somewhat worried about about comma-containing args <emilio> ... most of the time shouldn't have an effect <emilio> ... the implications are meaningful tho <emilio> ... wanted to make sure that it makes sense to other folks <astearns> ack emilio <TabAtkins> emilio: i guess that means cycles are discovered dynamically? <TabAtkins> emilio: can get a bit weird <TabAtkins> TabAtkins: correct <TabAtkins> emilio: worried about things that can create a cycle on an else branch, and theoretically could effect the condition <TabAtkins> emilio: havne't thought too deep about it, but seems sketchy <TabAtkins> emilio: other than that, think it makes sense <TabAtkins> emilio: var() sub tends to get annoying, the more features we add the slower it gets, nice to save some work <TabAtkins> emilio: but i'm a bit wary about cycle detection <TabAtkins> emilio: are we supposed to check the variable names inside beforehand? <emilio> TabAtkins: yea, cycle detection becomes dynamic <emilio> ... you don't do any checking <emilio> ... this allows variable-variables <emilio> ... because that'd be possible now <emilio> ... for the more general thing of the dynamic cyclic graph <emilio> andruud: that's kinda the point of this, not sure what you're worried about right now emilio <emilio> TabAtkins: I suspect he means "say the test for a branch uses a variable and the body produces a cycle for the test you're evaluating" <emilio> andruud: but that's the branch you take right? <emilio> TabAtkins: right but it only becomes cyclic once you evaluate the branch right? <emilio> andruud: you have that problem with if with the existing model don't you <emilio> TabAtkins: yeah they're all eagerly evaluated so you know cycles before-hand <emilio> andruud: we solved the example you had for if <emilio> ... having a custom prop in an if counts for cycle detection <emilio> TabAtkins: right that's the usage emilio is worried about <emilio> ... the first usage "should've been cyclic" <emilio> ... but the execution order gets you a defined behavior <TabAtkins> emilio: does that mean the order you perform prop substitution needs to be well-defined? <TabAtkins> emilio: right now if you have two rules that match with the same var, ... i guess my question is when. the behavior is different if you sub the first if() first, on whether this makes a cycle or not <TabAtkins> emilio: I suppose that means the whole thing is stateful on the set of matched declarations <TabAtkins> emilio: and we need to define reeally well whether we sub properties that are overwritten or not, which currnetly we skip... <TabAtkins> emilio: i think there's a way to make this work, details are jsut confusing <TabAtkins> emilio: but i think figuring out the spec is part of thi sissue, don't need to solve on the call <emilio> astearns: wanted to ask whether the state parsing is separated from the spread function <emilio> TabAtkins: if you stick with current parsing there's no reason to do spread <emilio> ... so would be a no-op of a feature <emilio> astearns: wondering whether variables with punctuation is something we absolutely need to support or not <emilio> TabAtkins: not about vars containing punctuation <emilio> astearns: I thought motivation for spread was getting punctuation for what needs to go in which part of the parsing <emilio> q+ <emilio> ack astearns <astearns> ack kizu <emilio> kizu: +1 to almost everything <emilio> ... would be great to have this algorithm, wanted to use var(var( for a long time <TabAtkins> var(var(--propname)) <bramus> +1 <emilio> ... only concern is if we are able to make the fallback not executed I'm totally fine for if() <emilio> ... for current var() I think there would be many compat issues <emilio> ... but I think it's possible to use this behavior as a conditional <emilio> ... that could be a workaround for the conditions in general <emilio> ... meaning that this behavior could be use as a transpillation target for if() <emilio> ... until we have if this could be used for that <emilio> ... that's the only case where I'd be a bit hesitant <emilio> ... It'd be nice to see if there are compat issues aside from my experiments <astearns> ack emilio <TabAtkins> emilio: i think alan was asking about whether we can work on spread after changing the parsing stuff <TabAtkins> emilio: that seems possible unless we really... as long as we keep doing the same thing for var() <TabAtkins> emilio: i think it woudl be nice to ahve this work for var() <TabAtkins> emilio: i think the place i've meaningfully seen commas be used is things like colors <TabAtkins> emilio: you can do `--foo: 1, 2, 3; color: rgb(var(--foo))` <emilio> TabAtkins: right, this would only change behavior for custom functions / var() / etc and other functions <emilio> ... a bullet that I'm willing to bite <TabAtkins> emilio: i agree, and think it takes mos tof the compat issues with var() away <TabAtkins> emilio: that gives me some hope that we can make var() work <emilio> TabAtkins: the compat issue here is if they are relying on non-substituted fallback triggering cyclicness <emilio> ... I suspect outside of experiments like kizu's it's not an issue in practice <emilio> astearns: I'm a bit concerned about some library depending on kizu's hacks and avoids defeating this <emilio> TabAtkins: andruud is working on this actively <emilio> ... so we'd find out about such things soon <emilio> andruud: var() we need a use counter for first <emilio> ... so I'd do that separately (soon) <emilio> ... and we can resolve on doing it compat-permitting <emilio> kizu: I wonder if there'd be no other compat issues I could see making this change for var() be tied to if() so that we can do at the same time <emilio> ... so you can also implement this along @supports ... if() <emilio> TabAtkins: if anything if() would show up earlier than var() <emilio> kizu: seems fine then <emilio> ... other browsers can do if-first or at the same time as well <emilio> astearns: so proposal is to specify this for all substitution functions and also the spread <emilio> TabAtkins: the spread syntax needs figuring out <emilio> astearns: with the usual caveats <emilio> ... so not a done deal <emilio> PROPOSED RESOLUTION: Specify this new parsing behavior for all subsitution functions, and some sort of spread-like thing <emilio> RESOLVED: Specify this new parsing behavior for all substitution functions, and some sort of spread-like thing |
Use-counters are in Chrome M135, going stable April 1st. |
In a world where huge trees of custom function calls and inline if()s need to be evaluated, it would be nice if we could stop evaluating once a matching condition is found:
In the above, we don't want to evaluate (substitute)
--expensive()
if an earlier branch will do.Relatedly, a
var()
function also has up to two branches: the main value produced by the custom property being referenced, and optionally the fallback value. As specified currently, we can not short-circuit this "evaluation", since we need to look for cycles in the fallback value:--x
and--y
are in a cycle above, even though the fallback paths are not taken. By default, I'd expect this to extend to the branches not taken forif()
as well, otherwise we'd miss the cycle in a case like:So:
if()
?var()
when it's not taken? (Requires compat investigation.)attr()
).The text was updated successfully, but these errors were encountered: