1392157 - Show a burst across the tab when a page has finished loading

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

This bug is split off from bug 1352119. Bug 1352119 specs a burst animation that should run when a page has finished loading. This bug is on file to fix up the performance of this animation so we can land it.

The burst animation and any performance improvements related to it will land as part of this bug.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

This is the baseline try push. This push shows the results of the tab loading indicator without the burst animation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18.

Comparing the baseline to the r+'d burst animation patch: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=33eedaf8383870de6436001236035104cbba0d0e&framework=1

Fixing some review nits
Baseline vs burst + review nits: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=f7b6eb2189678b5c939f1700a645d1b82f411c57&framework=1
Interdiff (Burst vs burst + review nits): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=33eedaf8383870de6436001236035104cbba0d0e&newProject=try&newRevision=f7b6eb2189678b5c939f1700a645d1b82f411c57&framework=1

I have implemented a few different optimizations. Each one has separate compare-talos results. All of these build on top of the previous, for example #5 includes #1-4. I have included links to interdiffs comparing the current revision to the previous one, for example #3 vs #2 to see if there is local improvement.

1. Don't force a synchronous layout flush when re-running the animation:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=21781ac8d450d285f7d2cdb6453404efe9a96746&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f7b6eb2189678b5c939f1700a645d1b82f411c57&newProject=try&newRevision=21781ac8d450d285f7d2cdb6453404efe9a96746&framework=1

2. Don't run the burst animation if the page finishes loading within 150ms:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=0cb66da1c6c7d56381f03f5660ca55477f1f6ea8&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=21781ac8d450d285f7d2cdb6453404efe9a96746&newProject=try&newRevision=0cb66da1c6c7d56381f03f5660ca55477f1f6ea8&framework=1

3. Don't run the burst animation if a tab animation is in progress:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=a2a274712b74ca33e32a7b216e16d40f50bf4f21&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0cb66da1c6c7d56381f03f5660ca55477f1f6ea8&newProject=try&newRevision=a2a274712b74ca33e32a7b216e16d40f50bf4f21&framework=1

4. Wait until the session has been restored before running the burst animation:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=a293684e733c84f10e442d414ae9b5f7c8d7de14&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a2a274712b74ca33e32a7b216e16d40f50bf4f21&newProject=try&newRevision=a293684e733c84f10e442d414ae9b5f7c8d7de14&framework=1

5. Remove the bursting attribute when a tab is closing:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=00cd5bad7e63062895ec937bb84be9e4f9ebd769&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a293684e733c84f10e442d414ae9b5f7c8d7de14&newProject=try&newRevision=00cd5bad7e63062895ec937bb84be9e4f9ebd769&framework=1

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

6. Remove import of BrowserUtils since it is already in the global scope:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=ab992f0db5f479da199256233341dd2fec4b0eb0&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=00cd5bad7e63062895ec937bb84be9e4f9ebd769&newProject=try&newRevision=ab992f0db5f479da199256233341dd2fec4b0eb0&framework=1

7. Cancel animation frame requests if the burst animation is triggered before a previous one can start:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=52c8d72ea59e73ab040bee7b2a4c0ba9b15c6424&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ab992f0db5f479da199256233341dd2fec4b0eb0&newProject=try&newRevision=52c8d72ea59e73ab040bee7b2a4c0ba9b15c6424&framework=1

Comment 3

[u595893]

Will we have a pref to disable this and the new tab animation(look nice but still want to disable it) which landed today?
It's distracting and just want to disable both of them.

Animations are pretty good looking but don't want animations so an option to disable it on my slow netbook will be appreciated.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to shellye from comment #3)
> Will we have a pref to disable this and the new tab animation(look nice but
> still want to disable it) which landed today?
> It's distracting and just want to disable both of them.
> 
> Animations are pretty good looking but don't want animations so an option to
> disable it on my slow netbook will be appreciated.

Yes, you can go to about:config and set toolkit.cosmeticAnimations.enabled to `false`. Doing so will disable tab animations, the burst added by this bug, stop/reload animation, notification bar animations, and potentially some others. We do not have more fine-grained prefs for disabling animations.

Comment 5

[u595893]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> (In reply to shellye from comment #3)
> > Will we have a pref to disable this and the new tab animation(look nice but
> > still want to disable it) which landed today?
> > It's distracting and just want to disable both of them.
> > 
> > Animations are pretty good looking but don't want animations so an option to
> > disable it on my slow netbook will be appreciated.
> 
> Yes, you can go to about:config and set toolkit.cosmeticAnimations.enabled
> to `false`. Doing so will disable tab animations, the burst added by this
> bug, stop/reload animation, notification bar animations, and potentially
> some others. We do not have more fine-grained prefs for disabling animations.

Have set that pref but still tab throbber animates.

more fine-grained prefs for disabling animations would be welcome as like download notifications but not others but currently it's all or nothing.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

> more fine-grained prefs for disabling animations would be welcome as like download notifications but not others but currently it's all or nothing.

Please, familiarize yourself with bugzilla etiquette: https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

This is not a request forum, this is not a discussion group. Do not set a NI on an engineer working on a specific bug to provide you a different feature that you'd wish to have.

:jaws said "We do not have more fine-grained prefs for disabling animations". If you want a feature, file a bug. If possible, provide a patch as well. Bugzilla is a technical tool for engineers to work in. Using it for different purposes actually harms our productivity.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

In optimization #2 above, a TART regression is introduced. I believe some of that may be due to the usage of performance.now(), which is about 17% slower than Date.now() on my local machine as measured by https://jsperf.com/perf-vs-date/1

8. Switch to using Date.now() from performance.now() since Date.now() is roughly 17% faster on my local machine and for tab loading times we don't need to worry about microsecond precision and a monotonically increasing clock isn't required for this calculation:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=893ae001aa0703b263b6b77b640101f54fac54be&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=52c8d72ea59e73ab040bee7b2a4c0ba9b15c6424&newProject=try&newRevision=893ae001aa0703b263b6b77b640101f54fac54be&framework=1

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to shellye from comment #5)
> Have set that pref but still tab throbber animates.

The tab throbber will continue to animate even if the pref is set to false, because the tab throbber is essential to knowing when a page is loading and has completed loading. We also will not disable the download animation even if this pref is set to false because otherwise the user will not know download progress.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

The patch from bug 1352119 had an animation-duration of 1.8s, of which about 1.4s was spent animating the burst outside of the tab shape due to the animation-timing-function that was being used. I talked to UX and we were able to adjust the timing-function and in turn lower the duration of the animation from 1.8s to 375ms.

Now that the duration has been lowered to 375ms, we are much less likely to have overlapping burst animations. This also means that if one is playing we can simply skip playing an overlapping animation, simplifying our code as well as removing some code that was shown to add some large regressions in #1.

Through logging I measured that gBrowser.tabAnimationsInProgress was truthy for 85% of TART. We saw the largest regression in TART when I added the 150ms check in #2.

The CSS animation uses the `transform` property, and we can use the `will-change` property to hint that this will change and optimize for the animation. I don't expect this to show any changes in the performance but it is worthwhile to get confirmation.

9. Reduce the animation-duration to 375ms and tweak the timing-function:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=22f39bf16623e8a0e144b979e633461cd2003a0e&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=893ae001aa0703b263b6b77b640101f54fac54be&newProject=try&newRevision=22f39bf16623e8a0e144b979e633461cd2003a0e&framework=1

10. Don't restart the animation if it is already in progress:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=106b5b9e6dd05b2a905630d438a9e4453ef7534a&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=22f39bf16623e8a0e144b979e633461cd2003a0e&newProject=try&newRevision=106b5b9e6dd05b2a905630d438a9e4453ef7534a&framework=1

11. Don't take into account how long it took to load a page before showing the burst:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=82af5c25dce708448f939b569b964e1326db6fe5&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=106b5b9e6dd05b2a905630d438a9e4453ef7534a&newProject=try&newRevision=82af5c25dce708448f939b569b964e1326db6fe5&framework=1

12. Use the `will-change` CSS property:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=74721376c4ee870db20702284104c76af9af90cd&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=82af5c25dce708448f939b569b964e1326db6fe5&newProject=try&newRevision=74721376c4ee870db20702284104c76af9af90cd&framework=1

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1392157 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes.

Review commit: https://reviewboard.mozilla.org/r/172172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172172/

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1392157 - Performance improvements to burst animation.

Review commit: https://reviewboard.mozilla.org/r/172174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172174/

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Some of the regressions shown in the previous pushes are regressions in memory use. After talking to erahm and jmaher, I triggered AWSY tests to run because they are more reliable than the tp5 test suite for memory tracking.

This is a link to the AWSY perfherder results comparing the baseline vs #7 change from above. The comparison shows no important (2%+) change:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=52c8d72ea59e73ab040bee7b2a4c0ba9b15c6424&framework=4&showOnlyImportant=0

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8900730 [details]
Bug 1392157 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes.

https://reviewboard.mozilla.org/r/172172/#review177428

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8900731 [details]
Bug 1392157 - Performance improvements to burst animation.

https://reviewboard.mozilla.org/r/172174/#review177424

::: browser/base/content/tabbrowser.xml:734
(Diff revision 1)
>  
>                    // Only animate the "burst" indicating the page has loaded if
>                    // the top-level page is the one that finished loading.
>                    if (aWebProgress.isTopLevel && !aWebProgress.isLoadingDocument &&
> +                      !this.mTabBrowser.tabAnimationsInProgress &&
> +                      Date.now() - this.mTab.timeWhenStartedLoading > 150 &&

I don't understand how this makes sense from a user's perspective. Assuming the burst animation is supposed to signal to the user that a page has finished loading, then it seems this should be done consistently even for fast-loading pages.

::: browser/base/content/tabbrowser.xml:7674
(Diff revision 1)
> -            // Stop the animation if it's already playing so we can restart it.
> +            return;
> -            this.removeAttribute("bursting");
> -            let burst = document.getAnonymousElementByAttribute(this, "anonid", "tab-loading-burst");
> -            window.getComputedStyle(burst).animationName;
>            }
>            this.setAttribute("bursting", "true");

At this point you can just remove this method and do tab.setAttribute("bursting") on the call site.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #14)
> Comment on attachment 8900731 [details]
> Bug 1392157 - Performance improvements to burst animation.
> 
> https://reviewboard.mozilla.org/r/172174/#review177424
> 
> ::: browser/base/content/tabbrowser.xml:734
> (Diff revision 1)
> >  
> >                    // Only animate the "burst" indicating the page has loaded if
> >                    // the top-level page is the one that finished loading.
> >                    if (aWebProgress.isTopLevel && !aWebProgress.isLoadingDocument &&
> > +                      !this.mTabBrowser.tabAnimationsInProgress &&
> > +                      Date.now() - this.mTab.timeWhenStartedLoading > 150 &&
> 
> I don't understand how this makes sense from a user's perspective. Assuming
> the burst animation is supposed to signal to the user that a page has
> finished loading, then it seems this should be done consistently even for
> fast-loading pages.

I removed this in #11 above, but removing this reintroduced the bloom_basic and damp regression. It's possible that we don't care about these regressions for bloom_basic and damp. bloom_basic loads many pages very fast in the same tab, too fast to actually see the tab contents changing. DAMP loads many tabs and loads the devtools in the tab, acting as a poor cousin of TART.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8900731 [details]
Bug 1392157 - Performance improvements to burst animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172174/diff/1-2/

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8900731 [details]
Bug 1392157 - Performance improvements to burst animation.

https://reviewboard.mozilla.org/r/172174/#review177474

::: browser/base/content/tabbrowser.xml:735
(Diff revision 2)
>                    // the top-level page is the one that finished loading.
>                    if (aWebProgress.isTopLevel && !aWebProgress.isLoadingDocument &&
> +                      !this.mTabBrowser.tabAnimationsInProgress &&
>                        Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {
> -                    this.mTab.animateLoadingBurst();
> +                    if (!this.mTab.hasAttribute("bursting")) {
> +                      this.mTab.setAttribute("bursting", "true");

nit: you can remove the hasAttribute check, it shouldn't make a difference

I'm not sure about the tabAnimationsInProgress check. If we're lucky, this is enough of an edge case that users won't run into it or just won't notice. If we're not lucky, this introduces inconsistent behavior.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8900731 [details]
Bug 1392157 - Performance improvements to burst animation.

https://reviewboard.mozilla.org/r/172174/#review177548

::: browser/base/content/tabbrowser.xml:735
(Diff revision 2)
>                    // the top-level page is the one that finished loading.
>                    if (aWebProgress.isTopLevel && !aWebProgress.isLoadingDocument &&
> +                      !this.mTabBrowser.tabAnimationsInProgress &&
>                        Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {
> -                    this.mTab.animateLoadingBurst();
> +                    if (!this.mTab.hasAttribute("bursting")) {
> +                      this.mTab.setAttribute("bursting", "true");

If you don't end up removing this for some reason, you should probably fold the hasAttribute check into the outer conditional. I suspect dao's right that it doesn't change much, but happy to hear otherwise.

::: browser/base/content/tabbrowser.xml
(Diff revision 2)
> -        <body><![CDATA[
> -          if (this.hasAttribute("bursting")) {
> -            // Stop the animation if it's already playing so we can restart it.
> -            this.removeAttribute("bursting");
> -            let burst = document.getAnonymousElementByAttribute(this, "anonid", "tab-loading-burst");
> -            window.getComputedStyle(burst).animationName;

Hooray!

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8900731 [details]
Bug 1392157 - Performance improvements to burst animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172174/diff/2-3/

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8900731 [details]
Bug 1392157 - Performance improvements to burst animation.

https://reviewboard.mozilla.org/r/172174/#review177474

> nit: you can remove the hasAttribute check, it shouldn't make a difference
> 
> I'm not sure about the tabAnimationsInProgress check. If we're lucky, this is enough of an edge case that users won't run into it or just won't notice. If we're not lucky, this introduces inconsistent behavior.

I'd like to keep the tabAnimationsInProgress for now. We know that removing it regresses TART significantly, and Eric is OK with not running it in this case. I think it's something that we could land now and if it does become a problem it will be easy to remove this later.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a8b63a9298d
Implement new page loading indicator animation, part 2: add a "burst" when loading finishes. r=jaws
https://hg.mozilla.org/integration/autoland/rev/ce5d0cb837b1
Performance improvements to burst animation. r=mconley

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a8b63a9298d
https://hg.mozilla.org/mozilla-central/rev/ce5d0cb837b1

Comment 23

[Stefan [:StefanG_QA]]

User Agent    Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build   ID    20170830100230

This bug fix is Verified with latest Nightly 57.0a1.