1472859 - currentColor animation breaks animation inspector

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect, P1)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: a file having a currentColor animation

Open the attachment file, open animation inspector and reload the file.

There is an error message in browser console;

 TypeError: rgba1 is undefined[Learn More] ColorPath.js:177:9

I think there is another issue on platform though. :/

Comment 1

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> I think there is another issue on platform though. :/

Never mind about this, I think I might write a wrong script there.

Comment 2

[Daisuke Akatsuka (:daisuke)]

Thanks Hiro.

Comment 3

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c936009029504dc964fe0832856d790d9ca57b4

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Ok, so the crash is caused by the same colors both on 'from' and 'to', right?  I think, as for this bug, just fixing the crash is fine.   You can leave currentColor problem in a later bug.

What I am concerned is here[1];

+        expectedValues: [
+          {
+            title: "rgb(0, 0, 255)",
+            marginInlineStart: "0%",
+          },

If I understand correctly this test case, this 'rgb(0, 0, 255)' is drawn as a text in a tooltip when user hovers on a dot in the animation inspector, right?  If so, I think we should use 'currentColor' keyword as it is, and I think using the computed RGB color to draw the graph is fine.

[1] https://hg.mozilla.org/try/rev/64cf060591dddf2bf43d60e55dc9cc13efcbad0a#l2.39

Comment 5

[Daisuke Akatsuka (:daisuke)]

Thanks for the feedback.
Yes, the reason of crash was in case of same colors.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> 
> What I am concerned is here[1];
> 
> +        expectedValues: [
> +          {
> +            title: "rgb(0, 0, 255)",
> +            marginInlineStart: "0%",
> +          },
> 
> If I understand correctly this test case, this 'rgb(0, 0, 255)' is drawn as
> a text in a tooltip when user hovers on a dot in the animation inspector,
> right?  If so, I think we should use 'currentColor' keyword as it is, and I
> think using the computed RGB color to draw the graph is fine.

Yes, that is of tooltip. 
Also, yes, I had thought so that we should use 'currentColor' as it is.
However, even if we set 'inherit' or 'unset' and so on, we get computed value(e.g. rgb(0, 255, 0)) from KeyframeEffect::GetProperties(). So, I had wanted to keep the consistency.
Also, if we send 'currentColor' to the client as it is, the color of graph should differ from actual animation since we can't get computed value in client side.

From both reasons, I had thought to change to computed value from 'currentColor' in this time.
Ideally, KeyframeEffect::GetProperties() returns both specified and computed values, then send them to client, then use them, I think.

What do you think??

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I'd prefer 'currentColor' in the tooltip since color might be also animating there.  That's said, currentColor animation with color animation doesn't work now, so it's not a big deal though.

Comment 7

[Daisuke Akatsuka (:daisuke)]

Okay, thanks! I will do along to your opinion.

Comment 8

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1472859 - Part 1: Avoid crashing which is in case the all values of keyframes are same.

Review commit: https://reviewboard.mozilla.org/r/254422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254422/

Comment 9

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1472859 - Part 2: Add test for same colors and currentcolor.

Review commit: https://reviewboard.mozilla.org/r/254424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254424/

Comment 10

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ad7b879a58d15f6e2660575599788a7a3cf1af6

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8989345 [details]
Bug 1472859 - Part 1: Avoid crashing which is in case the all values of keyframes are same.

https://reviewboard.mozilla.org/r/254422/#review261460

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8989346 [details]
Bug 1472859 - Part 2: Add test for same colors and currentcolor.

https://reviewboard.mozilla.org/r/254424/#review261462

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f94c634c79d0
Part 1: Avoid crashing which is in case the all values of keyframes are same. r=gl
https://hg.mozilla.org/integration/autoland/rev/262572362be8
Part 2: Add test for same colors and currentcolor. r=gl

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/f94c634c79d0
https://hg.mozilla.org/mozilla-central/rev/262572362be8

Comment 15

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8989345 [details]
Bug 1472859 - Part 1: Avoid crashing which is in case the all values of keyframes are same.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1399830
[User impact if declined]: If user inspects animation which has keyframes that all are same colors, animation inspector will crash.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: It has baked on Nightly for 1 day.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Low
[Why is the change risky/not risky?]: This change is one line only except tests. Also, this affects only animation inspector.
[String changes made/needed]: None

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8989345 [details]
Bug 1472859 - Part 1: Avoid crashing which is in case the all values of keyframes are same.

Low-risk fix for animation inspector, includes new tests.
Let's uplift for beta 7.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/79290fd284cf
https://hg.mozilla.org/releases/mozilla-beta/rev/e5972354da95

Comment 18

[Catalin Sasca, Desktop QA [:csasca]]

Based on comment 15, marking as qe-.