1380150 - Flickering between favicon and loading indicator after first load

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Eric Pang [:epang] UX]

Once a site loads the favicon appears, but sometimes more content is loaded causing the loading motion to start again repeatably  (flickering between the favicon and loading indicator).  This causes the experience to feel like a glitch. 

Note, this currently occurs in Firefox and is not a regression - but not a good experience in the current and new designs.  

I expect that once the load is complete the first time the motion shouldn't play again.  Another option is to have a threshold of when the motion can be played again.

Comment 1

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

Attachment: Bug 1380150 - Prevent subsequent loads on a page after the initial page load has completed from showing the progress indicator on the tab. This will prevent the favicon from flickering.

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

Comment 2

[:Gijs (he/him)]

Comment on attachment 8904746 [details]
Bug 1380150 - Prevent subsequent loads on a page after the initial page load has completed from showing the progress indicator on the tab. This will prevent the favicon from flickering.

https://reviewboard.mozilla.org/r/176532/#review181516

This affects whether other progress listeners will get called for these loads, not just the icon, because now you're early-returning from `_shouldShowProgress`. Affecting other listeners here doesn't seem quite right.

I'm also confused about the approach. Wouldn't it be enough to just not care about subresource loads here? Maybe I misunderstand how this is supposed to work...

::: browser/base/content/tabbrowser.xml:758
(Diff revision 1)
>                    }
>                    delete this.mBrowser.initialPageLoadedFromURLBar;
>                    // If the browser is loading it must not be crashed anymore
>                    this.mTab.removeAttribute("crashed");
> +
> +                  let listener = this.mTabBrowser._tabListeners.get(this.mTab);

Leftover?

Comment 3

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

Attachment: Bug 1380150 - Prevent subresource loads from showing the progress indicator on the tab.

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

Comment 4

[:Gijs (he/him)]

Comment on attachment 8906072 [details]
Bug 1380150 - Prevent subresource loads from showing the progress indicator on the tab.

https://reviewboard.mozilla.org/r/177810/#review182984

Very straightforward, r=me on the code. I assume you've tested this... :-)

Comment 5

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

Yes, tested on the URL linked to by this bug's URL field.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/440b703da884
Prevent subresource loads from showing the progress indicator on the tab. r=Gijs

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing many mochitests with exceptions:

https://hg.mozilla.org/integration/autoland/rev/ecd75d0e0a2f5215aa4c3a08502e4434192b4ab3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=440b703da884ff5eaa805314982c2003febf5349&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=129905947&repo=autoland

08:31:09     INFO - TEST-START | accessible/tests/browser/browser_shutdown_multi_proxy_acc_reference_doc.js
08:31:10     INFO - TEST-INFO | started process screencapture
08:31:10     INFO - TEST-INFO | screencapture: exit 0
08:31:10     INFO - Buffered messages logged at 08:31:09
08:31:10     INFO - Entering test bound 
08:31:10     INFO - TEST-PASS | accessible/tests/browser/browser_shutdown_multi_proxy_acc_reference_doc.js | Service initialized - 
08:31:10     INFO - TEST-PASS | accessible/tests/browser/browser_shutdown_multi_proxy_acc_reference_doc.js | Service initialized correctly - 
08:31:10     INFO - Buffered messages finished
08:31:10     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/browser_shutdown_multi_proxy_acc_reference_doc.js | uncaught exception - TypeError: aWebProgress is null at onProgressChange@chrome://browser/content/tabbrowser.xml:673:1
08:31:10     INFO - _callProgressListeners@resource://gre/modules/RemoteWebProgress.jsm:179:11
08:31:10     INFO - receiveMessage@resource://gre/modules/RemoteWebProgress.jsm:296:7
08:31:10     INFO - 
08:31:10     INFO - Stack trace:
08:31:10     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
08:31:10     INFO - resource://gre/modules/RemoteWebProgress.jsm:_callProgressListeners:179
08:31:10     INFO - resource://gre/modules/RemoteWebProgress.jsm:receiveMessage:296
08:31:10     INFO - GECKO(1722) | JavaScript error: chrome://browser/content/tabbrowser.xml, line 673: TypeError: aWebProgress is null
08:31:10     INFO - Console message: [JavaScript Error: "TypeError: aWebProgress is null" {file: "chrome://browser/content/tabbrowser.xml" line: 673}]
08:31:10     INFO - onProgressChange@chrome://browser/content/tabbrowser.xml:673:1
08:31:10     INFO - _callProgressListeners@resource://gre/modules/RemoteWebProgress.jsm:179:11
08:31:10     INFO - receiveMessage@resource://gre/modules/RemoteWebProgress.jsm:296:7
08:31:10     INFO - 
08:31:10     INFO - Not taking screenshot here: see the one that was previously logged

Comment 8

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

Comment on attachment 8906072 [details]
Bug 1380150 - Prevent subresource loads from showing the progress indicator on the tab.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9fe4ec27ca3
Prevent subresource loads from showing the progress indicator on the tab. r=Gijs

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c9fe4ec27ca3

Comment 12

[Stefan [:StefanG_QA]]

I was able to reproduce this behavior on Nightly build 57.0a1 from 20170910220126.

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 (20170911100210)

I have verified this issue as fixed on Nightly build 57.0a1 from 20170911100210.