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-color-adjust-1] Disallow repetition of color-scheme keywords? #3848

Closed
tabatkins opened this issue Apr 18, 2019 · 20 comments
Closed

[css-color-adjust-1] Disallow repetition of color-scheme keywords? #3848

tabatkins opened this issue Apr 18, 2019 · 20 comments
Labels

Comments

@tabatkins
Copy link
Member

Currently 'color-scheme' is defined to allow repeated keywords, like color-scheme: light light, and the repetitions are just dropped from the computed value.

I copied this from Rune's initial spec, but I'm not sure if we want it; we don't usually allow meaningful things to be repeated. Would it be okay to just call this invalid?

@frivoal
Copy link
Collaborator

frivoal commented Apr 21, 2019

I don't see a useful meaning to it, so I'd support dropping this.

@emilio
Copy link
Collaborator

emilio commented Apr 21, 2019

I suspect @lilles did that because if we allow parsing arbitrary identifiers for future compat, then you need to keep some kind of hash set of strings or such while parsing the property, to know if there are duplicates (since the engine doesn't have any internal representation for those)...

But I'm not sure why parsing arbitrary identifiers is ok, it seems undesirable, at least from the @supports and such point-of-view.

@tabatkins
Copy link
Member Author

But I'm not sure why parsing arbitrary identifiers is ok

Because CSS isn't super-great at handling properties that are additive sets in the first place, let alone ones that'll expand over time. It would really suck if you designed your page to handle all the color schemes, including the new sepia one just added in color-adjust-2, but an older UA drops the property and you end up supporting none of them, because sepia isn't recognized.

@emilio
Copy link
Collaborator

emilio commented Apr 21, 2019

Well, I mean, you'd use something like color-scheme: light; color-scheme: light sepia; right?

@emilio
Copy link
Collaborator

emilio commented Apr 21, 2019

Otherwise there's no way to know if the UA actually supports the sepia color-scheme. @supports (color-scheme: whatever) would return true.

@tabatkins
Copy link
Member Author

Even if the UA knows about sepia, there's no guarantee that it supports that value, and @supports wouldn't help you there. Luckily, color-scheme doesn't really care; it's all about giving the UA permission to use a particular color scheme. Unless you use only, you don't have any control over the scheme in the first place. (And even then, there's still no guarantee that your desired scheme is supported by the UA, regardless of whether it's recognized.)

@emilio
Copy link
Collaborator

emilio commented Apr 21, 2019

Ah, ok, fine then, sorry for the noise :)

@lilles
Copy link
Member

lilles commented Apr 21, 2019

We do allow repetition of property names in transition-property, and names in animation-name, for instance.

@emilio
Copy link
Collaborator

emilio commented Apr 21, 2019

They don't go away in the computed style as mentioned on the original comment on this issue though, right? I think Rune's argument about consistency with these other properties makes sense.

@lilles
Copy link
Member

lilles commented Apr 23, 2019

I am fine with keeping all names in the computed style. The current draft requires the order has to be kept as well. I drafted the way I did for computed style mainly to make it efficient to evaluate the used value in the implementation without recomputing style or used values for on preferred color scheme changes.

@tabatkins
Copy link
Member Author

I was wondering about that - your spec contained a comment about being able to collapse the computed value down to a bitset, but it also made order important for only usage, so I wasn't sure how you were planning to make that work. ^_^

@emilio
Copy link
Collaborator

emilio commented Apr 24, 2019

Yeah, storing it as a bitset is also incompatible with preserving unknown identifiers, afaict.

FWIW, the way will-change and such is implemented in Firefox is keeping the known properties in a bitfield, and the specified value in a separate array. So you pay a bit more memory (on byte) more than the array of names, but get fast checking:

https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/servo/components/style/values/specified/box.rs#669

But it's not clear to me that's what @lilles refers to, necessarily... Not sure how not keeping duplicated names helps to handle color scheme changes dynamically. I'd be curious to know though :)

@lilles
Copy link
Member

lilles commented May 9, 2019

@tabatkins @emilio never mind the bitset thing. It assumed order of the known color-schemes didn't matter.

Currently, the spec says only to keep recognized color-schemes. Doing the implementation here, I'm currently keeping all as specified. Should we just let computed style be 'auto' or optional 'only' followed by the color schemes in the order they were specified?

WDYT?

@emilio
Copy link
Collaborator

emilio commented May 10, 2019

I think keeping them as specified is better, since it matches will-change and similar properties that right now take an arbitrary amount of idents. Any reason not to do that?

@lilles
Copy link
Member

lilles commented May 13, 2019

Sounds good to me.

@fantasai
Copy link
Collaborator

The benefit of collapsing down repetitions is to facilitate computed value comparisons, which is something we do for animations and transitions. It might not matter so much in this particular property, but we should set a good precedent for future properties.

@lilles
Copy link
Member

lilles commented May 22, 2019

@fantasai Your proposal is to allow repetitions, but only keep the first instance of a repeated color scheme, so that specified -> computed becomes:

light dark blah light -> light dark blah
dark dark light -> dark light

?

@emilio
Copy link
Collaborator

emilio commented May 22, 2019

which is something we do for animations and transitions

Where?

document.body.style.transitionProperty = "foo, bar, baz, bar"
"foo, bar, baz, bar"
document.body.style.transitionProperty
"foo, bar, baz, bar"
getComputedStyle(document.body).transitionProperty
"foo, bar, baz, bar"

I'd rather not make specified and computed value differ unnecessarily.

@AmeliaBR
Copy link
Contributor

@emilio the repeated values in transition-property have an actual meaning and impact, because they affect the interpretation of repeats in the other transition-* properties, so they aren't ignored and can't be collapsed.

The general rule about simplifying computed style to not include redundant content is useful for animations and transitions in the sense that it makes it easier for the animation engines to know if two values are different & how to match them up for interpolating between them.

But that said, I don't think animations and transitions are important for this property. And I can't actually think of another property that allows arbitrary repeats of tokens but collapses them in the computed value. @fantasai do you have specific examples?

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Disallow repetition of color-scheme keywords?, and agreed to the following:

  • RESOLVED: computed value of color-scheme match its specified value
The full IRC log of that discussion <dael> Topic: Disallow repetition of color-scheme keywords?
<dael> github: https://github.com//issues/3848
<futhark> I was trying but Tab can present
<dael> TabAtkins: for future extensibility we allowed arbitrary keywords and they're ignored. Question is what happens when you repeat color-scheme keyword? We don't want to disallow, but do we keep it in computed value? Collapse along the way?
<dael> TabAtkins: No strong argument either way
<dael> TabAtkins: originally thoguth there was an efficiency argument but that's not true if trying to preserve unknown. I think conclusion is keep the same and don't simplify.
<dael> TabAtkins: Just have computed value = specified value
<futhark> I’m fine with either, it’s just that dropping duplicates means having to keep track of them during parsing
<dael> astearns: Any comments?
<futhark> Which requires a hash map or something
<dael> astearns: So close no change?
<dael> TabAtkins: I don't recall current state
<dael> TabAtkins: Let me look
<dael> TabAtkins: It would be changing spec
<fantasai> Proposal is to resolve no change [in the value] :)
<dael> astearns: That computed is same as specified value
<dael> TabAtkins: Yes
<dael> astearns: Obj to computed value of color-scheme match its specified value?
<dael> RESOLVED: computed value of color-scheme match its specified value

@lilles lilles closed this as completed in bfe5c78 Sep 13, 2019
lilles added a commit that referenced this issue Sep 13, 2019
[css-color-adjust-1] Don't drop repeated keywords from computed color-scheme. Issue #3848.
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

7 participants