1155653 - Animation target dom nodes should be previewed in the animation panel

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

The goal in the animation panel is to not only show the animations of the currently selected node in the inspector, but the animations of the nodes in its subtree too (see bug 1155651), and so it means that it will be hard for a user to know which node a particular animation applies to.

This bug is about adding a preview of the target node in each Animation widget.
Hovering the preview should highlight the node in the page.
Clicking should select that node in the inspector.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: bz://1155653/pbrosset

/r/7605 - Bug 1155653 - Preview animation target nodes in animationinspector panel

Pull down this commit:

hg pull -r df17f408f3340e0aebe47e76a4e49eeb90d3f782 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Patrick Brosset <:pbro>]

2 notes for Brian:
- I hope the comment in the new walker method makes enough sense. Basically, the problem is that if you want an actor to send you a NodeActor for a DOMNode reference it holds, you have to go through the walker, otherwise, the resulting NodeFront won't be properly referenced, and you won't be able to highlight or select that node in the inspector later on.
- Also, know that I've started to rewrite most of the UI for bug 1153271. I still think it's important to land this, because it may be a while before v3 fully lands, and I plan on developing it behind a pref. But we shouldn't care too much about the UI I guess, because it will go away.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8597228 [details]
MozReview Request: bz://1155653/pbrosset

/r/7605 - Bug 1155653 - Preview animation target nodes in animationinspector panel

Pull down this commit:

hg pull -r 074866feaf662447d4a1a4f18b58dc17d9e1897a https://reviewboard-hg.mozilla.org/gecko/

Comment 4

[Patrick Brosset <:pbro>]

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

Comment 5

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/7605/#review6665

The getNodeFromActor stuff seems OK, since we've dealt with this before in the style editor.  I'd like to see a separate server side test for this feature though.

::: browser/devtools/animationinspector/animation-panel.js:944
(Diff revision 2)
> +    gToolbox.highlighterUtils.highlightNodeFront(this.nodeFront);

This isn't a problem with this code (it's an issue with highlightNodeFront), but I just noticed that the highlighter isn't able to keep up with one of the animated nodes on: https://bgrins.github.io/devtools-demos/inspector/animation-timing.html

::: browser/devtools/animationinspector/animation-panel.js:836
(Diff revision 2)
> +    // Init the markup for displaying the target node.

I wish we had a component that created a node like this that could be reused across tools.  Something to keep in mind when building this is if we can export and reuse this UI in other places (like webconsole, etc).

::: browser/devtools/animationinspector/animation-panel.js:958
(Diff revision 2)
> +  render: function(playerFront) {

This doesn't get re-rendered on mutations.  May not be a huge deal for v1, but highlight a node that has child animations and then remove one of them from the console.  Notice that it's still there.  Likewise, if you change an attribute like remove an ID on one of the animations the UI doesn't update

::: browser/themes/shared/devtools/animationinspector.css:123
(Diff revision 2)
> +  color: var(--theme-highlight-orange);

These color's aren't going to match the markup view for the dark theme.  If you use classes like these instead it should work automatically for both themes: theme-fg-color3, theme-fg-color6, theme-fg-color2.  See https://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.xhtml#62

::: browser/devtools/animationinspector/test/browser_animation_target_highlight_select.js:58
(Diff revision 2)
> +  ok(true, "The inspector got updated");

I think you can remove this assertion since you have one directly below it.  I'd only leave it there if it was the only one between the yields

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8597228 [details]
MozReview Request: bz://1155653/pbrosset

Reviewboard didn't clear the review flag

Comment 7

[Patrick Brosset <:pbro>]

Thanks Brian, some comments inline below:

(In reply to Brian Grinstead [:bgrins] from comment #5)
> https://reviewboard.mozilla.org/r/7605/#review6665
> 
> The getNodeFromActor stuff seems OK, since we've dealt with this before in
> the style editor.  I'd like to see a separate server side test for this
> feature though.
Done, added a new test for this.
> ::: browser/devtools/animationinspector/animation-panel.js:944
> (Diff revision 2)
> > +    gToolbox.highlighterUtils.highlightNodeFront(this.nodeFront);
> 
> This isn't a problem with this code (it's an issue with highlightNodeFront),
> but I just noticed that the highlighter isn't able to keep up with one of
> the animated nodes on:
> https://bgrins.github.io/devtools-demos/inspector/animation-timing.html
What do you mean exactly? That it's moving a little slower than the node itself? I see this too, but on my computer it's not noticeable enough to be a problem I think. Are you seeing important lag?
> ::: browser/devtools/animationinspector/animation-panel.js:836
> (Diff revision 2)
> > +    // Init the markup for displaying the target node.
> 
> I wish we had a component that created a node like this that could be reused
> across tools.  Something to keep in mind when building this is if we can
> export and reuse this UI in other places (like webconsole, etc).
Good point. For now, I have moved all of the ui components into a separate module and exported the symbols. This way, this can be reused if needed.
> ::: browser/devtools/animationinspector/animation-panel.js:958
> (Diff revision 2)
> > +  render: function(playerFront) {
> 
> This doesn't get re-rendered on mutations.  May not be a huge deal for v1,
> but highlight a node that has child animations and then remove one of them
> from the console.  Notice that it's still there.
Hmm interesting, I didn't see this coming. Filed bug 1160988 for this as this specific problem isn't related to this patch.
> Likewise, if you change an
> attribute like remove an ID on one of the animations the UI doesn't update
This problem is related to the patch though. I'll get this fixed.
> 
> ::: browser/themes/shared/devtools/animationinspector.css:123
> (Diff revision 2)
> > +  color: var(--theme-highlight-orange);
> 
> These color's aren't going to match the markup view for the dark theme.  If
> you use classes like these instead it should work automatically for both
> themes: theme-fg-color3, theme-fg-color6, theme-fg-color2.  See
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/
> markup-view.xhtml#62
Right, thanks for catching this. Fixed.
> :::
> browser/devtools/animationinspector/test/
> browser_animation_target_highlight_select.js:58
> (Diff revision 2)
> > +  ok(true, "The inspector got updated");
> 
> I think you can remove this assertion since you have one directly below it. 
> I'd only leave it there if it was the only one between the yields
Done.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8597228 [details]
MozReview Request: bz://1155653/pbrosset

/r/7605 - Bug 1155653 - Preview animation target nodes in animationinspector panel; r=bgrins

Pull down this commit:

hg pull -r 6e65174287403673b9bdc81b462dd2dfd2677dca https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8597228 [details]
MozReview Request: bz://1155653/pbrosset

/r/7605 - Bug 1155653 - Preview animation target nodes in animationinspector panel; r=bgrins

Pull down this commit:

hg pull -r dfbf459d9de14233f125d028cf81f6b934941ea2 https://reviewboard-hg.mozilla.org/gecko/

Comment 10

[Patrick Brosset <:pbro>]

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

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #7)
> > This isn't a problem with this code (it's an issue with highlightNodeFront),
> > but I just noticed that the highlighter isn't able to keep up with one of
> > the animated nodes on:
> > https://bgrins.github.io/devtools-demos/inspector/animation-timing.html
> What do you mean exactly? That it's moving a little slower than the node
> itself? I see this too, but on my computer it's not noticeable enough to be
> a problem I think. Are you seeing important lag?

It's not super noticeable, though more so on a lower powered development VM.  Nothing to worry about for this bug, but it'd be worth profiling to see what the cause of the lag is.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8597228 [details]
MozReview Request: bz://1155653/pbrosset

https://reviewboard.mozilla.org/r/7603/#review6879

Changes and reorganization look fine, except that on the test page I still don't see the target list update if you have the body selected and then run this in the split console: $("#box1").remove()

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> Changes and reorganization look fine, except that on the test page I still
> don't see the target list update if you have the body selected and then run
> this in the split console: $("#box1").remove()

Also, please add a test case for this

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> Comment on attachment 8597228 [details]
> MozReview Request: bz://1155653/pbrosset
> 
> https://reviewboard.mozilla.org/r/7603/#review6879
> 
> Changes and reorganization look fine, except that on the test page I still
> don't see the target list update if you have the body selected and then run
> this in the split console: $("#box1").remove()
See my reply in comment 7 about this: this problem isn't related to this bug, but instead a case I forgot to handle in bug 1155651. That's why I filed bug 1160988 to fix it rather than pollute this patch with something somewhat unrelated.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8597228 [details]
MozReview Request: bz://1155653/pbrosset

https://reviewboard.mozilla.org/r/7603/#review6883

Ship It!

Comment 16

[Patrick Brosset <:pbro>]

Thank you Brian. Filed bug 1161217 for the highlighter lag.

Comment 17

[Patrick Brosset <:pbro>]

https://hg.mozilla.org/integration/fx-team/rev/f88d5fb92a29

Comment 18

[Guilherme Lima]

This patch landed on MC but didn't got marked as such.
https://hg.mozilla.org/mozilla-central/rev/f88d5fb92a29

Comment 20

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1155653 - Preview animation target nodes in animationinspector panel; r=bgrins