Closed Bug 1324593 Opened 6 years ago Closed 5 years ago

transition-timing-function preview always linear

Categories

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

50 Branch
defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: alberts, Assigned: pbro, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(2 files, 6 obsolete files)

Attached video 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.
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.
Mentor: pbrosset
Keywords: good-first-bug
OS: Windows 10 → All
Hardware: x86_64 → All
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
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Attached patch linear-transition.patch (obsolete) — Splinter Review
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
As you are not accepting review request for now, flagging a needinfo
Flags: needinfo?(pbrosset)
(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.
Flags: needinfo?(pbrosset)
Attached patch cubicbezierwidget.patch (obsolete) — Splinter Review
Extremely sorry for misinterpreting your suggestion in earlier comment.
Hope this works now :)
Attachment #8821915 - Attachment is obsolete: true
Attachment #8824143 - Flags: review?(pbrosset)
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 on attachment 8824143 [details] [diff] [review]
cubicbezierwidget.patch

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

Please see comment 7.
Attachment #8824143 - Flags: review?(pbrosset)
Inspector bug triage, filter on CLIMBING SHOES.
Priority: -- → P2
Duplicate of this bug: 1328002
Attached patch cubicbezierwidget.patch (obsolete) — Splinter Review
Hi Patrick, 
Hope this patch helps.

Thanks
Attachment #8824143 - Attachment is obsolete: true
Attachment #8829145 - Flags: review?(pbrosset)
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.
Attachment #8829145 - Flags: review?(pbrosset)
Attached patch cubicbezierwidget.patch (obsolete) — Splinter Review
Sorry about missing that, Hope this works now :)
Attachment #8829526 - Flags: review?(pbrosset)
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 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.
Attachment #8829145 - Attachment is obsolete: true
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.
Attachment #8829526 - Flags: review?(pbrosset) → review-
Attached patch cubicbezierwidget.patch (obsolete) — Splinter Review
Here's the patch I was talking about.
Please apply it locally and give it a try. Let me know what you think.
Attachment #8829526 - Attachment is obsolete: true
Flags: needinfo?(3ugzilla)
(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 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?
Attachment #8831143 - Flags: review?(jdescottes)
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
Flags: needinfo?(3ugzilla)
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).
Attachment #8831143 - Flags: review?(jdescottes) → review+
(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.
Attached patch cubicbezierwidget.patch (obsolete) — Splinter Review
Assignee: 3ugzilla → pbrosset
Attachment #8831143 - Attachment is obsolete: true
Comment on attachment 8834006 [details] [diff] [review]
cubicbezierwidget.patch

As discussed with jdescottes on IRC, carrying R+ over.
Attachment #8834006 - Flags: review+
Gree try build. Just corrected an eslint error.
Let's check this in.
Attachment #8834006 - Attachment is obsolete: true
Attachment #8834379 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61b6e09d2f2a
fixed transition-timing-function preview linear issue; r=pbro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/61b6e09d2f2a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.