Closed Bug 1463372 Opened 2 years ago Closed 2 years ago

Keyframes progress bar should be based on createdTime

Categories

(DevTools :: Inspector: Animations, enhancement)

enhancement
Not set
normal

Tracking

(firefox61 disabled, firefox62 verified)

VERIFIED FIXED
Firefox 62
Tracking Status
firefox61 --- disabled
firefox62 --- verified

People

(Reporter: daisuke, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Although we fixed bug 1454392, the keyframes progress bar had not been based on createdTime yet. We need fix.
Comment on attachment 8979486 [details]
Bug 1463372 - Part 1: Introduce createdTime to keyframes progress bar.

https://reviewboard.mozilla.org/r/245654/#review252526

::: devtools/client/inspector/animation/components/KeyframesProgressBar.js
(Diff revision 1)
>  
>    updateOffset(currentTime, animation, timeScale) {
>      const {
> +      createdTime,
>        playbackRate,
> -      previousStartTime = 0,

Can you keep previousStartTime here? Since you're already getting createdTime and playbackRate from animation.state here, you might as well keep previousStartTime too.
Attachment #8979486 - Flags: review?(pbrosset) → review+
Comment on attachment 8979487 [details]
Bug 1463372 - Part 2: Test for createdTime based keyframes progress bar.

https://reviewboard.mozilla.org/r/245656/#review252530

::: devtools/client/inspector/animation/test/browser_animation_keyframes-progress-bar_after-resuming.js:16
(Diff revision 1)
> +  const { animationInspector, panel } = await openAnimationInspector();
> +
> +  const scrubberPositions = [0, 0.25, 0.5, 0.75];
> +  const expectedPositions = [0, 0.25, 0.5, 0.75];
> +
> +  info("Check whether the keyframes progress bar position was correctly");

s/correctly/correct

::: devtools/client/inspector/animation/test/browser_animation_keyframes-progress-bar_after-resuming.js:19
(Diff revision 1)
> +  const expectedPositions = [0, 0.25, 0.5, 0.75];
> +
> +  info("Check whether the keyframes progress bar position was correctly");
> +  await assertPosition(panel, scrubberPositions, expectedPositions, animationInspector);
> +
> +  info("Check whether the keyframes progress bar position was correctly " +

s/correctly/correct
Attachment #8979487 - Flags: review?(pbrosset) → review+
Comment on attachment 8979486 [details]
Bug 1463372 - Part 1: Introduce createdTime to keyframes progress bar.

https://reviewboard.mozilla.org/r/245654/#review252526

> Can you keep previousStartTime here? Since you're already getting createdTime and playbackRate from animation.state here, you might as well keep previousStartTime too.

Thanks, Patrick.

Oh, I had done same way in bug 1454392 since actually we don't need to refer the variable, in case of latest version.
https://reviewboard.mozilla.org/r/242616/diff/4#index_header

How do you think?
(In reply to Daisuke Akatsuka (:daisuke) from comment #6)
> Comment on attachment 8979486 [details]
> Bug 1463372 - Part 1: Introduce createdTime to keyframes progress bar.
> 
> https://reviewboard.mozilla.org/r/245654/#review252526
> 
> > Can you keep previousStartTime here? Since you're already getting createdTime and playbackRate from animation.state here, you might as well keep previousStartTime too.
> 
> Thanks, Patrick.
> 
> Oh, I had done same way in bug 1454392 since actually we don't need to refer
> the variable, in case of latest version.
> https://reviewboard.mozilla.org/r/242616/diff/4#index_header
> 
> How do you think?

Let me make the patches to land as it is.
Then if we need to fix this change, I'd like to fix all at the same time.
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1be8a677b22
Part 1: Introduce createdTime to keyframes progress bar. r=pbro
https://hg.mozilla.org/integration/autoland/rev/4d264b49431f
Part 2: Test for createdTime based keyframes progress bar. r=pbro
https://hg.mozilla.org/mozilla-central/rev/b1be8a677b22
https://hg.mozilla.org/mozilla-central/rev/4d264b49431f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Hello,

Considering that the 3rd comment from bug 1464369, it seems that these 2 bugs are the same issue. 
Using the same steps to reproduce from there, this issue is now fixed Nightly v62.0a1 (20180528220216). Pausing and playing will not make the property scrubber behave incorrectly.
This issue has been validated on Windows 10, Ubuntu 16.04 LTS, and Mac OS X 10.12.6.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1464380
Duplicate of this bug: 1464369
Duplicate of this bug: 1464375
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.