Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

[css-ui] Verify caret-color animations #1162

Merged
merged 4 commits into from
Dec 15, 2016

Conversation

mrego
Copy link
Member

@mrego mrego commented 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.


This change is Reviewable

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.
@syncbot
Copy link
Collaborator

syncbot commented Dec 14, 2016

Automatic validation checks of commit 7af5a49 passed.

@mrego mrego requested a review from frivoal December 14, 2016 15:27
@mrego mrego self-assigned this Dec 14, 2016
test(
function(){
var textarea = document.getElementById("textarea");
assert_equals(getComputedStyle(textarea).caretColor, 'rgb(255, 0, 0)');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point of this line and of caret-color: red higher up in the test? It seems to me that we could just drop them and still test what the assert claims this test is about.

test(
function(){
var textarea = document.getElementById("textarea");
assert_equals(getComputedStyle(textarea).caretColor, 'rgb(255, 0, 0)');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point of this line and of caret-color: red higher up in the test? It seems to me that we could just drop them and still test what the assert claims this test is about.

@frivoal
Copy link

frivoal commented Dec 14, 2016

Seems to me that both test contain a small unnecessary part. Other than that, they look good to me.

@syncbot
Copy link
Collaborator

syncbot commented Dec 14, 2016

Automatic validation checks of commit 846cbb9 passed.

@mrego
Copy link
Member Author

mrego commented Dec 14, 2016

Yeah those lines were not needed, uploaded a new version without them.

BTW, this also depends on w3c/csswg-drafts#566 too (like #1161).

@frivoal
Copy link

frivoal commented Dec 15, 2016

BTW, this also depends on w3c/csswg-drafts#566 too

Does it? CSS-UI says that caret-color computes values the same was as the color property. The color property says that the computed value of color keywords is an rgb() value. css-animations says that animations operate by changing the computed value.

Both css-ui and css-color say that using fairly poor phrasing, as we're discussing in w3c/csswg-drafts#741, but the intent is still unambiguous for this case.

It seems to me that the test is valid even before we decide that resolved values are used values for colors as argued in w3c/csswg-drafts#566. Am I missing something?

@mrego
Copy link
Member Author

mrego commented Dec 15, 2016

It's basically the first assert:

        assert_equals(getComputedStyle(textarea).caretColor, 'rgb(255, 0, 255)');

It's checking the resolved style of auto (in 019 test) and currentcolor (in 020).
Without w3c/csswg-drafts#566 resolved this assert would be something like (in 019 test):

        assert_equals(getComputedStyle(textarea).caretColor, 'auto');

@frivoal
Copy link

frivoal commented Dec 15, 2016

You're right, I missed that. I think we could (should) simplify the test and remove that dependency on w3c/csswg-drafts#566 by removing this:

        player.currentTime = 0;
        assert_equals(getComputedStyle(textarea).caretColor, 'rgb(255, 0, 255)');

and (unrelated to the w3c/csswg-drafts#566, but still not necessary):

        player.currentTime = 10;
        assert_equals(getComputedStyle(textarea).caretColor, 'rgb(0, 255, 0)');

from both tests, and only keeping player.currentTime = 5 and the following assert, which is what really matters.

@syncbot
Copy link
Collaborator

syncbot commented Dec 15, 2016

Automatic validation checks of commit 8d3b746 passed.

@mrego
Copy link
Member Author

mrego commented Dec 15, 2016

@frivoal good idea! I've removed the unneeded lines in the new version.

@syncbot
Copy link
Collaborator

syncbot commented Dec 15, 2016

Automatic validation checks of commit e9bd02e passed.

@frivoal frivoal merged commit b647e82 into w3c:master Dec 15, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants