Closed Bug 1468711 Opened 2 years ago Closed Last year

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

Categories

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

defect

Tracking

(firefox62 fixed, firefox63 verified)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox62 --- fixed
firefox63 --- verified

People

(Reporter: danibodea, Assigned: daisuke)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

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.
Priority: -- → P3
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
Assignee: nobody → dakatsuka
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
Attachment #8986109 - Flags: review?(pbrosset) → review+
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`.
Attachment #8986110 - Flags: review?(pbrosset)
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
And, can we replace all to `animation-keyframes-rendered` instead of `inspector-updated`?
Patrick, sorry to bother you.
How should we do?
Flags: needinfo?(pbrosset)
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.
Flags: needinfo?(pbrosset)
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.
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 on attachment 8986110 [details]
Bug 1468711 - Part 2: Modify a test for pseudo element.

https://reviewboard.mozilla.org/r/251570/#review259928
Attachment #8986110 - Flags: review?(pbrosset) → review+
Thanks!
I'll make the patches to land.
Pushed by dakatsuka@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/8290ab3d43d4
https://hg.mozilla.org/mozilla-central/rev/283192564aca
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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 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
Attachment #8986109 - Flags: approval-mozilla-beta?
Attachment #8986110 - Flags: approval-mozilla-beta?
New feature we're aiming to ship in 62, so marking 62 affected.
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.
Attachment #8986109 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8986110 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.