1324593 - transition-timing-function preview always linear

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[Albert Scheiner [:alberts]]

Attachment: ff-timing-function.mp4

When opening the transition-timing-function option UI there is a little preview at the bottom, where a dot runs along a line to mimik the chosen transition timing. When I just tried it I only got a linear timing preview no matter what I choose.

Comment 1

[Patrick Brosset <:pbro>]

Good catch. Thank you Albert for filing this.

Looking at the code, I think this might have been broken for a very long time. Unfortunately, it seems we do not have automated tests for this, and no one noticed (or reported the issue).
So, this is a regression but I don't know when it appeared.

The issue is here: http://searchfox.org/mozilla-central/rev/46ded325e8f60b3d4c731b27c31d383911056a38/devtools/client/shared/widgets/CubicBezierWidget.js#421
When the graph is re-drawn, we ask the little preview widget to update too, but the wrong parameter is sent:

this.bezierCanvas.bezier + ""

This does not return the right cubic-bezier string.
However, this does:

this.bezierCanvas.bezier.toString()

So, this looks like an extremely easy fix, one line, replace `+ ""` with `.toString()`

However, the harder part will be to provide a test for this so we don't regress this again in the future. This needs to be an integration test that asserts that when the graph changes the preview changes too.

If the person wanting to fix this isn't comfortable with the testing part, that's fine, we can still land the fix first and then do another patch to land the test.
Make sure you go through our contribution guide first: https://wiki.mozilla.org/DevTools/Hacking

Some details for how to add a test:

We have multiple tests for the widget already:  /devtools/client/shared/test/browser_cubic-bezier-[01-06].js

I suggest copying this file /devtools/client/shared/test/browser_cubic-bezier-03.js
to /devtools/client/shared/test/browser_cubic-bezier-07.js

There is some code in there to programmatically change the drawn cubic-bezier. The test should just instantiate the widget, then change the cubic-bezier, and then check that the preview in TimingFunctionPreviewWidget is correct.

Comment 2

[[:Towkir] Ahmed]

Hi Patrick Brosset,
I am going to submit a patch soon, meanwhile I will look forward on the testing part. In case I can't handle that, I will submit the fix only.

Thanks

Comment 3

[[:Towkir] Ahmed]

Attachment: linear-transition.patch

Hi Patrick,
I tried to copy the file with this command but that did not work:
hg copy devtools/client/shared/test/browser_cubic-bezier-3.js devtools/client/shared/test/browser_cubic-bezier-7.js

I could create another file named browser_cubic-bezier-7.js and copy all the contents and add that as a new file but I know that is not the way to do this, It would be nice if you could refer me some docs or instruct the proper way to copy a file to a new one with a new name.

Hope this helps

Comment 4

[[:Towkir] Ahmed]

As you are not accepting review request for now, flagging a needinfo

Comment 5

[Patrick Brosset <:pbro>]

(In reply to [:Towkir] Ahmed from comment #3)
> Created attachment 8821915 [details] [diff] [review]
> linear-transition.patch
Hm, I think there's a typo in this patch. You're doing a string concatenation here:

this.timingPreview.preview(this.bezierCanvas.bezier + '.toString()');

what's happening here is you're concatenating the string value of this.bezierCanvas.bezier to another string '.toString()'. This won't have the expected result.

Try this instead:

this.timingPreview.preview(this.bezierCanvas.bezier.toString());

> Hi Patrick,
> I tried to copy the file with this command but that did not work:
> hg copy devtools/client/shared/test/browser_cubic-bezier-3.js
> devtools/client/shared/test/browser_cubic-bezier-7.js

Well, the file is not browser_cubic-bezier-3.js but browser_cubic-bezier-03.js (note the extra 0 in the filename).
So this is most probably why the command did not work for you.

Please make sure you use a 0 in the new filename too: browser_cubic-bezier-07.js instead of browser_cubic-bezier-7.js

> I could create another file named browser_cubic-bezier-7.js and copy all the
> contents and add that as a new file but I know that is not the way to do
> this, It would be nice if you could refer me some docs or instruct the
> proper way to copy a file to a new one with a new name.

Creating a new file is perfectly fine here. hg copy is good when you want the mercurial history of the source file to be associated with the copied file too. Here we don't care much about this. So you might as well just create a new file from scratch, with the right name and then copy the code you need from browser_cubic-bezier-03.js into it.

Comment 6

[[:Towkir] Ahmed]

Attachment: cubicbezierwidget.patch

Extremely sorry for misinterpreting your suggestion in earlier comment.
Hope this works now :)

Comment 7

[Patrick Brosset <:pbro>]

Thank you for this new patch.
I tested it but realized that the fix didn't work still.
After investigation I realized that this came from an issue I had not seen before.

In bug 1267414, the restartAnimation function of TimingFunctionPreviewWidget was changed as seen here:
https://hg.mozilla.org/mozilla-central/diff/76d556ef9180/devtools/client/shared/widgets/CubicBezierWidget.js#l1.226

And now, instead of a CSS animation, the dot animates using a script animation.
But the thing is, to preview a new timing function, we do this:
this.dot.style.animationTimingFunction = value;

See http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/devtools/client/shared/widgets/CubicBezierWidget.js#771

So, we set a new animation-timing-function css value to the element, but since the element is *not* animated with a css animation anymore, this won't work.

Instead, we should remove this line entirely, and pass the new timing function as an argument to restartAnimation.
Then inside restartAnimation, we can easily use it like so:

  restartAnimation: function (timingFunction) {
    // Just toggling the class won't do it unless there's a sync reflow
    this.dot.animate([
      { left: "-7px", offset: 0 },
      { left: "143px", offset: 0.25 },
      { left: "143px", offset: 0.5 },
      { left: "-7px", offset: 0.75 },
      { left: "-7px", offset: 1 }
    ], {
      duration: (this.PREVIEW_DURATION * 2),
      fill: "forwards",
      ease: timingFunction
    });

So, to summarize, your fix to the .toString() part is fine, but it's not enough. This extra change is needed too.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8824143 [details] [diff] [review]
cubicbezierwidget.patch

Review of attachment 8824143 [details] [diff] [review]:
-----------------------------------------------------------------

Please see comment 7.

Comment 9

[Julian Descottes [:jdescottes]]

Inspector bug triage, filter on CLIMBING SHOES.

Comment 11

[[:Towkir] Ahmed]

Attachment: cubicbezierwidget.patch

Hi Patrick, 
Hope this patch helps.

Thanks

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8829145 [details] [diff] [review]
cubicbezierwidget.patch

Review of attachment 8829145 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there. See my comment below.

::: devtools/client/shared/widgets/CubicBezierWidget.js
@@ +771,1 @@
>        this.restartAnimation();

You forgot to pass the timing function value to restartAnimation here.
You added a parameter to the restartAnimation method further below, so you need to pass it here, otherwise it will always be null.

Comment 13

[[:Towkir] Ahmed]

Attachment: cubicbezierwidget.patch

Sorry about missing that, Hope this works now :)

Comment 14

[[:Towkir] Ahmed]

How do I mark attachment2 [details] [diff] [review] as obsolete now ? is it okay to leave that as that is ? Or I should mark obsolete ?

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8829145 [details] [diff] [review]
cubicbezierwidget.patch

The easiest way to mark an attachment as obsolete is when you upload a new version, at the bottom of the upload page, there's a list of all existing attachment, and you can mark any of them as obsolete.

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8829526 [details] [diff] [review]
cubicbezierwidget.patch

