Closed Bug 1330544 Opened 7 years ago Closed 6 years ago

Animation view does not update when timing properties are changed

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Type:
defect

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: birtles, Assigned: ankita_tanwar)

References

Details

Attachments

(6 files, 3 obsolete files)

Screencast showing problem
62.72 KB, video/webm
Details
Bug_1330544_3.patch
2.18 KB, patch
Details | Diff | Splinter Review
Bug 1330544 - Part 1: Add 'direction' to the changing flag target.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1330544 - Part 2: Add 'easing' and 'fill' to the changing flag target.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1330544 - Part 3: Add test of altering properties.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Test case (updated to revised API)
637 bytes, text/html
Details
Attached file Test case (obsolete) — 
1. Open the attached test case
2. Open the DevTools panel with the animation inspector visible
3. Reload the test case
4. Observe how the animation alternates back and forth but the animation view does not change

In the test case, script changes the direction of the animation while it is in progress. However, the animation inspector view does not update.
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Hi,

I would like to work on this bug but is it possible to provide some information on where to start and exactly what output is expected in Animation view.

Thanks,
Ankita
Hi Ankita, thank you very much! Patrick and Daisuke can give you help about DevTools, I can give you some help about the Gecko/platform side.

Do you have a debug build of Firefox already?

The expected output in the Animation view is that the graph should change from a sawtooth pattern to pyramids:

From:

    /|  /|  /|  /|
   / | / | / | / |
  /  |/  |/  |/  |

to:

    /\    /\
   /  \  /  \
  /    \/    \

Normally, when an animation is changed animation mutation observers are called. This is a special API for DevTools.

In DevTools we have two parts: server and client.

The server should receive the animation changes. I believe the AnimationPlayerActor and AnimationsActor classes watch for animation mutations (one watches for additions and removals to the set of animations, and another watches for changes to its animation, I believe). This code is in devtools/server/actors/animation.js

So the first step would be to confirm that the AnimationPlayerActor is called when the direction changes. If it is not, it is probably a Gecko/platform bug and I can help you write a test and fix it.

If the server gets the change, then we need to check that it tells the client (devtools/client/animationinspector/animation-controller.js).

I think AnimationPlayerActor.onAnimationMutation would be a good place to start looking to see if it is detecting the changed 'direction' correctly.

