Closed
Bug 1120852
Opened 10 years ago
Closed 10 years ago
The animations panel doesn't work when there is an animation-delay (timeline is de-synchronized)
Categories
(DevTools :: Inspector: Animations, defect)
DevTools
Inspector: Animations
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 38
People
(Reporter: pbro, Assigned: pbro)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
pbro
:
review+
pbro
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
pbro
:
review+
pbro
:
checkin+
|
Details | Diff | Splinter Review |
STR:
- open https://bug1117603.bugzilla.mozilla.org/attachment.cgi?id=8543711
- open the inspector
- switch to the animations sidebar panel
- select the animated node
you might have to reload the page once you've selected the node as the animation may have already ended by then (the node will remain selected when you reload the page).
ER: the timeline progress is synchronized with the animation itself
AR: the timeline starts right away, not waiting for the 2s delay, and therefore finishes before the animation actually finishes.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → pbrosset
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8548229 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•10 years ago
|
Attachment #8548230 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 3•10 years ago
|
||
Try build for both patches: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4230d999543e
Comment 4•10 years ago
|
||
Comment on attachment 8548229 [details] [diff] [review]
bug1120852-1-expose-delay-on-actor.patch
Review of attachment 8548229 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/devtools/server/actors/animation.js
@@ +126,5 @@
> + } else {
> + return 0;
> + }
> +
> + if (delayText.indexOf(",") !== -1) {
We need test coverage for this case (multiple animation / transition delays separated by commas).
Attachment #8548229 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Thanks Brian for the review. This new version includes a new test file to ensure multiple delays, duration, counts were retrieved correctly by the actor.
New try build for this 1st part: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4a7e0b1ea6e
Attachment #8548229 -
Attachment is obsolete: true
Attachment #8548773 -
Flags: review+
Assignee | ||
Comment 6•10 years ago
|
||
part 1 in fx-team: https://hg.mozilla.org/integration/fx-team/rev/5ff5e6e6623b
Keywords: leave-open
Assignee | ||
Updated•10 years ago
|
Attachment #8548773 -
Flags: checkin+
Comment 7•10 years ago
|
||
Comment on attachment 8548230 [details] [diff] [review]
bug1120852-2-dont-start-animation-timeline-before-delay.patch
Review of attachment 8548230 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/devtools/animationinspector/test/browser_animation_time_formatting.js
@@ +6,5 @@
> +
> +// Test the time formatting helper function.
> +
> +add_task(function*() {
> + yield addTab(TEST_URL_ROOT + "doc_simple_animation.html");
This getFormattedTime function seems like something that could be decoupled from the UI and tested with a unit test instead of a mochitest (possibly moved into viewhelpers or exported separately in one of the animationinspector files rather than attached to the widget instance). Usually we just end up doing it in a mochitest like this so I wouldn't stop it from landing because of this, but it would speed up our test harness and allow cross-tool reuse if things like this were moved.
@@ +13,5 @@
> + let widget = panel.playerWidgets[0];
> +
> + is(widget.getFormattedTime(0), "0.00");
> + is(widget.getFormattedTime(1000), "1.00");
> + is(widget.getFormattedTime(5.123765827635872635), "0.00");
Nit: Add test cases with a number (plus decimal) in the 10s and 100s
Attachment #8548230 -
Flags: review?(bgrinstead) → review+
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•10 years ago
|
||
You're right, I should have moved this to a unit test. Maybe what I'll do is do this in a separate patch so that I can land part 2 quickly (since try is green), and then land this new unit test separately.
In any case, thanks for the reviews.
Comment 9•10 years ago
|
||
Flags: in-testsuite+
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #7)
> This getFormattedTime function seems like something that could be decoupled
> from the UI and tested with a unit test instead of a mochitest (possibly
> moved into viewhelpers or exported separately in one of the
> animationinspector files rather than attached to the widget instance).
> Usually we just end up doing it in a mochitest like this so I wouldn't stop
> it from landing because of this, but it would speed up our test harness and
> allow cross-tool reuse if things like this were moved.
In fact, I just came across toLocaleString (which now seems to be fully implemented, that didn't seem to be the case some time ago), and using it simplifies massively the time formatting function:
getFormattedTime: function(time) {
return (time/1000).toLocaleString(undefined, {
minimumFractionDigits: 2,
maximumFractionDigits: 2
});
},
In fact, now that only really use this native function, there's no real need to test it alone with a unit test.
What I'm going to do though is file a separate bug to refactor ViewHelpers.L10N.numberWithDecimals which has its own implementation for now.
Assignee | ||
Comment 11•10 years ago
|
||
Part 2 in fx-team: https://hg.mozilla.org/integration/fx-team/rev/d0b17fe68e83
Attachment #8548230 -
Attachment is obsolete: true
Attachment #8549489 -
Flags: review+
Attachment #8549489 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Assignee | ||
Updated•9 years ago
|
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•