Closed Bug 1407900 Opened 7 years ago Closed 7 years ago

Nothing appears in animation inspector's summary graph

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Type:
defect

Tracking

(firefox57 fix-optional, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: birtles, Assigned: daisuke)

Details

Attachments

(5 files)

Screenshot
28.58 KB, image/png
Details
Bug 1407900 - Part 1: Avoid updating animations UI during hidden.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1407900 - Part 2: Add test for hidden panel.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1407900 - Part 3: Fix the bug that can't open detail panel if closed detail panel was showing clicked animation.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1407900 - Part 4: Add test for can't open detail.
59 bytes, text/x-review-board-request
pbro
: review+
Details 
STR:
1. Open google.com
2. Right click the logo
3. Choose "Inspect element"
4. In the console enter:
   $0.animate({ transform: 'translate(100px)' }, { duration: 1000, endDelay: 500 });
5. Quickly switch to the inspector tab and look at the animations panel

Expected results:
The graph of the animation is shown.

Actual result:
No graph is visible

A further, possible unrelated, bug is that if I close the keyframes panel I can't open it again.
Thank you very much, Brian.
It might be a regression of bug 1405983.
I'll take a look immediately.
I was able to reproduce this issue. It only happens to me if I strictly follow the STR provided in comment 0. If instead I open the "split console" (so that both the animation panel and the panel are visible at the same time), and then execute the script, then it works fine.
So it's probably something related to the fact that the animation panel is hidden, and so perhaps we can't measure some element's size or something.
status-firefox57: --- → fix-optional
Priority: -- → P2
Thanks Patrick,
Yes, it reproduces if animations are update and render during the inspector was hidden.
I confirmed to reproduce in beta 57.0b6 as well. (Although the shape differ, cause is same.)

To resolve this, we may need to avoid updating the animations UI during hidden even if onAnimationMutations of animation-controller was called. Then, when re-open the ‘inspector’, update the UI again if animations has updating during hidden.
I’ll try to do above approach.
You can't just attach the node used to create the graph to somewhere in the document or some other document that is not display:none? You could still make it visibility:hidden or so that it doesn't render.
Thank you for the valuable opinions!

Unlike other inspectors, animation inspector are implemented inside isolated iframe, so it may be difficult to insert the element into parents or other documents.
Also, the current animation inspector avoids updating the UI when a node is newly selected and so on, for example. If we keep this thinking, I think that this update has become correct processing in the sense that it keeps consistency. Also, other inspectors such dom, grid and font as well is doing same processing.
Comment on attachment 8917875 [details]
Bug 1407900 - Part 1: Avoid updating animations UI during hidden.

https://reviewboard.mozilla.org/r/188808/#review194388

::: devtools/client/animationinspector/animation-controller.js:235
(Diff revision 1)
>    onNewNodeFront: Task.async(function* () {
> -    // Ignore if the panel isn't visible or the node selection hasn't changed.
> -    if (!this.isPanelVisible() ||
> -        this.nodeFront === gInspector.selection.nodeFront) {
> +    // Ignore if the panel isn't visible.
> +    // Or the node selection hasn't changed and no animation mutations event occurs during
> +    // hidden.
> +    if (!this.isPanelVisible() || (this.nodeFront === gInspector.selection.nodeFront &&
> +                                   !this.isMutationsEventOccuredDuringHidden)) {

Suggestion for a new property name:
`mutationsDetectedWhileHidden`

::: devtools/client/animationinspector/animation-controller.js:371
(Diff revision 1)
> +      // Avoid updating the UI during hidden since we use computed value of an element
> +      // which append to this panel to create graph shape. If hidden, we can't get the
> +      // computed value correctly.
> +      // So we set the flag so that update when re-open this inspector.

Maybe this comments says a bit too much. The fact that we use computed values is sort of an internal implementation detail that doesn't really make sense at the animation-controller.js level.
Plus, not updating while hidden is actually a perf improvement. 
We are working a lot on performance at the moment, and we are writing code to avoid updating anything that is currently hidden. So your work here is very nice.
And I think this comment should just be concerned about this.
Suggestion:

// Avoid updating the UI while the panel is hidden.
// This avoids unnecessary work.
Attachment #8917875 - Flags: review?(pbrosset) → review+
Comment on attachment 8917876 [details]
Bug 1407900 - Part 2: Add test for hidden panel.

https://reviewboard.mozilla.org/r/188810/#review194392

::: devtools/client/animationinspector/test/browser.ini:47
(Diff revision 1)
>  [browser_animation_participate_in_inspector_update.js]
>  [browser_animation_playerFronts_are_refreshed.js]
>  [browser_animation_playerWidgets_appear_on_panel_init.js]
>  [browser_animation_playerWidgets_target_nodes.js]
>  [browser_animation_pseudo_elements.js]
> +[browser_animation_refresh_during_hidden.js]

s/during/while

Also, we have a related test already:
browser_animation_refresh_when_active.js

I'm ok to add a second one instead of making that one longer, but I would suggest to name it in a smilar way:

browser_animation_refresh_when_active_after_mutations.js

::: devtools/client/animationinspector/test/browser_animation_refresh_during_hidden.js:9
(Diff revision 1)
> +
> +"use strict";
> +
> +requestLongerTimeout(2);
> +
> +// Test that refresh animation UI during the panel is hidden.

s/during/while

::: devtools/client/animationinspector/test/browser_animation_refresh_during_hidden.js:22
(Diff revision 1)
> +  info("Make animation by console");
> +  const webconsole = yield toolbox.selectTool("webconsole");
> +  webconsole.hud.jsterm.clearOutput(true);
> +  yield webconsole.hud.jsterm.execute("document.querySelector('#target').animate(" +
> +                                      "{ transform: 'translate(100px)' }," +
> +                                      "{ duration: 100000, easing: 'steps(2)' });");
> +  yield webconsole.hud.jsterm.execute("console.log('wait a bit');");

This looks a bit too complicated to me. I think we can do something a lot simpler.
The problem also appears when you switch away to another sidebar panel (like the rule-view), so I think we should just stay on the inspector, but simply switch to the rule-view instead (this would in fact make the test faster as well).
And then, while the animation panel is hidden, instead of executing via the console API, we can just execute the same animation code in-content instead.

I think we have several ways to do this from mochitests: `executeInContent`, or `evalInDebuggee` or `ContentTask.spawn` or using the `TestActor`.
Attachment #8917876 - Flags: review?(pbrosset)
Comment on attachment 8917877 [details]
Bug 1407900 - Part 3: Fix the bug that can't open detail panel if closed detail panel was showing clicked animation.

https://reviewboard.mozilla.org/r/188812/#review194398
Attachment #8917877 - Flags: review?(pbrosset) → review+
Comment on attachment 8917878 [details]
Bug 1407900 - Part 4: Add test for can't open detail.

https://reviewboard.mozilla.org/r/188814/#review194400

::: devtools/client/animationinspector/test/browser_animation_detail_displayed.js:32
(Diff revision 1)
>  
>    // 1. Existance of animation-detail element.
>    ok(animationDetailEl, "The animation-detail element should exist");
>  
>    // 2. Hidden at first if multiple animations were displayed.
> -  const win = animationDetailEl.ownerDocument.defaultView;
> +  const win = timelineComponent.rootWrapperEl.ownerDocument.defaultView;

`ownerGlobal` is a shortcut for `ownerDocument.defaultView` and we have an ESLint rule view that prevents from using the long form.
Attachment #8917878 - Flags: review?(pbrosset) → review+
Comment on attachment 8917876 [details]
Bug 1407900 - Part 2: Add test for hidden panel.

https://reviewboard.mozilla.org/r/188810/#review194806
Attachment #8917876 - Flags: review?(pbrosset) → review+
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e0ade0abeb9
Part 1: Avoid updating animations UI during hidden. r=pbro
https://hg.mozilla.org/integration/autoland/rev/eeff76e2b49b
Part 2: Add test for hidden panel. r=pbro
https://hg.mozilla.org/integration/autoland/rev/e3e885ced5fa
Part 3: Fix the bug that can't open detail panel if closed detail panel was showing clicked animation. r=pbro
https://hg.mozilla.org/integration/autoland/rev/bc4cbd826553
Part 4: Add test for can't open detail. r=pbro
Product: Firefox → DevTools
Assignee: nobody → dakatsuka
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: