Closed Bug 1151824 Opened 9 years ago Closed 9 years ago

Animation inspector play/pause-all button doesn't pause animations on <body>

Categories

(DevTools :: Inspector: Animations, defect)

Product:
DevTools
Component:
Inspector: Animations
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WORKSFORME

People

(Reporter: pbro, Unassigned, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file)

animation.patch
1.07 KB, patch
pbro
: feedback+
Details | Diff | Splinter Review 
STR:

- open https://bug1151018.bugzilla.mozilla.org/attachment.cgi?id=8588120
- open the inspector
- in the Animations sidebar panel, hit the "All animations" pause button at the top

Expected :) All animations on the page are paused

Actual :( The animation on the <body> node isn't paused
Probably a simple fix here: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/animation.js#591
The loop that gets all animations skips the <body> node (it starts looping with its child nodes).

We'll need a new test case in /toolkit/devtools/server/tests/browser/ too.
Mentor: pbrosset
Whiteboard: [good first bug][lang=js]
Blocks: 1120900
Hey Patrick, I'd like to get started with this bug. I've already done a few bugs. So, you don't have to worry about setting me up :P

Anyways, I've noticed that bug. It appears when we open the Inspector for the first time in the page. If we click on some other node and click `<body>` again, the bug disappears...
Flags: needinfo?(pbrosset)
(In reply to Ravi Shankar [:waffles] from comment #2)
> Anyways, I've noticed that bug. It appears when we open the Inspector for
> the first time in the page. If we click on some other node and click
> `<body>` again, the bug disappears...
You are very right, that's because the toggleAll method (called when the pause-all button is clicked) first loops through the animations displayed in the panel and pauses them first before calling the AnimationsActor to pause all existing animations:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/animationinspector/animation-panel.js#114
It's worth noting that with bug 1151018 fixed, this will work even the first time the inspector is open.
But still, if you select another node (the <div> in the sample page for instance) and then click the pause button, then the animation on the <body> node won't be paused.
So we need to fix the loop in the AnimationsActor's getAllAnimationPlayers method: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/animation.js#591

Thanks for your interest in this bug, I'll assign it to you. Let me know if the information above is enough to get started or if you have other questions (I'm also available on IRC #devtools, nick pbrosset).
Flags: needinfo?(pbrosset)
Attached patch animation.patch — — Splinter Review 
Hey Patrick, I noticed that if we prefer `document` to `body`, we're able to eliminate the bug :)
Attachment #8591791 - Flags: review?(pbrosset)
Comment on attachment 8591791 [details] [diff] [review]
animation.patch

Review of attachment 8591791 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Just a comment about simplifying even further.
Also, can you add a new test for this as suggested in comment 1?

::: toolkit/devtools/server/actors/animation.js
@@ +594,5 @@
>      // These loops shouldn't be as bad as they look.
>      // Typically, there will be very few windows, and getElementsByTagName is
>      // really fast even on large DOM trees.
>      for (let window of this.tabActor.windows) {
> +      let root = window.document || window.document.body;

Yes indeed that's the right idea, we need to use window.document to start iterating through nodes, but we don't need the second branch here. All windows have a document property, so the OR part will never be used.
So I believe, you can keep it simple:

let root = window.document;
Attachment #8591791 - Flags: review?(pbrosset) → feedback+
Hey Patrick, Sorry that I wasn't around for almost 3 weeks now, as I had my exams back then. Now that my vacation has started, I think I can get back on track again. But, I notice that there has been some major changes to `animation.js`, that I now can't even find `window.document.body`. Can you help me out with what I gotta do next?
Flags: needinfo?(pbrosset)
And, I also noticed that the steps can't be reproduced in my Nightly, that I had built using the current revision. So, I guess the bug's been fixed already... :/
Oh you're right, a couple of major new features were added recently to the animation-inspector (bug 1155651 in particular I think), and as a result, a bunch of code had to be changed in animation.js, which ended up fixing this bug!
Sorry about that. There are plenty other bugs in the devtools you can help with if you want. In particular, this lists all either good-first or mentored bugs:
https://bugzilla.mozilla.org/buglist.cgi?bug_status=NEW&component=Developer%20Tools&component=Developer%20Tools%3A%203D%20View&component=Developer%20Tools%3A%20Canvas%20Debugger&component=Developer%20Tools%3A%20Console&component=Developer%20Tools%3A%20Debugger&component=Developer%20Tools%3A%20Framework&component=Developer%20Tools%3A%20Graphic%20Commandline%20and%20Toolbar&component=Developer%20Tools%3A%20Inspector&component=Developer%20Tools%3A%20Memory&component=Developer%20Tools%3A%20Netmonitor&component=Developer%20Tools%3A%20Object%20Inspector&component=Developer%20Tools%3A%20Responsive%20Mode&component=Developer%20Tools%3A%20Scratchpad&component=Developer%20Tools%3A%20Source%20Editor&component=Developer%20Tools%3A%20Storage%20Inspector&component=Developer%20Tools%3A%20Style%20Editor&component=Developer%20Tools%3A%20User%20Stories&component=Developer%20Tools%3A%20Web%20Audio%20Editor&component=Developer%20Tools%3A%20WebGL%20Shader%20Editor&component=Developer%20Tools%3A%20WebIDE&email1=nobody&emailassigned_to1=1&emailtype1=substring&f1=bug_mentor&f2=status_whiteboard&j_top=OR&list_id=12227152&o1=isnotempty&o2=anywords&product=Firefox&query_format=advanced&resolution=---&v2=good%20first&query_based_on=&columnlist=component%2Cshort_desc%2Cbug_mentor
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(pbrosset)
Resolution: --- → WORKSFORME
Oh, gee. Thanks Patrick. That helps... :)
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: