Closed Bug 1468343 Opened 6 years ago Closed 6 years ago

Very long duration transition makes animation inspector unresponsive

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62 fixed, firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: hiro, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(9 files)

long-transition.html
354 bytes, text/html
Details
infinity-duration-all.png
32.21 KB, image/png
Details
with-delay.png
23.97 KB, image/png
Details
no-delay.png
13.38 KB, image/png
Details
mix-duration.png
38.80 KB, image/png
Details
Bug 1468343 - Part 1: Make graph to address infinity duration.
59 bytes, text/x-review-board-request
pbro
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1468343 - Part 2: Change tick label to address infinity duration.
59 bytes, text/x-review-board-request
jdescottes
: review+
flod
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1468343 - Part 3: Change tooltip content to address infinity duration.
59 bytes, text/x-review-board-request
jdescottes
: review+
flod
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1468343 - Part 4: Add test for infinity duration.
59 bytes, text/x-review-board-request
jdescottes
: review+
lizzard
: approval-mozilla-beta+
Details
Attached file long-transition.html — 
      No description provided.
Thanks Hiro.

The cause looks the duration is Infinity. Though the previous animation inspector as well looks to have same problem since we did not handle the Infinity duration, we need to fix.
Blocks: 1399830
Priority: -- → P1
Product: Firefox → DevTools
Summary: Very long duration transition makes animation inspector unresponsilbe → Very long duration transition makes animation inspector unresponsive
Assignee: nobody → dakatsuka
I attached the screenshot of the prototype which all duration is infinity.
The new features for this are:

* Set linear-gradient to the path which is for infinity duration animation.
* Use "∞" to tick label.
Attached image with-delay.png — 
This screenshot has that animation has delay and iterationStart.
Attached image no-delay.png — 
This animation has no delay and iterationStart.
Attached image mix-duration.png — 
This final screenshot is that mixes infinity duration and limited duration.
The last tick label is not "∞".

I'd like to proceed with this features in this time.
(In reply to Daisuke Akatsuka (:daisuke) from comment #4)
> Created attachment 8986699 [details]
> no-delay.png
> 
> This animation has no delay and iterationStart.

Is this graph supposed to be empty in this image?
(In reply to Brian Birtles (:birtles) from comment #6)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #4)
> > Created attachment 8986699 [details]
> > no-delay.png
> > 
> > This animation has no delay and iterationStart.
> 
> Is this graph supposed to be empty in this image?

Yes, since the progress of animation is zero.
However, as discussed on f2f, we will set a stroke line to the graph in another bug to show the animation progress.
Thanks!
Filed bug 1470345.
Blocks: 1470345
Comment on attachment 8986979 [details]
Bug 1468343 - Part 3: Change tooltip content to address infinity duration.

Sorry, please let me cancel this review since the try was failed.
I have changed the animationinspector.properties, but the old animation inspector had been affected.
I'll fix soon.
Attachment #8986979 - Flags: review?(jdescottes)
Rebased the patches.
Comment on attachment 8986977 [details]
Bug 1468343 - Part 1: Make graph to address infinity duration.

https://reviewboard.mozilla.org/r/252232/#review259196

::: devtools/client/inspector/animation/utils/timescale.js:46
(Diff revision 3)
>        const toRate = v => v / playbackRate;
>        const startTime = createdTime + toRate(Math.min(delay, 0));
> -      const endTime = createdTime +
> +      let endTime = 0;
> +
> +      if (duration === Infinity) {
> +        endTime = createdTime + (delay > 0 ? delay * 2 : 1);

This line needs a comment. So readers understand why we multiply the delay by 2 if there is delay. And why we add 1 otherwise.
Attachment #8986977 - Flags: review?(pbrosset) → review+
Comment on attachment 8986978 [details]
Bug 1468343 - Part 2: Change tick label to address infinity duration.

https://reviewboard.mozilla.org/r/252234/#review259316

Looks good. One small doubt regarding localization.

::: devtools/client/locales/en-US/animationinspector.properties:40
(Diff revision 3)
>  # This string is displayed in each animation player widget. It is the label
>  # displayed before the animation duration.
>  player.animationDurationLabel=Duration:
>  
> +# LOCALIZATION NOTE (player.infiniteDurationText):
> +# This string is displayed in a tooltip on animation player widget in case

This string is both used in the timeline and in the tooltip. 

We usually avoid using the same localized strings in different parts of the UI. Maybe since the string is extremely simple it's fine to do so here. I also don't know if ∞ should in general be localized? Will ping :flod for l10n review.
Attachment #8986978 - Flags: review?(jdescottes) → review+
Comment on attachment 8986979 [details]
Bug 1468343 - Part 3: Change tooltip content to address infinity duration.

https://reviewboard.mozilla.org/r/252236/#review259324

::: devtools/client/locales/en-US/animationinspector.properties:81
(Diff revision 3)
>  # This string is displayed in a tooltip that appears when hovering over
>  # animations in the timeline. It is the label displayed before the animation
>  # iterationStart value.
>  # %1$S will be replaced by the original iteration start value
>  # %2$S will be replaced by the actual time of iteration start
> -player.animationIterationStartLabel=Iteration start: %1$S (%2$Ss)
> +player.animationIterationStartLabel=Iteration start: %1$S (%2$S)

For such a change, we need to update the string name, so that localizers can also update their version and remove the unit.

-> player.animationIterationStartLabel2
Attachment #8986979 - Flags: review?(jdescottes) → review+
Comment on attachment 8986980 [details]
Bug 1468343 - Part 4: Add test for infinity duration.

https://reviewboard.mozilla.org/r/252238/#review259326

Code looks good to me, and this avoid crashing Firefox so that's great :) 

For the feature itself I don't know how much effort we want to put into those "infinite" animations.
Some things feel a bit weird right now:
- not showing the "Infinity" sign when we have both infinite and limited animations
- the different representations (gradient, or empty)

Is there a real reason for the user to have infinite durations? 
If not, maybe we should show a warning or something?

::: devtools/client/inspector/animation/test/browser_animation_infinity-duration_current-time-scrubber.js:1
(Diff revision 3)
> +"use strict";

Could we add a License header? (same comment for other files)

::: devtools/client/inspector/animation/test/browser_animation_infinity-duration_current-time-scrubber.js:14
(Diff revision 3)
> +
> +  info("Set initial state");
> +  await clickOnCurrentTimeScrubberController(animationInspector, panel, 0);
> +  const initialCurrentTime = animationInspector.state.animations[0].state.currentTime;
> +
> +  info("Check whether the animation currentTime was gained");

Do you mean "updated" or "increased" rather than "gained"?

::: devtools/client/inspector/animation/test/browser_animation_infinity-duration_current-time-scrubber.js:26
(Diff revision 3)
> +  const barEl = areaEl.querySelector(".keyframes-progress-bar");
> +  const controllerBounds = areaEl.getBoundingClientRect();
> +  const barBounds = barEl.getBoundingClientRect();
> +  const barX = barBounds.x + barBounds.width / 2 - controllerBounds.x;
> +  const expectedBarX = controllerBounds.width * 0.5;
> +  ok(expectedBarX - 1 < barX && barX < expectedBarX + 1,

nit: I personally find 
  Math.abs(barX - expectedBarX) < 1
  
easier to understand, but that's probably very subjective.

::: devtools/client/inspector/animation/test/browser_animation_infinity-duration_tick-label.js:19
(Diff revision 3)
> +    "The content should not be \u221E"
> +  );
> +
> +  info("Check the tick label content with infinity duration animation only");
> +  await selectNodeAndWaitForAnimations(".infinity", inspector);
> +  is(

For information, when running this in --headless --verify, it seems to fail in chaos mode on :

FAIL The content should be ∞ - Got 0ms, expected ∞

Not sure where this might come from, might be worth having a look before landing.
Attachment #8986980 - Flags: review?(jdescottes) → review+
Comment on attachment 8986977 [details]
Bug 1468343 - Part 1: Make graph to address infinity duration.

https://reviewboard.mozilla.org/r/252232/#review259196

> This line needs a comment. So readers understand why we multiply the delay by 2 if there is delay. And why we add 1 otherwise.

Thanks, Patrick!
Ah, yes, I'll add a comment.
Comment on attachment 8986978 [details]
Bug 1468343 - Part 2: Change tick label to address infinity duration.

https://reviewboard.mozilla.org/r/252234/#review259316

> This string is both used in the timeline and in the tooltip. 
> 
> We usually avoid using the same localized strings in different parts of the UI. Maybe since the string is extremely simple it's fine to do so here. I also don't know if ∞ should in general be localized? Will ping :flod for l10n review.

I see, I'll add one more variable.
Also, I request the review to :flod as well.
Thanks!
Comment on attachment 8986979 [details]
Bug 1468343 - Part 3: Change tooltip content to address infinity duration.

https://reviewboard.mozilla.org/r/252236/#review259324

> For such a change, we need to update the string name, so that localizers can also update their version and remove the unit.
> 
> -> player.animationIterationStartLabel2

I see, thanks.
Comment on attachment 8986980 [details]
Bug 1468343 - Part 4: Add test for infinity duration.

https://reviewboard.mozilla.org/r/252238/#review259326

Thanks Julian!

> - not showing the "Infinity" sign when we have both infinite and limited animations

In the case of mixing, I thought that it would be easier to understand if the time of limited duration was displayed. (Infinity sign will not be the hint for limited duration animation)

> - the different representations (gradient, or empty)

This shoudl fix in bug 1470345.

> Is there a real reason for the user to have infinite durations? 
> If not, maybe we should show a warning or something?

Yeah, however, since the Web Animations spec allows Infinity[1] for the duration, we don't need the warning, I thing.

[1] https://drafts.csswg.org/web-animations/#dom-effecttiming-duration

> Could we add a License header? (same comment for other files)

Oh, I'm sorry!

> Do you mean "updated" or "increased" rather than "gained"?

Let me use `increased`, thanks!

> nit: I personally find 
>   Math.abs(barX - expectedBarX) < 1
>   
> easier to understand, but that's probably very subjective.

Yep!

> For information, when running this in --headless --verify, it seems to fail in chaos mode on :
> 
> FAIL The content should be ∞ - Got 0ms, expected ∞
> 
> Not sure where this might come from, might be worth having a look before landing.

Thank you for the information.
Let me check.
Comment on attachment 8986980 [details]
Bug 1468343 - Part 4: Add test for infinity duration.

https://reviewboard.mozilla.org/r/252238/#review259326

> Thank you for the information.
> Let me check.

I found the reason, and fixed in the updated patch.
https://reviewboard.mozilla.org/r/252234/diff/3-4/
Comment on attachment 8986979 [details]
Bug 1468343 - Part 3: Change tooltip content to address infinity duration.

https://reviewboard.mozilla.org/r/252236/#review259538

r+ with the obsolete string removed

In general it's bad to reuse a string in different context, because different locales could need different forms (e.g. declarative vs imperative tone, describing what the element does vs telling the program to do something). 

But I'm looking at the code fragments here, and the amount of harcoded spaces and concatenations, and I'm afraid this is the least of our problems :-\

::: devtools/client/locales/en-US/animationinspector.properties:81
(Diff revision 4)
>  # This string is displayed in a tooltip that appears when hovering over
>  # animations in the timeline. It is the label displayed before the animation
>  # iterationStart value.
>  # %1$S will be replaced by the original iteration start value
>  # %2$S will be replaced by the actual time of iteration start
>  player.animationIterationStartLabel=Iteration start: %1$S (%2$Ss)

If this string is not used anymore, it should be removed from the .properties file

::: devtools/client/locales/en-US/animationinspector.properties:88
(Diff revision 4)
> +# LOCALIZATION NOTE (player.animationIterationStartLabel2):
> +# This string is displayed in a tooltip that appears when hovering over
> +# animations in the timeline. It is the label displayed before the animation
> +# iterationStart value.
> +# %1$S will be replaced by the original iteration start value
> +# %2$S will be replaced by the actual time of iteration start without time unit

Can you provide an example with numbers of the resulting string? It would clarify things a lot.
Attachment #8986979 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8986978 [details]
Bug 1468343 - Part 2: Change tick label to address infinity duration.

https://reviewboard.mozilla.org/r/252234/#review259536

::: devtools/client/locales/en-US/animationinspector.properties:111
(Diff revision 4)
>  # time (in seconds too);
>  player.timeLabel=%Ss
>  
> +# LOCALIZATION NOTE (player.infiniteDurationText):
> +# This string is displayed in animation player widget, in case of the animation
> +# duration is infinity.

Not sure about the wording of the comment, maybe "…in case the duration of the animation is infinite"?

Using ∞ shouldn't be an issue, we are already using it in this file for iteration count.

https://transvision.mozfr.org/string/?entity=devtools/client/animationinspector.properties:player.infiniteIterationCountText&repo=gecko_strings
Attachment #8986978 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8986979 [details]
Bug 1468343 - Part 3: Change tooltip content to address infinity duration.

https://reviewboard.mozilla.org/r/252236/#review259538

> If this string is not used anymore, it should be removed from the .properties file

Thank you very much, Francesco!
Ah, this is still using in old animation inspector[1].
So let me leave as it is. 
I will remove this in bug 1463621 which removes old animation inspector. Thanks!

[1] https://searchfox.org/mozilla-central/source/devtools/client/inspector/animation-old/components/animation-time-block.js#245

> Can you provide an example with numbers of the resulting string? It would clarify things a lot.

Okay!
Comment on attachment 8986978 [details]
Bug 1468343 - Part 2: Change tick label to address infinity duration.

https://reviewboard.mozilla.org/r/252234/#review259536

> Not sure about the wording of the comment, maybe "…in case the duration of the animation is infinite"?
> 
> Using ∞ shouldn't be an issue, we are already using it in this file for iteration count.
> 
> https://transvision.mozfr.org/string/?entity=devtools/client/animationinspector.properties:player.infiniteIterationCountText&repo=gecko_strings

Thanks, I'll update the comment.
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5946aca6ec6d
Part 1: Make graph to address infinity duration. r=pbro
https://hg.mozilla.org/integration/autoland/rev/e4e19711c3dd
Part 2: Change tick label to address infinity duration. r=flod,jdescottes
https://hg.mozilla.org/integration/autoland/rev/b3943b9b71c5
Part 3: Change tooltip content to address infinity duration. r=flod,jdescottes
https://hg.mozilla.org/integration/autoland/rev/1c464a721c0e
Part 4: Add test for infinity duration. r=jdescottes
Comment on attachment 8986977 [details]
Bug 1468343 - Part 1: Make graph to address infinity duration.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1399830
[User impact if declined]: When user see the animation of infinite duration, the animation inspector will be freezed.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: It has baked on Nightly for 2 days.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: bug 1449477
[Is the change risky?]: Low
[Why is the change risky/not risky?]: All automated tests were green for 2 days. Also since this affects only animation inspector.
[String changes made/needed]: None
Attachment #8986977 - Flags: approval-mozilla-beta?
Attachment #8986978 - Flags: approval-mozilla-beta?
Attachment #8986979 - Flags: approval-mozilla-beta?
Attachment #8986980 - Flags: approval-mozilla-beta?
(In reply to Daisuke Akatsuka (:daisuke) from comment #51)
> [String changes made/needed]: None

There are string changes (i.e. new strings) in these patches…

For the record, I'm OK with uplifting this.
(In reply to Francesco Lodolo [:flod] from comment #52)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #51)
> > [String changes made/needed]: None
> 
> There are string changes (i.e. new strings) in these patches…
> 
> For the record, I'm OK with uplifting this.

Ah! Thank you for the notice!

[String changes made/needed]: 
Add new strings for both the tooltip on animation graph and the label of timeline header.
Comment on attachment 8986977 [details]
Bug 1468343 - Part 1: Make graph to address infinity duration.

OK to uplift for beta 5 for an improvement to this feature since we're still in early beta and this only affects the feature - new strings also approved by l10n team.
Attachment #8986977 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8986978 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8986979 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8986980 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: