938153 - Selectively display the globe favicon placeholder when favicon fetches take long enough to be noticeable

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Ian Barlow (:ibarlow)]

It looks like some of the work that happened in bug 923218 had an effect on more than just the home page lists. The *URL bar* favicon behaviour seems to have changed too. 

We are now often seeing a blank void while we wait for a favicon to load, instead of the previous (and more desirable) behaviour where we placed a globe while waiting for the favicon. 

Let's use this bug to restore the original behaviour to the URL bar.

Comment 1

[Richard Newman [:rnewman]]

Yeah, that was deliberate, but we probably overshot. (We did try to get this right, though: Bug 923218 Comment 6! :P)

The previous behavior was:

* Show the spinner.
* Hide the spinner, replace it with a blank spot.
* Show the globe.
* A few milliseconds later, show the real favicon.

That looked really janky and busy when the favicon was ready for display, and still kinda janky when the favicon took less than two seconds to load.

We fixed this in Bug 923218: instead of showing the globe *then* showing the favicon, we only show the globe if the favicon fetch failed. Typically that's quick, but there's still a gap between the spinner and the icon, so if the favicon takes a while to load it looks weird in a different way.

mfinkle and I were chatting about this today. The ideal behavior would be to do something like this:

* Overlay the spinner on the globe.
* Fade out the spinner as the page load completes, revealing the globe.
* Fade in the real favicon when it arrives (which ideally will be immediately as the spinner disappears).

Sriram probably has some input on this from a drawing perspective…

Comment 2

[Richard Newman [:rnewman]]

Aaah, I see what happened in that bug.

I filed it about the URL bar ("URL bar" in Bug 923218 Comment 0), and you thought I was talking about the home page lists ("slots" in Bug 923218 Comment 3). Sorry, I should have been clearer about which bit of the UI we were tweaking :D

On the plus side, now we have the opportunity to make it even better!

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #1)
> mfinkle and I were chatting about this today. The ideal behavior would be to
> do something like this:
> 
> * Overlay the spinner on the globe.
> * Fade out the spinner as the page load completes, revealing the globe.
> * Fade in the real favicon when it arrives (which ideally will be
> immediately as the spinner disappears).

IMO, the gist of this bug is that there should never be a void in the toolbar. The animation part is an extra, I'd discuss/fix it in a separate bug.

Comment 4

[Richard Newman [:rnewman]]

Per discussion with Ian on IRC: the short-term fix for this bug would be to undo Bug 923218. That sucks, too, so we're not going to do it.

Instead, I'm going to file a bug to preload about:home's favicon, so it loads instantly on first run (no silly void before the Fennec/Firefox icon loads). That's a big part of the ugliness, and what we do now is stupid, so let's just fix that.

(If we do the special-page fetching logic right, then this'll also buy us other favicon speedups.)

That'll leave the only bad experience being a void when loading a site's favicon for the first time (from network or disk).

I plan to leave this bug open to address that; the solution will probably be to have an additional favicon cache callback method that tells the caller whether they're going to have to wait for a disk/network fetch; in that method we can show the globe, so the void will only exist for an instant (long enough to check the cache), and then we'll either show globe then icon, or just wait another instant for the real icon to show up.

Re-flagging as tracking 'cos I don't know if we'd want to take that non-trivial work for near-beta or early-beta.

Speak now or forever hold your peace.

Comment 5

[Richard Newman [:rnewman]]

Filed as Bug 941868.

Comment 6

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

Last triage meeting had a lot of folks suggesting the old "show globe, then show favicon" was better than the current behavior. Let's get a better plan.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Last triage meeting had a lot of folks suggesting the old "show globe, then
> show favicon" was better than the current behavior. Let's get a better plan.

You want the easy path (flickering globe in most cases), or the more difficult path (show the globe for non-cached icons)?

Comment 8

[Richard Newman [:rnewman]]

Re-ping? I'm inclined to take the slightly more difficult route here, just because the old behavior was ugly enough that we already tried to fix it once.

Comment 9

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

Sure. Let's start exploring the more difficult route. If you could see my face, you would know the extent of my displeasure in adding more complexity for favicons.

There has got to be some simple, elegant way forward here.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Selectively display the globe favicon placeholder when favicon fetches take long enough to be noticeable.

Howzat? Shows the globe unless the favicon is cached, in which case we don't show the globe, and the favicon appears immediately. Looks good, though on my device the favicon loads are so quick compared to page loads that you only see the globe for 100ms anyway.

Comment 11

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

Comment on attachment 8349193 [details] [diff] [review]
Selectively display the globe favicon placeholder when favicon fetches take long enough to be noticeable.

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

>+    public void showDefaultFavicon() {
>+        mFavicon.setImageResource(R.drawable.favicon);
>+    }

Should we reset mLastFavicon in here? We use mLastFavicon for some optimizations in setFavicon.

Comment 12

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/7ef525f0c865

If this sticks, will request uplift.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ef525f0c865

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8349193 [details] [diff] [review]
Selectively display the globe favicon placeholder when favicon fetches take long enough to be noticeable.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Multiple aeons of favicon improvements.

User impact if declined: 
  Users will see a blank spot in the URL bar until a network favicon is loaded. This patch reintroduces the grey globe icon unless a page's favicon is already cached.

Testing completed (on m-c, etc.): 
  Been baking for a while with no observed problems to my knowledge.

Risk to taking this patch (and alternatives if risky): 
  One of the least risky favicon changes in recent months :P

String or IDL/UUID changes made by this patch:
  None.

Comment 16

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/54b3b3fa8bbe
https://hg.mozilla.org/releases/mozilla-beta/rev/12e33df9b206

Comment 17

[Flaviu Cos]

Verified as fixed in builds:
- FF 27;
- 28 Beta 4;
- Aurora 29.0a2 (2014-02-23);
Device: Asus Transformer Tab (Android 4.0.3).