Closed Bug 1380150 Opened 2 years ago Closed 2 years ago

Flickering between favicon and loading indicator after first load

Categories

(Firefox :: Tabbed Browser, defect, P1)

56 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- verified

People

(Reporter: epang, Assigned: jaws)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [reserve-photon-animation] )

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [photon-animation] → [photon-animation][triage]
Priority: P1 → --
Flags: qe-verify+
Priority: -- → P3
QA Contact: jwilliams
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]
Whiteboard: [reserve-photon-animation] → [photon-visual][triage]
Priority: P3 → --
QA Contact: jwilliams → brindusa.tot
Flags: qe-verify+
QA Contact: brindusa.tot
Whiteboard: [photon-visual][triage]
Priority: -- → P3
Whiteboard: [reserve-photon-animation]
Flags: qe-verify?
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Priority: P3 → P1
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?
Attachment #8904746 - Flags: review?(gijskruitbosch+bugs)
Attachment #8904746 - Attachment is obsolete: true
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... :-)
Attachment #8906072 - Flags: review?(gijskruitbosch+bugs) → review+
Yes, tested on the URL linked to by this bug's URL field.
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/440b703da884
Prevent subresource loads from showing the progress indicator on the tab. r=Gijs
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
Flags: needinfo?(jaws)
Flags: needinfo?(jaws)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c9fe4ec27ca3
Prevent subresource loads from showing the progress indicator on the tab. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/c9fe4ec27ca3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Duplicate of this bug: 1398442
Depends on: 1399101
Flags: qe-verify? → qe-verify+
QA Contact: stefan.georgiev
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.