Closed Bug 1426194 Opened 2 years ago Closed 2 years ago

Animation Inspector freezes the browser

Categories

(DevTools :: Inspector: Animations, defect, P1)

defect

Tracking

(firefox-esr52 unaffected)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox-esr52 --- unaffected

People

(Reporter: Kwan, Assigned: daisuke)

References

Details

(Keywords: regression)

Attachments

(2 files)

STR:
1) Go to https://duckduckgo.com/?q=css+animations
2) Open DevTools Inspector
3) Select the Animations panel
4) The browser is now frozen (page animations continue to visibly run though)

Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bb63705ec95e878172e8f4c4628e11814683ffdc&tochange=c3530ca8ede501e49fdd9efb7e5d7f72df7f522b

Regressed by bug 1422218.
Flags: needinfo?(dakatsuka)
Thank you for the reporting.
I could reproduce it.
Will fix as soon as possible, thanks!
Flags: needinfo?(dakatsuka)
Comment on attachment 8938273 [details]
Bug 1426194 - Part 2: Add test.

https://reviewboard.mozilla.org/r/209042/#review214762
Attachment #8938273 - Flags: review?(pbrosset) → review+
Comment on attachment 8938272 [details]
Bug 1426194 - Part 1: Correspond to the keyframes which have same offset.

https://reviewboard.mozilla.org/r/209040/#review214764

::: devtools/client/animationinspector/graph-helper.js:269
(Diff revision 1)
> +      // The computed value on the time which represents keyframe offset changes to the
> +      // value which is written in the keyframe.
> +      // However, we can set keyframes that have same offset.
> +      // In this case, keyframes A, B which is same offset animate as  the animation from
> +      // previous keyframe of A to A and from B to next B.
> +      // However, the both computed values will be value of B since the offsets are same.
> +      // To correspond this, each distance of keyframes calculate from time of start
> +      // keyframe to end keyframe - BOUND_EXCLUDING_TIME except same offset.

Sorry Daisuke, the code changes look good to me, but I don't understand this comment clearly enough to R+ this.
Could you try to explain to me again what's happening in the bug, and how do we solve it here. 
In fact, this explanation would be good to have as a multi-line commit message too.
I just want to be sure I R+ the right thing.
Thanks!
Attachment #8938272 - Flags: review?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #6)
> Comment on attachment 8938272 [details]
> Bug 1426194 - Part 1: Correspond to the keyframes which have same offset.
> 
> https://reviewboard.mozilla.org/r/209040/#review214764
> 
> ::: devtools/client/animationinspector/graph-helper.js:269
> (Diff revision 1)
> > +      // The computed value on the time which represents keyframe offset changes to the
> > +      // value which is written in the keyframe.
> > +      // However, we can set keyframes that have same offset.
> > +      // In this case, keyframes A, B which is same offset animate as  the animation from
> > +      // previous keyframe of A to A and from B to next B.
> > +      // However, the both computed values will be value of B since the offsets are same.
> > +      // To correspond this, each distance of keyframes calculate from time of start
> > +      // keyframe to end keyframe - BOUND_EXCLUDING_TIME except same offset.
> 
> Sorry Daisuke, the code changes look good to me, but I don't understand this
> comment clearly enough to R+ this.
> Could you try to explain to me again what's happening in the bug, and how do
> we solve it here. 
> In fact, this explanation would be good to have as a multi-line commit
> message too.
> I just want to be sure I R+ the right thing.
> Thanks!

Thanks Patrick! Okay!
Assignee: nobody → dakatsuka
Status: NEW → ASSIGNED
Priority: -- → P1
59 merges to beta next week I believe, Daisuke, do you want me to review the new commits you pushed?
Flags: needinfo?(dakatsuka)
Yes, thanks!
Flags: needinfo?(dakatsuka)
Attachment #8938272 - Flags: review?(pbrosset)
Comment on attachment 8938272 [details]
Bug 1426194 - Part 1: Correspond to the keyframes which have same offset.

https://reviewboard.mozilla.org/r/209040/#review215760

Thank you for the long explanation :)
Attachment #8938272 - Flags: review?(pbrosset) → review+
Thank you very much!
I'll land soon.
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4a89ebfbb40
Part 1: Correspond to the keyframes which have same offset. r=pbro
https://hg.mozilla.org/integration/autoland/rev/e4f2f93a78f0
Part 2: Add test. r=pbro
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/1230c90d4d99
Part 1: Correspond to the keyframes which have same offset. r=pbro
https://hg.mozilla.org/mozilla-central/rev/209dc7135305
Part 2: Add test. r=pbro
https://hg.mozilla.org/mozilla-central/rev/1230c90d4d99
https://hg.mozilla.org/mozilla-central/rev/209dc7135305
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.