Review of attachment 8829526 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/widgets/CubicBezierWidget.js
@@ +768,4 @@
>      clearTimeout(this.autoRestartAnimation);
>  
>      if (parseTimingFunction(value)) {
> +      this.restartAnimation(timingFunction);

Hmm, did you test this locally? I'm asking because timingFunction is not defined in this scope. The name of the variable you should pass to the restartAnimation function here is 'value'.
So that fails with a JS exception in the browser console. And the bug isn't fixed.

So I'm a bit surprised to see this patch because when you made the fix, you tested it right? 
Once you've made a code change, you need to re-build Firefox (`./mach build faster` works great for this, and is fast), and then open Firefox again (`./mach run`), and then test that your fix does work.
In this case, that means re-opening the cubic-bezier tooltip in the rule-view, and checking that the little dot that animates at the bottom of the tooltip does move according to the current curve.

So, I fixed this locally to test it out, and realized that there was something else broken. The code inside restartAnimation isn't correct anymore. We're using the web animation API now to animate the dot, but the easing isn't applied. First of all the parameter isn't `ease`, but `easing` (sorry if I said `ease` before, it was probably just off the top of my head, that's why testing locally helps!)
Also, there's a way to avoid the setTimeout altogether, we could just use an infinite animation instead.

I'll submit a patch soon to show you what I mean.

Comment 17

[Patrick Brosset <:pbro>]

Attachment: cubicbezierwidget.patch

Here's the patch I was talking about.
Please apply it locally and give it a try. Let me know what you think.

Comment 18

[Patrick Brosset <:pbro>]

Here's a try push for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90b7cc99085eeda6631d4f062c6aa731464ea3dd

Comment 19

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #18)
> Here's a try push for this patch:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=90b7cc99085eeda6631d4f062c6aa731464ea3dd
I had a text editor config problem that led to eslint errors: Expected linebreaks to be 'LF' but found 'CRLF'
Fixed, and pushed again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c9533abef6adb9c525553373cdbb9024c629ec1

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8831143 [details] [diff] [review]
cubicbezierwidget.patch

Try is green. Since I wrote the end of this patch, I think it's safer to ask for an additional review to someone else.
Julian, could you please take a look at this?

Comment 21

[[:Towkir] Ahmed]

Hi Patrick, Sorry that I could not do as per comment 16 and comment 17
I had some build issue relating to compiler and could not even test it. Looks like you have done it all. Clearing the flag. 
Thanks

Comment 22

[Julian Descottes [:jdescottes]]

Comment on attachment 8831143 [details] [diff] [review]
cubicbezierwidget.patch

Review of attachment 8831143 [details] [diff] [review]:
-----------------------------------------------------------------

Great! minor nits.

::: devtools/client/shared/test/browser_cubic-bezier-07.js
@@ +20,5 @@
> +
> +  yield previewDotReactsToChanges(w, [0.6, -0.28, 0.74, 0.05]);
> +  yield previewDotReactsToChanges(w, [0.9, 0.03, 0.69, 0.22]);
> +  yield previewDotReactsToChanges(w, [0.68, -0.55, 0.27, 1.55]);
> +  yield previewDotReactsToChanges(w, [0.25, 0.1, 0.25, 1], "ease");

nit: PREDEFINED.ease

@@ +21,5 @@
> +  yield previewDotReactsToChanges(w, [0.6, -0.28, 0.74, 0.05]);
> +  yield previewDotReactsToChanges(w, [0.9, 0.03, 0.69, 0.22]);
> +  yield previewDotReactsToChanges(w, [0.68, -0.55, 0.27, 1.55]);
> +  yield previewDotReactsToChanges(w, [0.25, 0.1, 0.25, 1], "ease");
> +  yield previewDotReactsToChanges(w, [0.42, 0, 0.58, 1], "ease-in-out");

nit: PREDEFINED["ease-in-out"]

::: devtools/client/shared/widgets/CubicBezierWidget.js
@@ +773,5 @@
>  
>    /**
>     * Re-start the preview animation from the beginning
>     */
> +  restartAnimation: function (timingFunction) {

nit: jsdoc

@@ +785,5 @@
>      ], {
> +      duration: this.PREVIEW_DURATION,
> +      easing: timingFunction,
> +      iterations: Infinity,
> +      direction: "alternate"

I guess there's no way to easily pause at each iteration while still preserving the proper timing function. That's a small behavior change but I think that's fine.

Also it was a bit unclear if the pause was part of the animation preview or not. I guess if we reintroduce it, we should make it clear that it's not part of the animation preview (by fading out the dot for instance).

Comment 23

[Patrick Brosset <:pbro>]

(In reply to Julian Descottes [:jdescottes] from comment #22)
> 
> @@ +785,5 @@
> >      ], {
> > +      duration: this.PREVIEW_DURATION,
> > +      easing: timingFunction,
> > +      iterations: Infinity,
> > +      direction: "alternate"
> 
> I guess there's no way to easily pause at each iteration while still
> preserving the proper timing function. That's a small behavior change but I
> think that's fine.
> 
> Also it was a bit unclear if the pause was part of the animation preview or
> not. I guess if we reintroduce it, we should make it clear that it's not
> part of the animation preview (by fading out the dot for instance).

I spent some time thinking about this and ended up with the following changes:

    // The animation consists of a few keyframes that move the dot to the right of the
    // container, and then move it back to the left.
    // It also contains some pause where the dot is semi transparent, before it moves to
    // the right, and once again, before it comes back to the left.
    // The timing function passed to this function is applied to the keyframes that
    // actually move the dot. This way it can be previewed in both direction, instead of
    // being spread over the whole animation.
    this.dot.animate([
      { left: "-7px", opacity: .5, offset: 0 },
      { left: "-7px", opacity: .5, offset: .19 },
      { left: "-7px", opacity: 1, offset: .2, easing: timingFunction },
      { left: "143px", opacity: 1, offset: .5 },
      { left: "143px", opacity: .5, offset: .51 },
      { left: "143px", opacity: .5, offset: .7 },
      { left: "143px", opacity: 1, offset: .71, easing: timingFunction },
      { left: "-7px", opacity: 1, offset: 1 }
    ], {
      duration: this.PREVIEW_DURATION * 2,
      iterations: Infinity
    });

This way: 
- there is now a pause between each movement of the dot
- the dot is faded during these pauses
- the dot has the right timing function whether it goes to the right or to the left. Previously, with the direction "alternate", it would reverse the timing-function when the dot went back to the left.
Do you want to review this again?

This also means that I had to change the test to this instead:

  let goingToRight = animations[0].effect.getKeyframes()[2];
  is(goingToRight.easing, expectedEasing,
     `The easing when going to the right was set correctly to ${coords}`);

  let goingToLeft = animations[0].effect.getKeyframes()[6];
  is(goingToLeft.easing, expectedEasing,
     `The easing when going to the left was set correctly to ${coords}`);

Previously, it would be testing animations[0].effect.timing.easing. But now, it needs to test the easing on the right keyframes instead.

Comment 24

[Patrick Brosset <:pbro>]

Attachment: cubicbezierwidget.patch

Comment 25

[Patrick Brosset <:pbro>]

Comment on attachment 8834006 [details] [diff] [review]
cubicbezierwidget.patch

As discussed with jdescottes on IRC, carrying R+ over.

Comment 26

[Patrick Brosset <:pbro>]

Let's do a final try push and land the change after that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bf062cba0a3a209d7bd3b70b2240236d855c3f7

Comment 27

[Patrick Brosset <:pbro>]

Attachment: cubicbezierwidget.patch

Gree try build. Just corrected an eslint error.
Let's check this in.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61b6e09d2f2a
fixed transition-timing-function preview linear issue; r=pbro

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/61b6e09d2f2a