1468711 - The property graphs are displayed inverted (left right) if property values are decreasing

Status: RESOLVED FIXED

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

Description

[Daniel Bodea [:danibodea]]

Attachment: Property graph displayed left-right inverted PROBABLY because initial and final values are decreasing

STR:
1. Open the following page:
https://bug1450526.bmoattachments.org/attachment.cgi?id=8964151
2. Open Animation Inspector.
3. Hover over the orange dots/discs at the beginning and ending of the opacity property and observe the values displayed on the tool-tip.
4. Observe the opacity property's graph.
Actual result: The graph is displayed as increasing.
Expected: The graph is displayed as decreasing from 75% to 0.

Comment 1

[Daisuke Akatsuka (:daisuke)]

This occur in case of pseudo element only since we have not send the proper element to nsDOMWindowUtils::ComputeAnimationDistance. [1][2]

[1] https://searchfox.org/mozilla-central/source/devtools/server/actors/animation.js#531
[2] https://searchfox.org/mozilla-central/source/devtools/server/actors/animation.js#556

Comment 2

[Daisuke Akatsuka (:daisuke)]

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

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1468711 - Part 1: Send a proper element for pseudo element to nsDOMWindowUtils::ComputeAnimationDistance.

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

Comment 4

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1468711 - Part 2: Modify a test for pseudo element.

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

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8986109 [details]
Bug 1468711 - Part 1: Send a proper element for pseudo element to nsDOMWindowUtils::ComputeAnimationDistance.

https://reviewboard.mozilla.org/r/251568/#review258466

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8986110 [details]
Bug 1468711 - Part 2: Modify a test for pseudo element.

https://reviewboard.mozilla.org/r/251570/#review258468

::: devtools/client/inspector/animation/animation.js:627
(Diff revision 1)
>        : [];
>      this.updateState(animations);
>      this.setAnimationStateChangedListenerEnabled(true);
>  
>      done();
> +    this.emit("animation-inspector-updated");

I don't think this is needed, here's why:

First we do:
`const done = this.inspector.updating("newanimationinspector");`

And then later we do:
`done();`

What this does is it tells the inspector panel that the animationinspector is done updating. When the inspector panel receives this, it checks if it has any other panels that are also updating (maybe the rule-view, maybe computed-view), and once everything is done, at the end, it emits the `inspector-updated` 
event.

So I don't think you need this new event, and instead, in head.js, you can just wait for `inspector-updated`.

Comment 7

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8986110 [details]
Bug 1468711 - Part 2: Modify a test for pseudo element.

https://reviewboard.mozilla.org/r/251570/#review258468

> I don't think this is needed, here's why:
> 
> First we do:
> `const done = this.inspector.updating("newanimationinspector");`
> 
> And then later we do:
> `done();`
> 
> What this does is it tells the inspector panel that the animationinspector is done updating. When the inspector panel receives this, it checks if it has any other panels that are also updating (maybe the rule-view, maybe computed-view), and once everything is done, at the end, it emits the `inspector-updated` 
> event.
> 
> So I don't think you need this new event, and instead, in head.js, you can just wait for `inspector-updated`.

Yeah, I had thought so.
However, the waitForRendering (actually waitForAnimatoinDetail[1]) can not wait correctly if we wait for `inspector-updated`.
Because occarsionaly the `animation-keyframes-rendered` was already called while waiting the `inspector-updated`. 
That means the test will be timeouted. So as to resolve this, since I wanted to get immediately after updating the animation inspector (especially the state), I added new event.
What do you think?

https://searchfox.org/mozilla-central/source/devtools/client/inspector/animation/test/head.js#533

Comment 8

[Daisuke Akatsuka (:daisuke)]

And, can we replace all to `animation-keyframes-rendered` instead of `inspector-updated`?

Comment 9

[Daisuke Akatsuka (:daisuke)]

Patrick, sorry to bother you.
How should we do?

Comment 10

[Patrick Brosset <:pbro>]

Sorry for the delay, I hadn't seen your response until you needinfo'd me.

How about this: would it work if we changed clickOnTargetNode to this:

const clickOnTargetNode = async function(animationInspector, panel, index) {
  info(`Click on a target node in animation target component[${ index }]`);
  const targetEl = panel.querySelectorAll(".animation-target .objectBox")[index];
  targetEl.scrollIntoView(false);

  // Start waiting for the relevant promises that will get resolved once the node is selected.
  const onHighlight = animationInspector.inspector.toolbox.once("node-highlight");
  const onInspectorUpdated = animationInspector.inspector.once("inspector-updated");
  const onRendered = waitForRendering(animationInspector);

  EventUtils.synthesizeMouseAtCenter(targetEl, {}, targetEl.ownerGlobal);

  await Promise.all([onHighlight, onInspectorUpdated, onRendered]);
}

This way we race them, and we can't get into the case where one event already fired while we were waiting for another one, so we missed it.

Comment 11

[Daisuke Akatsuka (:daisuke)]

Thanks, Patrick!
I think this may be failed since the waitForRendering (especially waitForAnimatoinDetail) evaluates whether we need to wait or not by the animation state of animation inspector.
However, because I understood your approach, I will try similar way.

Comment 12

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8986109 [details]
Bug 1468711 - Part 1: Send a proper element for pseudo element to nsDOMWindowUtils::ComputeAnimationDistance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251568/diff/1-2/

Comment 13

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8986110 [details]
Bug 1468711 - Part 2: Modify a test for pseudo element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251570/diff/1-2/

Comment 14

[Daisuke Akatsuka (:daisuke)]

I have changed to wait for rendering without new event.
And try has no failed.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9acc608d58c87acc3dd92608660aec0477d352f9

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8986110 [details]
Bug 1468711 - Part 2: Modify a test for pseudo element.

https://reviewboard.mozilla.org/r/251570/#review259928

Comment 16

[Daisuke Akatsuka (:daisuke)]

Thanks!
I'll make the patches to land.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8290ab3d43d4
Part 1: Send a proper element for pseudo element to nsDOMWindowUtils::ComputeAnimationDistance. r=pbro
https://hg.mozilla.org/integration/autoland/rev/283192564aca
Part 2: Modify a test for pseudo element. r=pbro

Comment 18

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/8290ab3d43d4
https://hg.mozilla.org/mozilla-central/rev/283192564aca

Comment 19

[Daniel Bodea [:danibodea]]

Tested on Windows 10, Ubuntu 16.04 and Mac OS X 10.12.6 and could not be reproduced.
This issue is verified on the latest Nightly v63.0a1 (2018-06-27).

Comment 20

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8986109 [details]
Bug 1468711 - Part 1: Send a proper element for pseudo element to nsDOMWindowUtils::ComputeAnimationDistance.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1399830
[User impact if declined]: User can misunderstand the animation behavior on pseudo element.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[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 was a few lines and we got Verified status from QA.
[String changes made/needed]: None

Comment 21

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

New feature we're aiming to ship in 62, so marking 62 affected.

Comment 22

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

Comment on attachment 8986109 [details]
Bug 1468711 - Part 1: Send a proper element for pseudo element to nsDOMWindowUtils::ComputeAnimationDistance.

Verified in nightly, fix for a new feature in early beta.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e3b8a0af8238
https://hg.mozilla.org/releases/mozilla-beta/rev/d57f1bdcdf52