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-ui-3] caret-color animations, 'auto' and 'currentColor' #781

Closed
frivoal opened this issue Dec 1, 2016 · 21 comments
Closed

[css-ui-3] caret-color animations, 'auto' and 'currentColor' #781

frivoal opened this issue Dec 1, 2016 · 21 comments
Assignees
Labels
Closed Rejected as Wontfix by Editor Discretion Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-ui-3

Comments

@frivoal
Copy link
Collaborator

frivoal commented Dec 1, 2016

The auto and currentColor values of the caret-color property compute to themselves as a keywords, rather than to an RGB(A) value, because otherwise that would mess with inheritance.

However, (unless I am misunderstanding how transitions and animations work) this also means that you cannot smoothly transition or animate betweenauto or currentColor and another color.

While it is not the end of the world, it seems a little unfortunate. Code like this for example, while probably not a major use case, doesn't look unreasonable, and it'd be nicer if the transition worked without discontinuity:

.foo {
  color: initial; /* or whatever */
  caret-color: auto;
  transition: all 1s;
}
.foo:hover {
  color: green;
  caret-color: lime;
}

Preserving sanity of inheritance is much more important than that, but @mrego ’s first try and implementing caret-color in Blink actually got both: for inheritance and getComputedStyle() purposes, auto and currentColor compute to themselves as keywords, but at the same time, they still animate.

Is the ability to do that unique to Blink's internals, or can other browsers do it too? Do we have a concept in the spec that allows to express this? If yes to both, should we do it? Did I misunderstand something?

@frivoal frivoal self-assigned this Dec 1, 2016
@mrego
Copy link
Member

mrego commented Dec 1, 2016

but at the same time, they still animate

Sorry but I was wrong, my initial implementation animates from auto to "lime" only in transitions, but not in animations.

Anyway I agree it'd be nice to be able to find a good solution for this.

@mrego
Copy link
Member

mrego commented Dec 1, 2016

Sorry for coming back to this question, but would it be possible that the computed value of currentColor and auto would be a rgb() (like it happens for the rest of color properties)?
If we add some text on css-ui-3 spec similar to the one in css-color-4:

This happens at used-value time, which means that if the value is inherited, it’s inherited as currentcolor, not as the value of the color property, so descendants will use their own color property to resolve it.

That would probably simplify the implementations, just having to accept auto as value for parsing, but behaving like a regular color in most of the situations.
Maybe it doesn't make sense either.

@upsuper
Copy link
Member

upsuper commented Dec 2, 2016

would it be possible that the computed value of currentColor and auto would be a rgb() (like it happens for the rest of color properties)?

No. Computed value of currentcolor is the keyword currentcolor. The text you referenced from CSS Color 4 also indicates this. It is only resolved at used-value time, not computed-value time. And this is necessary for color properties which are inherited by default (text-emphasis-color and -webkit-text-*-color).

As for auto, I am not really happy with adding another computed-value time keyword to <color>. It would be hard to implement as I've mentioned in a comment of #364.

I think, auto should be defined to not be interpolatible with <color> values, and I don't think that would be a big issue. Authors should specify currentcolor or numeric color values if they want animation on this property.

@upsuper
Copy link
Member

upsuper commented Dec 2, 2016

Making auto not interpolatible with other values also matches what we currently do for other properties, e.g. the box offset properties. I admit that it may not be ideal, but I think that's the best we can do for now.

@frivoal
Copy link
Collaborator Author

frivoal commented Dec 2, 2016

I also started on the assumption that it was not ideal but the best we can do. @mrego 's experience while implementing made me wonder if there was a way to do better. If there isn't, I think we can keep the spec as it is and close this issue.

It'd be nice to get a couple more comments from other people, but I'm leaning towards closing as no-change.

@mrego
Copy link
Member

mrego commented Dec 2, 2016

No. Computed value of currentcolor is the keyword currentcolor. The text you referenced from CSS Color 4 also indicates this. It is only resolved at used-value time, not computed-value time. And this is necessary for color properties which are inherited by default (text-emphasis-color and -webkit-text-*-color).

Mmm, so let me ask a few questions.

In the browsers I've tested if you use getComputedStyle() of a color property that has been set to currentcolor they return for example rgb(0,0,0). Is that the expected behavior? Or it's just a legacy thing that shouldn't be applied to new properties (like caret-color)?

Apart from that, reading your comment I understand that the following CSS won't be interpolable:

  .foo {
    caret-color: auto;
    transition: caret-color 1s;
  }
  .foo:hover {
    caret-color: lime;
  }

But using currentcolor it'll be interpolable and you'll see a transition:

  .foo {
    caret-color: currentcolor;
    transition: caret-color 1s;
  }
  .foo:hover {
    caret-color: lime;
  }

Is that the expected behavior for caret-color?

Thanks!

@upsuper
Copy link
Member

upsuper commented Dec 2, 2016

getComputedStyle() is not supposed to return the computed value for color properties, because that may break web compat, see #566. In addition, there is currently no stable syntax to describe a color combined from numeric color and currentcolor in CSS.

This is a testcase for whether currentcolor is handled as a computed-value time keyword: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4708

In this testcase, color is changing in the (paused) animation from #ff0000 to #0000ff, while another color property is changing from currentcolor to #00ff00. You can see that

  • Chrome shows two blocks both have color rgb(64, 128, 64), which indicates that currentcolor is handled as computed-value time keyword.
  • Firefox 50 as well as Safari shows two blocks with color rgb(128, 128, 0) which means currentcolor becomes a numeric color at computed-value time.
  • Firefox 52 shows one block with rgb(128, 128, 0) and the other rgb(64, 128, 64), because that version of Firefox has supported computed-value time currentcolor for border-color, but not background-color.

@upsuper
Copy link
Member

upsuper commented Dec 2, 2016

Is that the expected behavior for caret-color?

Yes.

@mrego
Copy link
Member

mrego commented Dec 2, 2016

Thanks for the detailed explanation!

getComputedStyle() is not supposed to return the computed value for color properties, because that may break web compat, see #566.

So now thinking on caret-color, I understand that for auto, getComputedStyle() should return auto.
But what it should return for currentcolor? A numeric value like the rest of color properties?

@upsuper
Copy link
Member

upsuper commented Dec 2, 2016

So now thinking on caret-color, I understand that for auto, getComputedStyle() should return auto.

It depends on what we would say in the CSSOM spec for resolved value of this property. If the resolved value of caret-color is its used value, you would return numeric value for all values of it including auto. If it says something else, then something else.

I don't have strong opinion here what the behavior should be, but I tend to think currentcolor should be resolved to used value like other color properties, and making auto follow that seems to be simplier for wording of the spec.

@frivoal
Copy link
Collaborator Author

frivoal commented Dec 3, 2016

Some tests for caret-color will need updating if #566 is resolved the way you suggest and the resolved value of color properties including caret-color should be the used value, as they currently follow the current test of CSSOM (which doesn't have color properties in the list of properties for which the resolved value is the used value).

Not objecting, just writing it down to remember:
https://github.com/w3c/csswg-test/blob/master/css-ui-3/caret-color-009.html
https://github.com/w3c/csswg-test/blob/master/css-ui-3/caret-color-013.html

@mrego
Copy link
Member

mrego commented Dec 7, 2016

If we can make auto behaves as currentColor, I'm wondering if we still need the auto value then?
What would be the difference between auto and currentColor?

@mrego
Copy link
Member

mrego commented Dec 9, 2016

I guess the only difference between currentColor and auto would be this part of the spec:

User agents may automatically adjust the color of caret to ensure good visibility and contrast with the surrounding content, possibly based on the currentColor, background, shadows, etc.

@upsuper
Copy link
Member

upsuper commented Dec 9, 2016

Yes. My patch to implement caret-color just makes auto an alias of currentcolor since Gecko doesn't really have any different mechanism to adjust the color right now. I'm not sure if any browser else has that. If there is any, I guess we should keep this value.

@frivoal
Copy link
Collaborator Author

frivoal commented Dec 12, 2016

I guess the only difference between currentColor and auto would be this part of the spec:
User agents may automatically adjust the color of caret to ensure good visibility and contrast with the surrounding content, possibly based on the currentColor, background, shadows, etc.

Right, that's what it's for. It ought to be pretty cheap to implement if you don't want to do anything special, and it opens up the door for smarter behavior or aligning with whatever unusual thing an OS might have as a native behavior.

It also enables some extra flexibility when/if we later add more properties that relate to the caret. For instance, level4 proposes caret-shape: block](https://drafts.csswg.org/css-ui-4/#caret-shape), and in that case it might be good for the auto caret-color to be neither currentColor nor the background color, but something that contrasts with both

Anyway, I don't think this is essential, but it seems nice to have a low cost.

@mrego
Copy link
Member

mrego commented Dec 12, 2016

In Chromium we're not planning to implement anything special for auto at this point either.

If the CSS WG is fine with those changes I'll update the tests and the Blink implementation.

Regarding the animations, considering that the suggested changes to make auto similar to currentColor are accepted. Would it be possible to interpolate from auto to a given color or not?

@upsuper
Copy link
Member

upsuper commented Dec 12, 2016

If we are keeping the independent auto keyword value, I would be against making it interpolate with other colors, since that would add complexity to any implementation who actually wants to do something different with auto. Although no impl currently plans to do that, why do we want to set extra trouble blocking others to implement it if we think this could be useful?

@frivoal
Copy link
Collaborator Author

frivoal commented Dec 14, 2016

@upsuper I agree with that. I am not an animation expert, but I believe the spec is correctly written if that's what we want. Do you agree, or are there any change / clarification that would be appropriate?

@upsuper
Copy link
Member

upsuper commented Dec 14, 2016

I think the current text of this spec should be fine for this. Interpolating between currentcolor and others should be handled by CSS Transitions, not here, and auto is not interpolatible with others since it is not mentioned in color animation type.

@mrego
Copy link
Member

mrego commented Dec 14, 2016

Thank you very much for the detailed answers.

I guess the only change we need on the tests and maybe on the specs is related to use the "used value" as "resolved value" for getComputedStyle().
I'll modify the tests in csswg-tests repo and I'll update Blink implementation to follow this too.

mrego added a commit to mrego/csswg-test that referenced this issue Dec 14, 2016
As discussed on issue w3c/csswg-drafts#781, I'm updating the tests
so that caret-color behaves similar to the rest of color properties.
mrego added a commit to mrego/csswg-test that referenced this issue Dec 14, 2016
As discussed on issue w3c/csswg-drafts#781, I'm updating the tests
so that caret-color behaves similar to the rest of color properties.
mrego added a commit to mrego/csswg-test that referenced this issue Dec 14, 2016
Added 2 new tests to check that "caret-color: auto" isn't interpolatible,
but "caret-color: currentcolor" is.

See w3c/csswg-drafts#781 for more information.
@frivoal
Copy link
Collaborator Author

frivoal commented Dec 15, 2016

@astearns @rossen:

So the consensus on this thread is no change, and the people who raised the issue (me and @mrego) are OK with this.

Given that the spec is in CR and for DoC purposes, we probably want a group decision anyway rather than a decision by the editor, but at the same time I am not sure this is worth teleconf time given the apparent agreement.

What do we do? Can you do a Call for Consensus to www-style, or should I Agenda+ anyway, or is a decision by the Editor actually OK even though the spec is in CR?

gsnedders pushed a commit to jgraham/css-test-built that referenced this issue Jan 4, 2017
As discussed on issue w3c/csswg-drafts#781, I'm updating the tests
so that caret-color behaves similar to the rest of color properties.

Build from revision 5b3a864679e8b965513bd285487b7af640fb3f30
gsnedders pushed a commit to jgraham/css-test-built that referenced this issue Jan 4, 2017
Added 2 new tests to check that "caret-color: auto" isn't interpolatible,
but "caret-color: currentcolor" is.

See w3c/csswg-drafts#781 for more information.

Build from revision 7af5a49dc68b5efefa3dd93012a07a4fb9392968
gsnedders pushed a commit to jgraham/css-test-built that referenced this issue Jan 4, 2017
As discussed on issue w3c/csswg-drafts#781, I'm updating the tests
so that caret-color behaves similar to the rest of color properties.

Build from revision 5b3a864679e8b965513bd285487b7af640fb3f30
gsnedders pushed a commit to jgraham/css-test-built that referenced this issue Jan 5, 2017
As discussed on issue w3c/csswg-drafts#781, I'm updating the tests
so that caret-color behaves similar to the rest of color properties.

Build from revision 5b3a864679e8b965513bd285487b7af640fb3f30
@frivoal frivoal added Closed Rejected as Wontfix by Editor Discretion Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. and removed Needs Process Help labels Jan 9, 2017
@frivoal frivoal closed this as completed Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Rejected as Wontfix by Editor Discretion Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-ui-3
Projects
None yet
Development

No branches or pull requests

3 participants