Animation target dom nodes should be previewed in the animation panel

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Developer Tools: Animation Inspector
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

unspecified
Firefox 40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1153271
Depends on: 1155651
(Assignee)

Updated

3 years ago
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created 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 df17f408f3340e0aebe47e76a4e49eeb90d3f782 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8597228 - Flags: review?(bgrinstead)
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
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/
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 on attachment 8597228 [details]
MozReview Request: bz://1155653/pbrosset

Reviewboard didn't clear the review flag
Attachment #8597228 - Flags: review?(bgrinstead)
(Assignee)

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
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/
Attachment #8597228 - Flags: review?(bgrinstead)
(Assignee)

Comment 9

3 years ago
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/
(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 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()
Attachment #8597228 - Flags: review?(bgrinstead)
(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
(Assignee)

Comment 14

3 years ago
(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.
Blocks: 1160988
Attachment #8597228 - Flags: review+
Comment on attachment 8597228 [details]
MozReview Request: bz://1155653/pbrosset

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

Ship It!
(Assignee)

Comment 16

3 years ago
Thank you Brian. Filed bug 1161217 for the highlighter lag.

Comment 18

3 years ago
This patch landed on MC but didn't got marked as such.
https://hg.mozilla.org/mozilla-central/rev/f88d5fb92a29
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
(Assignee)

Comment 19

3 years ago
Comment on attachment 8597228 [details]
MozReview Request: bz://1155653/pbrosset
Attachment #8597228 - Attachment is obsolete: true
Attachment #8620075 - Flags: review+
(Assignee)

Comment 20

3 years ago
Created attachment 8620075 [details]
MozReview Request: Bug 1155653 - Preview animation target nodes in animationinspector panel; r=bgrins
(Assignee)

Updated

3 years ago
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
You need to log in before you can comment on or make changes to this bug.