Make the Animation Inspector able to display animations running on any selected node, even when those nodes are in oop iframes
Categories
(DevTools :: Inspector: Animations, enhancement, P1)
Tracking
(Fission Milestone:M6, firefox75 fixed)
Tracking | Status | |
---|---|---|
firefox75 | --- | fixed |
People
(Reporter: pbro, Assigned: daisuke)
References
(Blocks 1 open bug)
Details
(Whiteboard: dt-fission-m2-mvp)
Attachments
(3 files, 1 obsolete file)
With fission, and once bug 1560200 is fixed, we'll be able to select nodes in the inspector that may live in out of process iframes.
When this happens, the Animation Inspector must continue being able to display all of the active animations that run on the selected node and node's subtree.
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Comment 1•5 years ago
|
||
One call site in particular will need to be updated:
devtools/client/inspector/animation/animation.js line 312
return this.inspector.walker.getNodeFromActor(actorID, ["node"]);
this will need to use the walker contextual to the animation actor instead.
Comment 2•5 years ago
|
||
Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.
This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:
0ee3c76a-bc79-4eb2-8d12-05dc0b68e732
Reporter | ||
Comment 3•5 years ago
•
|
||
In the main panel file: animation.js, this function
_getAnimationsFront() {
if (this.animationsFrontPromise) {
return this.animationsFrontPromise;
}
this.animationsFrontPromise = (async () => {
const target = this.inspector.currentTarget;
const front = await target.getFront("animations");
front.setWalkerActor(this.inspector.walker);
return front;
})();
return this.animationsFrontPromise;
}
will need to be changed. This is called just once when the panel starts.
This is wrong for 2 reasons:
- with fission, there will be process switches on navigation, so we need to use the
TargetList
API to react to new top-level targets and get the new animations front when that happens - but also, we want to get the animations that are running inside the frame of the currently selected element, so we might as well get the animations front only when a new node is selected, using
selection.nodeFront.targetFront.getFront("animations")
instead.
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Comment 5•5 years ago
|
||
The animations panel retrieves the instance of the AnimationsFront when starting
that corresponds to the top-level target the inspector is connected to.
It uses that to list animations and set their states and current times.
This is fine, this is the current logic of this panel, which we want to retain
(i.e. we do want to change it to also list animations inside nested, perhaps
remote, iframes).
However with fission, navigating to a different site in the same tab will cause
a process switch and we therefore need to make sure an updated instance of the
AnimationsFront is retrieved then.
We can do this using the TargetList API.
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Comment 6•5 years ago
|
||
(In reply to Patrick Brosset <:pbro> from comment #3)
- with fission, there will be process switches on navigation, so we need to use the
TargetList
API to react to new top-level targets and get the new animations front when that happens
This is what the patch above does.
- but also, we want to get the animations that are running inside the frame of the currently selected element, so we might as well get the animations front only when a new node is selected, using
selection.nodeFront.targetFront.getFront("animations")
instead.
I ended up deciding against this as this would introduce a new feature to the animation panel that doesn't exist today. Whenever selecting a node inside an iframe, this would have the effect of showing the animations for this iframe.
Assignee | ||
Comment 7•5 years ago
|
||
I discussed with Patrick on Slack, I will take this bug.
Assignee | ||
Comment 8•5 years ago
|
||
Assignee | ||
Comment 11•5 years ago
|
||
I wrote a test for target-switching for this, but it is failing when navigating from the main process page to the content process page.
As long as I investigated, it seems that this failure occurs for a few reasons.
As at least it seems that we are hitting the bug 1604126, I'll fix bug 1604126 before this one.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 12•5 years ago
|
||
Pushed by dakatsuka.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/84bea78312b2 Install target-switching mechanism. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/03a2a8f68e02 Rename and refactor some functions. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/2d442aa5fd93 Add a test for target-switching. r=jdescottes
Comment 13•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/84bea78312b2
https://hg.mozilla.org/mozilla-central/rev/03a2a8f68e02
https://hg.mozilla.org/mozilla-central/rev/2d442aa5fd93
Updated•5 years ago
|
Description
•