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-cascade-4] Revert at computed-value time #4155

Closed
andruud opened this issue Jul 29, 2019 · 14 comments
Closed

[css-cascade-4] Revert at computed-value time #4155

andruud opened this issue Jul 29, 2019 · 14 comments
Labels

Comments

@andruud
Copy link
Member

andruud commented Jul 29, 2019

CSS Color Adjustment Module Level 1 expects to be able to revert color properties based on the computed value (I assume) of forced-colors-adjust. I'd expect it to be too late to revert computed-value time, since revert is an instruction for the cascade, and being computed-value time means we've already started resolving the cascade.

In general I'd expect the cascade not to be modified while computing values. (I'm sure there's an example where we already do ...)

What do css-cascade experts think about this?

@fantasai fantasai added the css-color-adjust-1 Current Work label Aug 3, 2019
@fantasai
Copy link
Collaborator

fantasai commented Aug 3, 2019

It's a bit messy, but we do have some amount of cascading dependence on computed property values already: the way logical properties cascade depends on the values of writing-mode and direction.

It's not a very generalizable mechanism, so the number of properties depended on here needs to be kept to an absolute minimum. But it does seem to describe the behavior here, so that's how it's described in the spec.

CC @atanassov

@Loirooriol
Copy link
Contributor

Logical properties cascade normally, the writing mode is only used when determining the computed values, so it's fine.

@andruud
Copy link
Member Author

andruud commented Aug 3, 2019

@fantasai Yeah, I wish css-logical didn't set this precedent ...

Though I'd argue that this revert case is worse. As @Loirooriol is pointing out (I think), the logical properties can be implemented as separate cascadable properties. As long as we keep around the cascade criteria (origin, importance, order of appearance, etc) for each value in the cascade, we can figure out whether a given logical property should win over (what turned out to be) the corresponding physical property computed-value time. So in a way we can translate the cascade-time dependency into a computed-value-time-only dependency that's not much worse than the dependency between font-size and em units.

In order to do the same in this case, we'd have to keep around one cascaded value for each cascade origin, or something. (This is what WebKit is doing, incidentally, and it's not pretty).

FF has a very elegant solution to revert: rules are applied in reverse order, such that revert simply becomes "just forget the current cascaded value up until this point". (We'll get to the reverted-to value eventually). Computed-value time revert breaks this, and it's a shame IMO.

I may be drama-queening a little right now, but I worry slightly that CSS will deteriorate into nondescript mystery meat without discernible properties if we keep chipping away at former invariants.

Perhaps we should instead specify that, in forced colors mode, all UA declarations become !important or something.

@andruud
Copy link
Member Author

andruud commented Aug 3, 2019

in forced colors mode, all UA declarations become !important or something.

Yeah .. that clearly doesn't solve anything. Duh.

@andruud
Copy link
Member Author

andruud commented May 7, 2020

I'll include my rant in this topic from the Blink I2S, for reference:

(Excerpt begins).

The biggest thing I don't like about this spec is that it's attempting to 'revert' at computed-value time, which I really think shouldn't be allowed. We risk running into a circularity at some point, since computing 'forced-color-adjust' requires cascading, yet its computed value affects how things cascade. (Yes, css-logical has a similar problem, which is not a convincing argument for making things worse). It's not a huge problem in practice, since the properties being reverted don't really affect other properties currently.

But imagine if forced colors mode wanted to do 'font-size:revert' (which doesn't make sense, but let's say that it did):

@property --x {
  syntax: "<length>";
  inherits: false;
  initial-value: 0px;
}

div {
  font-size: 20px;
  --x: 10em;
  forced-color-adjust: var(--x);
}

In order to know the computed value of 'forced-color-adjust' in this example, we must first compute '--x'. This contains 'em' units, which means the computed value of 'font-size' needs to be known. The computed value of 'font-size' is obviously '20px', which makes the computed value of '--x' 200px. When substituted into forced-color-adjust, we get forced-color-adjust:200px, which doesn't parse for that property, hence it becomes invalid at computed-value time, and is treated as unset (which becomes auto in this case).

Now the forced colors mode wants to revert font-size back to the UA-style (e.g. 16px), but the computed value of font-size has already been determined at 20px. Worse, its previously computed value had side effects: --x has a computed value of 200px, which would no longer be correct if font-size is reverted after its value is computed. Reverting font-size alone will not be enough to undo the "damage" done by the temporary and ultimately incorrect computed value of font-size. This is a bad situation.

To reiterate, the spec doesn't revert any properties that other properties depend on, so it doesn't exhibit this bad behavior currently. But it seems like it very easily could in the future, if we were to e.g. change how colors work. And in general I wanted to highlight why I think reverting at computed-value time isn't great.

(Excerpt ends).

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-cascade-4] Revert at computed-value time.

The full IRC log of that discussion <dael> Topic: [css-cascade-4] Revert at computed-value time
<dael> github: https://github.com//issues/4155
<gregwhitworth> yeah, can we at least do anatomy investigation in Open UI for this?
<dael> Rossen_: Brought up by Anders who would be best to summarize.
<dael> Rossen_: Base of issue is when and how do properties overwritten by forced colors get reverted
<dael> Rossen_: Based on the issue there is a pushback against if the revert rules can apply at computed value time and if this creates dependency between properties and the way revert and override work
<dael> Rossen_: Couple weeks ago took a resolution where we introduced the !override that can be used to override all the properties and all the adjust properties. At that point forced-colors-adjust property is not needed
<dael> Rossen_: Questioning if this issue will go away and we can close no change needed
<dael> Rossen_: Noted TabAtkins and fantasai as you made edits in this space you may or may not have gotten to this work. Can you tell us where you are with this?
<dael> TabAtkins: We haven't gotten to it so not comfortable with details yet.
<dael> Rossen_: On the face value would you think if we had !override (tbd name) specified and impl would that simply allow us to resolve this issue?
<fremy> q+
<dael> TabAtkins: Given that this is the default...whenever forced-color sis auto the UA stylesheet overrides. This is about effects of forced-color-adjust mode. The property just lets you turn things off. THe revert is what happens by default not something the author turns on
<dael> Rossen_: Yeah. My hope was we could use !override UA declaration instead of current revert
<dael> TabAtkins: mmmm...possibly
<dael> TabAtkins: If UA hid it behind MQ maybe
<TabAtkins> And then authors could use !override to win over the UA style when necessary...
<gregwhitworth> emilio: I added a comment regarding the Open UI work to the issue: https://github.com//issues/5049#issuecomment-631582242 Feel free to setup some time with me so we can begin defining that and inform the psuedo element naming and if there is need for anything else
<dael> Rossen_: If UA spec color with !override forced-color and no user style matched with the eq you have the forced-color. If user wants to expose they can. That's why going down the path that override will solve this issue as well.
<dael> Rossen_: Given we don't have Anders on I'm fine pushing this by a week to see if we can make progress
<Rossen_> ack f
<dael> fremy: Quick note- Basically revert at computed time kind of does exist with :visited. We have stored in browser real style for rendering and style for :visited. If I understand spec we only need it for 2 properties, color and bg-color. Doesn't sound like a huge deal
<dael> fremy: Doesn't seem complex. Examples in issue don't seem like reason examples. They're crazy scenarios. Works for scanrios in the spec right now.
<dael> Rossen_: I think we won't be able to make progress o nthis issue

@FremyCompany
Copy link
Contributor

@andruud I think I agree with you that revert-at-computation-time is a rather novel concept that can pose challenge if applied everywhere, but it's not unheard of. My understanding is that the forced-colors specification only requires such reverting to apply for a handful properties, and those don't trigger the kind of complicated situations you seem worried of here.

As mentioned in the discussion today, this is pretty similar to how browsers already have to remember an additional "color as if :visited didn't exist" for many of these properties. My understanding of this, and how I recall it has been implemented in the past, is that for a handful of properties like color/background-color, a second field is added to the element styles that only considers rules in the user agent stylesheet; and those additional fields can be used to revert values at computed time easily, just by switching to the other backing field on demand. That doesn't seem to be a complicated solution to me.

@andruud
Copy link
Member Author

andruud commented May 21, 2020

They're crazy scenarios.

Hehe, great assessment.

@FremyCompany I'm not worried about the implementation difficulty of this as it stands currently. My worry is that we're setting a potentially dangerous precedent on the spec level.

The act of computing the value of force-color-adjust means the computed value of all the properties it depends on must also be known. I would then consider it too late to revert any of those dependencies to a different computed value, as it would mean there are two answers for what the computed value is.

I'm well aware that force-color-adjust can't currently depend on anything that will be reverted. So for now that works. But this also sets a precedent that reverting at computed-value-time is OK, and reinforces the unfortunate precedent set by css-logical that computed-value-time results can affect cascade behavior, and that is the part that worries me.

But since I seem to be the only one sufficiently worried about it (and I don't really have a better solution), I think it makes sense to close this issue and live with it. If we ever manage to create a world where the bad situation is possible, hopefully we can invent some mitigation to maintain consistency in the cascade.

@tabatkins
Copy link
Member

@andruud Are you familiar with the !override idea we talked about on the call (and on a previous call)?

I haven't written it out yet (soon!), but basically if a declaration is tagged with !override, you track it as an additional cascaded value (essentially remembering two values for the property - one "all the values, both !override and not" and one "only the !override values").

Then, if forced-colors-mode is active, this is a property affected by forced-colors mode, and there's at least one !override value for the property, the !override value automatically wins. Otherwise, the normal cascade wins.

Does this satisfy your circularity concerns? It should achieve the same effects as "revert at computed-value time" if the author doesn't do anything, and let us drop the forced-color-adjust property altogether, as authors can tag their own declarations with !override to win over the UA values when necessary.

@andruud
Copy link
Member Author

andruud commented May 25, 2020

I think I know what we should do now. Allow revert at computed-value time as a thing, but specify (if/when necessary) that forced-color-adjust becomes invalid at computed-value time if it depends on any property affected by the forced colors mode. This way we don't actually need to compute the value of the invalid dependency, and there isn't an inconsistency.

More generally properties whose computed value affects cascade behavior should be invalid at computed-value time if it depends on things it's affecting in the cascade. So for example, if direction somehow managed to depend on margin-left (use your imagination for how this could be possible in the future), then direction is IACVT because there might be a css-logical property which may or may not resolve to margin-left, depending on the computed value of direction.

I'm closing this for now, since the crazy scenarios are currently unreachable (and they might stay that way).

@tabatkins Not familiar, but sounds like it would indeed avoid the problem. However, I was under the impression that we wanted a way to disable forced colors for entire subtrees, in which case !override doesn't sound very practical?

Also:

  • Why cascade !override separately if forced-color-adjust doesn't exist? If forced-color-adjust doesn't exist, we're automatically not depending on computed-value time anymore, so !override might as well be a normal priority in the cascade.
  • Cascading multiple values for each property seems like an unsustainable pattern. As @FremyCompany points out we're already doing this in practice for colors, so this would mean we'd now need to cascade four values per color: :visited regular, :visited !override, unvisited regular, unvisited !override). We should IMO avoid this completely.

@andruud andruud closed this as completed May 25, 2020
@tabatkins tabatkins reopened this May 26, 2020
@tabatkins
Copy link
Member

However, I was under the impression that we wanted a way to disable forced colors for entire subtrees, in which case !override doesn't sound very practical?

That's what f-c-a did, but it wasn't necessarily what was required. If you can target the subtree with selectors, you can hit all the necessary color properties with !override, and achieve the same thing.

Why cascade !override separately if forced-color-adjust doesn't exist? If forced-color-adjust doesn't exist, we're automatically not depending on computed-value time anymore, so !override might as well be a normal priority in the cascade.

I don't understand the question; !override and f-c-a have nothing to do with each other.

We can't have !override be another priority level; it's meant to be used in the UA stylesheet to implement forced-colors mode, after all, and it wouldn't make sense to have those rules always win. It's also meant to be used by authors to override the UA stylesheet where necessary, and it seems like it would be bad for usability to have it mess up the author's normal specificity when forced-colors mode isn't active.

Cascading multiple values for each property seems like an unsustainable pattern. As @FremyCompany points out we're already doing this in practice for colors, so this would mean we'd now need to cascade four values per color: :visited regular, :visited !override, unvisited regular, unvisited !override). We should IMO avoid this completely.

Oh, indeed, it can't be done very much. Note that I'm still hopeful we can fixed the :visited problem (unfortunately Emil left Google, and Alex has been very busy with other things), so we'd ideally just go back to the two cascaded values rather than four. ^_^


That all said, if you do think that revert-at-computed-value-time is fine after all, then I guess we don't need to solve anything with !override and we'll be fine.

@andruud
Copy link
Member Author

andruud commented May 26, 2020

However, I was under the impression that we wanted a way to disable forced colors for entire subtrees, in which case !override doesn't sound very practical?

That's what f-c-a did, but it wasn't necessarily what was required. If you can target the subtree with selectors, you can hit all the necessary color properties with !override, and achieve the same thing.

Fair enough.

Why cascade !override separately if forced-color-adjust doesn't exist? If forced-color-adjust doesn't exist, we're automatically not depending on computed-value time anymore, so !override might as well be a normal priority in the cascade.

I don't understand the question; !override and f-c-a have nothing to do with each other.

We can't have !override be another priority level; it's meant to be used in the UA stylesheet to implement forced-colors mode, after all, and it wouldn't make sense to have those rules always win. It's also meant to be used by authors to override the UA stylesheet where necessary, and it seems like it would be bad for usability to have it mess up the author's normal specificity when forced-colors mode isn't active.

OK, I'll try to explain again. If there is no property that controls whether or not forced-colors mode is active on a given element at computed value time, then we can make all the decisions we need to make cascade time. When we're considering an !override declaration for entry into the cascade, forced colors mode is either active or not at that time, regardless of the computed value of anything, hence we can just discard the !override declaration if forced-colors mode isn't active. It could be prioritized like (extending the list in css-cascade-4):

  1. Author override declarations [new]
  2. User override declarations [new]
  3. User agent override declarations [new]
  4. Transition declarations
  5. Important user agent declarations
  6. Important user declarations
  7. Important author declarations
  8. Animation declarations
  9. Normal author declarations
  10. Normal user declarations
  11. Normal user agent declarations

Then you'd get the behavior you want, without cascading two values for each property. Cascading multiple values sounds like something you'd want to do precisely because you want to make a decision about value A or value B "later", based on the computed value of something maybe, hence why I mentioned forced-color-adjust in relation to !override.

It's almost midnight here, so maybe I'm missing something obvious which prevents the above from working. Hope not. Personally I'd avoid cascading multiple values like the plague, and not even consider it as a list resort. Even if we can fix :visited (which would be very nice <3).

That all said, if you do think that revert-at-computed-value-time is fine after all, then I guess we don't need to solve anything with !override and we'll be fine.

Yeah, I think there's a good way to deal with issues (if they ever happen) via the invalid at computed-value time concept. Didn't think of that at first.

@tabatkins
Copy link
Member

OK, I'll try to explain again. If there is no property that controls whether or not forced-colors mode is active on a given element at computed value time, then we can make all the decisions we need to make cascade time.

Ah, sorry, you're right, yes. Just flew over my head. Indeed, if we just pretend !override is a no-op normally, but puts the declaration in the !override origin when forced-colors mode is on, that would work.

@andruud
Copy link
Member Author

andruud commented Jun 2, 2020

@tabatkins Close the issue again? Unless there's more to discuss here? 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants