853844 - Investigate removing the pageload throbber

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

We have a love/hate relationship with the pageload throbber. We love to hate it.

The throbber:
1.  Can start and stop more than once during a page load
2.  Affects CPU load and animation quality. Past tests have shown this affect pageload speed too.
3.  Can keep spinning a long time ofter the page seems to be loaded. This can happen if the page has very large images or social media buttons.

All of this issues led us to talk about removing or radically changing the throbber.

I want to start a few experiments in this little project:
1. Just removing the throbber and see how the pageloads feel. We should be able to load the favicon much sooner in this case too.
2. Use the content background as a billboard, where we show a "Loading" graphic watermark. As the page content loads, this watermark will be painted over, thus disappear.
3. Add a fallback indicator in cases where we take too long to actually paint anything on the screen.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 1: Remove the throbber

This patch just removes the spinner/throbber itself. We need to keep the setProgressVisibility method and keep calling it because it controls the "STOP" button too.

The patch also allows the favicon to update ASAP, and not delay it until the page loads.

Some observations:
* The favicon and title update very quickly, but the old page content remains making things a bit confusing. I wonder if we need to "white-out" the page content much earlier. This would be a nice time to add "loading" watermark.
* I find myself using the page much sooner than with a throbber. I must be conditioned to wait until the throbber stops.
* I kinda like the way this works so far. Pages are loading fast enough to keep me from wondering "WTF is going on?". This could be the pages I visit though. Again, the watermark would help for larger pages.

Test build:
http://people.mozilla.com/~mfinkle/fennec/fennec-22.0a1-nospinner.apk

Comment 2

[Lucas Rocha (:lucasr)]

Quick feedback: looks interesting. I agree that we'd need to provide some immediate response in terms of UI loading state. The "white-out" idea is worth a try but might look too jarring. Maybe showing the bg dominant color for the target page as soon as possible instead? Not sure this would be possible.

I think the ideal approach would probably be to keep the throbber (or some sort of obvious "loading state" UI element in the toolbar) but hide it as soon as the first "big chunk" of the page is displayed. Most of the time, it seems that our page loads happen like:

1. A big chunk of the page is displayed
2. Bits and pieces are gradually loaded (images in most cases)

I wonder if there's a way to recognize when we're shown a "big enough" initial chunk of the page and hide the throbber when that happens.

The advantage of the throbber over "whitening-out" the screen is that the throbber is visually more subtle.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #2)

> The advantage of the throbber over "whitening-out" the screen is that the
> throbber is visually more subtle.

I agree, but disagree :)

At some point we will be loading an entirely different webpage into the content area. So it will be "jarring" anyway.

Keeping the throbber until a first paint might be a good next step. It would fix #3 in comment 0 and maybe help a little with #2.

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> (In reply to Lucas Rocha (:lucasr) from comment #2)
> 
> > The advantage of the throbber over "whitening-out" the screen is that the
> > throbber is visually more subtle.
> 
> I agree, but disagree :)
> 
> At some point we will be loading an entirely different webpage into the
> content area. So it will be "jarring" anyway.

Well, I guess the main difference would be that if you white-out the screen before loading the next page there would be not one but two big visual switches (current page -> white -> next page)

Comment 5

[:Margaret Leibovic]

To chime in, I like the idea of doing something to clear the old page ASAP. Even if we don't have something new to show yet, hanging around on the old page can make the browser feel unresponsive.

I'd be interested to see what it feels like to have some sort of "we're working on it" indicator in the page content.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to :Margaret Leibovic from comment #5)
> To chime in, I like the idea of doing something to clear the old page ASAP.
> Even if we don't have something new to show yet, hanging around on the old
> page can make the browser feel unresponsive.

I prefer leaving the old page on-screen until we have the new page ready to draw. Showing the old page is more useful than showing a blank page; I often click on a link even though I'm not done with the previous page (e.g. still reading the last sentence) because I know it'll take a second or two before the new page is painted, and I can finish reading before the new page paints. If the old page gets blanked out then those couple of seconds are completely useless to me.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> (In reply to :Margaret Leibovic from comment #5)
> > To chime in, I like the idea of doing something to clear the old page ASAP.
> > Even if we don't have something new to show yet, hanging around on the old
> > page can make the browser feel unresponsive.
> 
> I prefer leaving the old page on-screen until we have the new page ready to
> draw. Showing the old page is more useful than showing a blank page; I often
> click on a link even though I'm not done with the previous page (e.g. still
> reading the last sentence) because I know it'll take a second or two before
> the new page is painted, and I can finish reading before the new page
> paints. If the old page gets blanked out then those couple of seconds are
> completely useless to me.

So because we are slow to load pages, you have formed a behavior of page-reading/loading :)

I don't like the old page hanging around because the page title (and favicon if we stop using the throbber) change very quickly, so I get the new page title with the old page content.

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 728202 [details] [diff] [review]
WIP 1: Remove the throbber

The patch itself seems fine, but I don't like the feel of it. When I click a link, the browser just sits completely frozen for many seconds, and it feels like something is broken. The effect is worst after clicking a link before anything is drawn. I'm not sure clearing the screen first would help much with this perception - I'd still be staring at a static screen for a long time without any perception of something happening.

Note that I'm testing this on the Mozilla Guest network where the effect will be more pronounced; clicking some links takes 20 seconds or more to change from the Google results page to the new page. Using my phone's 4G network is much better, though I'm still not sure I'd consider it an improvement over what we have in Nightly.

If we add a watermark, will it have some sort of animation? It doesn't matter where we do it, but I feel that we need something moving to prevent the browser from feeling stuck.

Comment 9

[Brian Nicholson (:bnicholson)]

For anyone who doesn't have a sufficiently slow connection, here's a video of what I'm seeing: http://dl.dropbox.com/u/3017599/Fennec/VID_20130325_121856.mp4

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Good feedback from everyone. We have been chatting about this in IRC too. Most people want some indication that the browser has started working on loading the page. I do too, as I allude to in comment 1.

One thing we have not talked much about is how we feel about the opposite end of the spectrum. Do we feel the throbber stays around too long? Do you ever wonder why the throbber is still spinning after the page appears to be fully loaded?

Comment 11

[Thomas Stache]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> One thing we have not talked much about is how we feel about the opposite
> end of the spectrum. Do we feel the throbber stays around too long? Do you
> ever wonder why the throbber is still spinning after the page appears to be
> fully loaded?

Inclined to say yes and yes. Although right now on Wifi I see this rarely. The worst example is the desktop version of techcrunch.com.

I think there might be some side issues tainting the impression of the page load process: 
1. the current throbber is janky, the rotation is just not smooth. Since Jelly Bean this is jarring.
2. hard to hit Stop button (often I just manage to enter the awesome screen),
3. Reader mode button is only enabled really late, so you have to wait for all ads, social and other 3rd party "widgets" to load

Comment 12

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Remove the blue throbber

This spins the dashed square (empty favicon) until page loads. All the blue ones are removed.
Note: This does the same <rotate> animation on a view's bitmap -- rather than during a frame by frame animation. Hence, the time to load a frame between animation is saved.
I don't know any tool to see if there is a difference in performance.
Also, the animation can be changed from a simple <rotate> to a pulsating one, or a heart-beat or a fade-in and fade-out.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 729247 [details] [diff] [review]
WIP: Remove the blue throbber

That was supposed to be a WIP. :(

Comment 16

[Ian Barlow (:ibarlow)]

Here's another possible approach we could take, using a progress bar with a more-or-less fixed timing.

Keynote animation prototype: http://cl.ly/1L1m403f0r3Y
Quicktime mov: http://cl.ly/0G0K2x450H0N

In this design, we create a sense of loading vs done by splitting the animation into two parts. In the first 25% of the screen, the bar moves more slowly such that it appears to be working on loading page elements. Once it crosses the 25% mark it quickly shoots the rest of the way across the screen, so as to suggest that loading is complete. 

Does anyone have time to get a prototype of this into a build? Would be great to see if it feels faster in real life, and if it is smoother than the current throbber we use.

Comment 17

[Ian Barlow (:ibarlow)]

BTW I believe Firefox on Windows 8 is using a similar approach at the moment.

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Proof of concept

This is a bit thrown together as a proof of concept for you ian. Build at:

http://people.mozilla.com/~wjohnston/progressbar.apk

We don't really get much for progress events. i.e. AFAICT, we don't know the size of a lot of resources, so as far as we know, progress is always at 100% which is kinda useless. I threw a minimum page size on (based on the AMO and SUMO pages we ship) so that you can see some movement. Maybe Metro's found a good solution?

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 747699 [details] [diff] [review]
Proof of concept

If we ever decide to send Gecko progress events, which I still veto, we'd use the algorithm we tested in XUL Fennec, which matched the C++ filter. We re-impled in JS due to e10s issues. 

Today though, we could even use Gavin's patch in bug 474464 to minimize message traffic.

Comment 20

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #18)
> Created attachment 747699 [details] [diff] [review]
> Proof of concept
> 
> This is a bit thrown together as a proof of concept for you ian. Build at:
> 
> http://people.mozilla.com/~wjohnston/progressbar.apk
> 
> We don't really get much for progress events. i.e. AFAICT, we don't know the
> size of a lot of resources, so as far as we know, progress is always at 100%
> which is kinda useless. I threw a minimum page size on (based on the AMO and
> SUMO pages we ship) so that you can see some movement. Maybe Metro's found a
> good solution?

Wow. It's not perfect yet but when it works, it feel delightfully snappy! A huge improvement over the spinning throbber, in my mind. 

I suppose a couple of things we should think about are:
* removing the spinning throbber in the toolbar -- this bar is meant to replace that. 
* trying to keep the bar from ever stopping dead halfway through and feeling stuck. It should always be moving, even if it needs to slow to a crawl for a while. 
* as you mentioned, some way of knowing either how much is left in a page, or some way of knowing if enough visible information has loaded to be useful. 


Let's keep at it. I can make some graphics for this too.

Comment 21

[Wesley Johnston (:wesj)]

Shilpan's interested in this. I'll get him in contact with wlach so we can start doing some good performance comparisons with test builds.

Comment 22

[Shilpan Bhagat]

Based on some testing I found that using an AnimationDrawable takes an approximately 1.5ms to draw each frame. However, a progressbar takes about 0.5ms on every setProgess() method call.

Assuming that we still have 10 fps speed for the spinner, it might be advantageous to use the spinner instead of using a progressbar on very light pages. But otherwise, it's clear that moving from the spinner to progressbar will be faster and less jittery.

Comment 23

[Shilpan Bhagat]

Here is a comparision between the throbber at 10 fps and progressbar loading nytimes

Throbber: https://dl.dropboxusercontent.com/u/11916346/mozilla/throbber%20nytimes.png

Progressbar: https://dl.dropboxusercontent.com/u/11916346/mozilla/progressbar%20nytimes.png

Progress bar is currently the patch Wes put up here. A polished version coming up soon.

Comment 24

[Chris Lord [:cwiiis]]

(In reply to Shilpan Bhagat from comment #23)
> Here is a comparision between the throbber at 10 fps and progressbar loading
> nytimes
> 
> Throbber:
> https://dl.dropboxusercontent.com/u/11916346/mozilla/throbber%20nytimes.png
> 
> Progressbar:
> https://dl.dropboxusercontent.com/u/11916346/mozilla/progressbar%20nytimes.
> png
> 
> Progress bar is currently the patch Wes put up here. A polished version
> coming up soon.

We could always throttle the progress bar to 10fps too if that's an issue (though it'd look much nicer if we didn't of course :))

Comment 25

[Shilpan Bhagat]

Attachment: [WIP] Optmized pageload throbber with smoother animation + less cpu intensive

This patch removes AnimationDrawable (i.e. frame by frame animation). Method profiling suggests that it takes half the cpu time it previously did plus makes the animation smoother. 

I know not everyone is happy about having a progress bar :) Hence, this could be a good thing to do in case we plan to stick with the throbber.

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 783611 [details] [diff] [review]
[WIP] Optmized  pageload throbber with smoother animation + less cpu intensive

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>+import android.view.animation.AnimationUtils;

Supported since API 1 !

>+    private boolean mSpinnerVisible;

Boolean state flags are my arch nemesis. We must find a way to destroy this.

>-        mProgressSpinner = (AnimationDrawable) res.getDrawable(R.drawable.progress_spinner);
>+        mProgressSpinner = AnimationUtils.loadAnimation(mActivity, R.anim.progress_spinner);

I assume loadAnimation is not expensive

>     public void setProgressVisibility(boolean visible) {

>+            mFavicon.setImageResource(R.drawable.progress_spinner);
>+            //To stop the glitch caused by mutiple start() calls.
>+            if (!mSpinnerVisible) {

Can't we use mProgressSpinner.hasStarted() ?

>+            if (mSpinnerVisible) {

Same

>     private void setFavicon(Bitmap image) {
>-        if (Tabs.getInstance().getSelectedTab().getState() == Tab.STATE_LOADING)
>-            return;

What's this change about?

>diff --git a/mobile/android/base/resources/layout/browser_toolbar.xml b/mobile/android/base/resources/layout/browser_toolbar.xml

>         <ImageButton android:id="@+id/favicon"

>-                     android:paddingLeft="12dip"
>-                     android:layout_marginRight="4dip"
>+                     android:layout_marginLeft="8dip"
>+                     android:paddingRight="4dip"
>+                     android:paddingLeft="4dip"

Tell me more about this change. Is this because the image is smaller?

>diff --git a/mobile/android/base/resources/values/dimens.xml b/mobile/android/base/resources/values/dimens.xml

>-    <dimen name="browser_toolbar_favicon_size">29.33dip</dimen>
>+    <dimen name="browser_toolbar_favicon_size">25.33dip</dimen>

Tell me more. Looks like the size is getting smaller. Won't this stretch the spinner out of shape? Or is the new image small enough to be unaffected?

f+ from me, but sending to Sriram next. If this is the same visual experience, has some positive perf improvement and has less images, then I am OK with landing it.

Comment 27

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 783611 [details] [diff] [review]
[WIP] Optmized  pageload throbber with smoother animation + less cpu intensive

Review of attachment 783611 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good. Needs couple of changes and new icons.

::: mobile/android/base/BrowserToolbar.java
@@ +103,5 @@
>      private boolean mHasSoftMenuButton;
>  
>      private boolean mShowSiteSecurity;
>      private boolean mShowReader;
> +    private boolean mSpinnerVisible;

This might not be needed.

@@ +788,5 @@
>          // The "Throbber start" and "Throbber stop" log messages in this method
>          // are needed by S1/S2 tests (http://mrcote.info/phonedash/#).
>          // See discussion in Bug 804457. Bug 805124 tracks paring these down.
>          if (visible) {
> +            mFavicon.setImageResource(R.drawable.progress_spinner);

New line after this.

@@ +790,5 @@
>          // See discussion in Bug 804457. Bug 805124 tracks paring these down.
>          if (visible) {
> +            mFavicon.setImageResource(R.drawable.progress_spinner);
> +            //To stop the glitch caused by mutiple start() calls.
> +            if (!mSpinnerVisible) {

mProgressSpinner.hasStarted() would do.

@@ +802,3 @@
>              Tab selectedTab = Tabs.getInstance().getSelectedTab();
>              if (selectedTab != null)
>                  setFavicon(selectedTab.getFavicon());

Here is some problem. When you switch the image from a spinner to the actual favicon, it's going to rotate for a flip second -- which is fine. But, the animation could stop at any rotation other than 0deg. In this case, the favicon would jump back from Xdeg to 0deg. Is there something we could do about it? May be post a runnable to complete the rotation and reset at 0deg? I remember writing similar patch and facing such an issue.

@@ +802,5 @@
>              Tab selectedTab = Tabs.getInstance().getSelectedTab();
>              if (selectedTab != null)
>                  setFavicon(selectedTab.getFavicon());
> +
> +            if (mSpinnerVisible) {

mProgressSpinner.hasStarted();

@@ +804,5 @@
>                  setFavicon(selectedTab.getFavicon());
> +
> +            if (mSpinnerVisible) {
> +                setPageActionVisibility(false);
> +                mFavicon.setAnimation(null);

I believe the favicon is reset to original position because, we don't do a fillAfter. Please confirm me if that's the case.

@@ +963,5 @@
>  
>          setTitle(title);
>      }
>  
>      private void setFavicon(Bitmap image) {

This is going to have some impact. If I remember correctly, my patch rotates an empty squared favicon until there is a favicon, and then rotates the favicon once set. We might not want to do that here. The favicon should be set once the page loads. So we might need this statement.

::: mobile/android/base/Makefile.in
@@ +645,5 @@
>    res/drawable-mdpi/ic_menu_new_tab.png \
>    res/drawable-mdpi/ic_menu_reload.png \
>    res/drawable-mdpi/ic_status_logo.png \
>    res/drawable-mdpi/icon_pageaction.png \
> +  res/drawable-mdpi/progress_spinner.png \

This is scaring me. Why didn't I see that we don't use high res progress spinners all these days?? That's explains the pixellation issue :( May be you should ask for 3 versions (and only 3) :P. I vaguely remember that we had 7 different browser-toolbar sizes, and that required many-many progress spinners -- and hence dropped the idea. Now we can use one single size on 48dp and 56dp. So just 3 images.

::: mobile/android/base/resources/layout/browser_toolbar.xml
@@ +102,5 @@
>                       android:layout_height="fill_parent"
>                       android:scaleType="fitCenter"
> +                     android:layout_marginLeft="8dip"
> +                     android:paddingRight="4dip"
> +                     android:paddingLeft="4dip"

Why is this change needed? Also, there is one more "browser_toolbar.xml" :P

::: mobile/android/base/resources/values/dimens.xml
@@ +29,5 @@
>      <dimen name="browser_toolbar_height">48dp</dimen>
>      <dimen name="browser_toolbar_button_padding">12dp</dimen>
>      <dimen name="browser_toolbar_icon_width">48dp</dimen>
>      <dimen name="browser_toolbar_lock_width">20dp</dimen>
> +    <dimen name="browser_toolbar_favicon_size">25.33dip</dimen>

Why is this changed? I somehow don't like this scaling. Also, this is going to affect all favicons for the page. The first time I did this, we had some weird calcuations like a 32px showing up on a mdpi should have 21.33dp or so. 25.33 sounds scary.

Comment 28

[Sriram Ramasubramanian [:sriram]]

I couldn't change the review flag to feedback. It's an f+ from my side.

Comment 29

[Sriram Ramasubramanian [:sriram]]

If you face issues just like what I said -- favicon jumping back from a weird rotation, here is a different alternative.

It's better to have a FrameLayout that has an ImageView (favicon) and another ImageView (spinner). Show/hide them based on what needs to be shown. FrameLayout is good in that it's effective in showing only one of its children. So, only either of them would be shown. Also, the spinner can be hidden (and reset) while the favicon change (without any rotation).

Comment 30

[Shilpan Bhagat]

Sriram, I have addressed the issue you mentioned above in the current patch. You won't  You won't find that weird glitch anymore. I will put up another patch addressing the rest of yours and Mark's comments soon.

Comment 31

[Shilpan Bhagat]

Sriram, I have addressed the issue you mentioned above in the current patch. You won't  You won't find that weird glitch anymore. I will put up another patch addressing the rest of yours and Mark's comments soon.

Comment 32

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 783611 [details] [diff] [review]
[WIP] Optmized  pageload throbber with smoother animation + less cpu intensive

Review of attachment 783611 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed things seem fine with me. This patch feels good to land.
Please add the hdpi and xhdpi resources in a separate file.

::: mobile/android/base/BrowserToolbar.java
@@ +795,5 @@
> +                setPageActionVisibility(true);
> +                mFavicon.setAnimation(mProgressSpinner);
> +                mProgressSpinner.start();
> +            }
> +            mSpinnerVisible = true;

Move this inside the if().

@@ +807,5 @@
> +                setPageActionVisibility(false);
> +                mFavicon.setAnimation(null);
> +                mProgressSpinner.cancel();
> +            }
> +            mSpinnerVisible = false;

Move this inside the if().

Comment 33

[Shilpan Bhagat]

Attachment: Patch: Optimized throbber

Carrying r+ forward

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

What about the review comments about using mProgressSpinner.hasStarted() instead of the evil mSpinnerVisible ?

Comment 35

[Shilpan Bhagat]

hasStarted() returns true for every single call once the animation is played for the first time. The flag seems to be the only way to skip over the unnecessary overhead of initializing the animation every time before playing it. :(

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/75473a07af63

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/75473a07af63

Comment 38

[Aaron Train [:aaronmt]]

Verified smoooth frames
http://31.media.tumblr.com/tumblr_m5pho1y7at1rwcc6bo1_400.gif