Once you have found the bug, the next part is to write an automated test for this. This can be hard and the tests are slow to run but there are many existing tests you can use for reference. (devtools/client/animationinspector/test I think? Patrick? Daisuke?)
I hope you don't mind me assigning the bug to you. This is so that no-one else tried to work on it. If you think you won't be able to fix it, I can unassign you.
Assignee: nobody → ankita_tanwar
Status: NEW → ASSIGNED
Thank you Brian. I will try my best to fix the bug.
Attached patch Bug_1330544.patch (obsolete) — — Splinter Review 
Added condition check for direction while updating "hasChanged" value in file /devtools/server/actors/animation.js.
(In reply to Ankita Tanwar from comment #7)
> Created attachment 8840775 [details] [diff] [review]
> Bug_1330544.patch
> 
> Added condition check for direction while updating "hasChanged" value in
> file /devtools/server/actors/animation.js.
There might have been a problem when you created this patch. There are no changes listed for animation.js in there. However, there are many unrelated changes.
Can you explain how you created this patch file, so we can help you?
Hello Patrick. 

Before creating the patch I pull the changes in mozilla-central repository using SourceTree. Once the pull was complete I executed the following command:

hg export . > PatchName.patch;

And this created a .patch file in .hg folder. Guide me if I am doing it wrong.
Attached patch Bug_1330544_2.patch (obsolete) — — Splinter Review 
Added condition check for direction while updating "hasChanged" value in file /devtools/server/actors/animation.js.
Attachment #8840775 - Attachment is obsolete: true
Comment on attachment 8841910 [details] [diff] [review]
Bug_1330544_2.patch

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

Great! This seems to work fine, and is a pretty simple change, so that's great.
I don't remember why we did not include direction in there at first. But anyway, this seems like the right fix.
This made me think of bug 1151022, which you might be interested in working on after this one. Basically, the problem is when you have an infinite animation, we only display the first iteration. But if the direction is alternate, then we should really be displaying 2 iterations, otherwise you don't know that the direction changes, and you can't pause the animation in the reverse direction.
This also made me think of bug 1342335 which you are already working on. With your fix here, at least we see the animation updated in the timeline, but the scrubber disappears, which is exactly bug 1342335.

I have made one comment in the code below.

And I have one last request for this: we need a new test, or test case that will make sure this bug doesn't re-occur in the future. We have plenty of automated tests that we use to avoid regressions. So it would be great if you could add one that tests that the animation is refreshed when the direction changes.

In particular, we have this test: devtools\client\animationinspector\test\browser_animation_ui_updates_when_animation_data_changes.js
It checks that the timeline is updated when the duration, iterationCount and delay are changed.
I think we should add another test that checks the same thing when the direction changes too.

You can probably start by creating a new file by copying this one. Make sure you add an entry for the new file in devtools\client\animationinspector\test\browser.ini
And, to actually run the test, you will need to run a new mach command:
./mach mochitest /path/to/the/test.js

Let me know if you have more questions about this. Looking forward to a new patch for this.

::: devtools/server/actors/animation.js
@@ +375,4 @@
>          // iterationcount or iterationStart has changed (for now at least).
>          let newState = this.getState();
>          let oldState = this.currentState;
> +        //Added condition check for direction while updating "hasChanged" value for bug 1330544

No need to add comments that explain the changes you made and what bug you made them in.
We can use the HG history to navigate the code changes and know when something was changed and why.
It's important, however, that you describe the changes you made within the commit message, which you did, so that's great.
So you can simply remove this line here.
Attachment #8841910 - Flags: feedback+
Comment on attachment 8841910 [details] [diff] [review]
Bug_1330544_2.patch

>         hasChanged = newState.delay !== oldState.delay ||
>                      newState.iterationCount !== oldState.iterationCount ||
>                      newState.iterationStart !== oldState.iterationStart ||
>                      newState.duration !== oldState.duration ||
>-                     newState.endDelay !== oldState.endDelay;
>+                     newState.endDelay !== oldState.endDelay ||
>+                     newState.direction !== oldState.direction;

I think we should extend this to check 'easing' and 'fill' too.

(The full set of timing properties is here: https://w3c.github.io/web-animations/#the-animationeffecttimingreadonly-interface)

I suspect we only checked the above set of properties because they were the only ones we actually represented in the animation inspector. The comment before this line also seems to suggest that:

>         // Only consider the state has having changed if any of delay, duration,
>         // iterationcount or iterationStart has changed (for now at least).

However, given that bug 1210796 is going to make us show keyframes too and we'll want to update the view when those change, and given that we should only get animation mutation notifications from Gecko if *something* changed (and we have pretty good tests for the animation mutation observer code), I wonder if we need this check at all?
Added condition check for direction while updating "hasChanged" value in file /devtools/server/actors/animation.js.
Also, added a test for the same in file /devtools/client/animationinspector/test/browser_animation_ui_updates_when_animation_data_changes.js
Attachment #8841910 - Attachment is obsolete: true
Ankita, are you still working on this?
Flags: needinfo?(ankita_tanwar)
Hi Brian,

I submitted the patch in the last comment along with the test case for the fix.
Flags: needinfo?(ankita_tanwar)
Hi Ankita, did you see my comments in comment 12? I think we should remove the check altogether. The test is still needed however, and we should add a similar test for 'fill' and 'easing' too, and perhaps keyframes also.
Hi Ankita!

I'd like to fix this bug.
If you don't mind, I continue to fix.
Flags: needinfo?(ankita_tanwar)
(In reply to Daisuke Akatsuka (:daisuke) from comment #17)
> Hi Ankita!
> 
> I'd like to fix this bug.
> If you don't mind, I continue to fix.

Hi Daisuke

Please go ahead with this bug.
Thank you.
Flags: needinfo?(ankita_tanwar)
Product: Firefox → DevTools
I found another issue which is not actually this bug, but related bug.
If user change the duration related properties (e.g. iterations, duration, delay), and if the total duration is shorter than the currentTime of the animation, the animation will be finished. In this case, we can know that animation was finished by mutation event, so we would be able to handle it, I think.
The issue is next of this.  
Futhermore, if user change the duration to more short, no any events occur. In this case, the graph can't update. 
(Instead, if change more than currentTime, since we can get the event, updating will work well.)

I'll file this as another bug.
Comment on attachment 8989681 [details]
Bug 1330544 - Part 1: Add 'direction' to the changing flag target.

https://reviewboard.mozilla.org/r/254692/#review262120

::: commit-message-2b115:1
(Diff revision 1)
> +Bug 1330544 - Part 1: Animation view does not update when timing properties are changed. r?pbro

This commit message does not really explain what the commit does, it's a bit too general. Could you rephrase please?
Attachment #8989681 - Flags: review?(pbrosset) → review+
Comment on attachment 8989682 [details]
Bug 1330544 - Part 2: Add 'easing' and 'fill' to the changing flag target.

https://reviewboard.mozilla.org/r/254694/#review262122

::: devtools/server/actors/animation.js:393
(Diff revision 1)
>          // iterationCount, iterationStart, or playbackRate has changed (for now
>          // at least).

This comment needs to be updated now that we consider state changes on many more properties.
Attachment #8989682 - Flags: review?(pbrosset) → review+
Comment on attachment 8989681 [details]
Bug 1330544 - Part 1: Add 'direction' to the changing flag target.

https://reviewboard.mozilla.org/r/254692/#review262120

> This commit message does not really explain what the commit does, it's a bit too general. Could you rephrase please?

Thanks Patrick.
Because this is Ankita's commit, I just added as it is.
However, yes, I'll change the message.
Comment on attachment 8989682 [details]
Bug 1330544 - Part 2: Add 'easing' and 'fill' to the changing flag target.

https://reviewboard.mozilla.org/r/254694/#review262122

> This comment needs to be updated now that we consider state changes on many more properties.

Indeed! Thanks!
Comment on attachment 8989683 [details]
Bug 1330544 - Part 3: Add test of altering properties.

https://reviewboard.mozilla.org/r/254696/#review262128

::: devtools/client/inspector/animation/test/doc_frame_script.js:42
(Diff revision 1)
> +addMessageListener("Test:SetStyles", function(msg) {
> +  const { selector, properties, } = msg.data;
> +  const element = superQuerySelector(selector);
> +
> +  if (!element) {
> +    return;
> +  }
> +
> +  for (const propertyName in properties) {
> +    const propertyValue = properties[propertyName];
> +    element.style[propertyName] = propertyValue;
> +  }
> +
> +  sendAsyncMessage("Test:SetStyles");
> +});
> +
>  function isRemovableElement(animation, selectors) {
>    for (const selector of selectors) {
>      if (animation.effect.target.matches(selector)) {
>        return false;
>      }
>    }
>  
>    return true;
>  }
> +
> +/**
> + * Like document.querySelector but can go into iframes too.
> + * ".container iframe || .sub-container div" will first try to find the node
> + * matched by ".container iframe" in the root document, then try to get the
> + * content document inside it, and then try to match ".sub-container div" inside
> + * this document.
> + * Any selector coming before the || separator *MUST* match a frame node.
> + * @param {String} superSelector.
> + * @return {DOMNode} The node, or null if not found.
> + */
> +function superQuerySelector(superSelector, root = content.document) {
> +  const frameIndex = superSelector.indexOf("||");
> +  if (frameIndex === -1) {
> +    return root.querySelector(superSelector);
> +  }
> +  const rootSelector = superSelector.substring(0, frameIndex).trim();
> +  const childSelector = superSelector.substring(frameIndex + 2).trim();
> +  root = root.querySelector(rootSelector);
> +  if (!root || !root.contentWindow) {
> +    return null;
> +  }
> +
> +  return superQuerySelector(childSelector, root.contentWindow.document);
> +}

Both of these functions already exist in the code base, in devtools/client/shared/test/frame-script-utils.js, we shouldn't duplicate them here.
This frame-script-utils frame script is already loaded by a lot of test suites I think. You should just make sure it's loaded for you too and then just use it.
It looks like you need a slightly different version of setStyles though, which sets multiple styles at once. That's fine, just add the function to frame-script-utils.js instead, so others can use it too.
Attachment #8989683 - Flags: review?(pbrosset)
Comment on attachment 8989683 [details]
Bug 1330544 - Part 3: Add test of altering properties.

https://reviewboard.mozilla.org/r/254696/#review262128

> Both of these functions already exist in the code base, in devtools/client/shared/test/frame-script-utils.js, we shouldn't duplicate them here.
> This frame-script-utils frame script is already loaded by a lot of test suites I think. You should just make sure it's loaded for you too and then just use it.
> It looks like you need a slightly different version of setStyles though, which sets multiple styles at once. That's fine, just add the function to frame-script-utils.js instead, so others can use it too.

Ah, indeed. Will fix them.
Comment on attachment 8989683 [details]
Bug 1330544 - Part 3: Add test of altering properties.

https://reviewboard.mozilla.org/r/254696/#review262146
Attachment #8989683 - Flags: review?(pbrosset) → review+
Thanks, I will make the patches to land after checking the try.
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfc8ad9b2244
Part 1: Add 'direction' to the changing flag target. r=pbro
https://hg.mozilla.org/integration/autoland/rev/a2369d01b9b6
Part 2: Add 'easing' and 'fill' to the changing flag target. r=pbro
https://hg.mozilla.org/integration/autoland/rev/72882160c090
Part 3: Add test of altering properties. r=pbro
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: