Some top sites have incorrect favicons
Categories
(Firefox :: Address Bar, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox76 | --- | verified |
People
(Reporter: billdillensrevenge, Assigned: bugzilla)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
megabar ts wrong favicon.png
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:75.0) Gecko/20100101 Firefox/75.0
Steps to reproduce:
the top sites list version of the megabar (sorry for the wording, I don't know how else to describe it) shows the incorrect favicon at least in some cases, such as twitter. See attached screenshot (I put red arrow). But when you actually type in the URL bar, the twitter entry has the correct favicon. (screenshot below)
|
||
Comment 1•5 years ago
|
||
Bugbug thinks this bug should belong to this component, but please revert this change in case of error.
|
||
Comment 3•5 years ago
|
||
This is interesting. If you open the network devtools and load https://twitter.com/firefox, there are two favicons that get loaded by FaviconLoader.jsm. In order:
- https://abs.twimg.com/responsive-web/web/icon-ios.8ea219d4.png (white bird, blue background)
- https://abs.twimg.com/favicons/twitter.ico (blue bird, transparent background)
Presumably twitter.ico ends up being the favicon stored in Places because it's the icon shown when you type "twitter" in the urlbar.
But icon-ios.8ea219d4.png ends up being stored by activity stream, and so it's the value of link.favicon here in the top-sites provider: https://searchfox.org/mozilla-central/rev/c1e3d3edd4a9b784971555dc74a5de23d768b2e1/browser/components/urlbar/UrlbarProviderTopSites.jsm#118 (link.tippyTopIcon is null/undefined in this case I'm guessing because the URL is twitter.com/firefox and not simply twitter.com.)
So at first glance it seems like activity stream is storing the wrong favicon. Ed, do you know anything about this?
I'll make this a P3 because it doesn't seem like it should block release.
It shouldn't block release but it should be fixed because it's important to have consistency for recognition. The icons should be the same.
Also, is this for all favicons or just some like twitter?
Also, is this another different bug in Firefox? Go to https://abs.twimg.com/favicons/twitter.ico in both Chrome and Firefox, here's what I'm seeing (Firefox on the left, Chrome on the right). It's supposed to be transparent isn't it? Not a white square?
|
|
||
Comment 6•5 years ago
|
||
Yes, every bug in Bugzilla should be fixed. I don't know whether other sites are affected. In your original comment you said "at least in some cases." Did you see others? Re: comment 5, it may not be a bug per se but just a different choice in how Firefox displays images when you navigate to them, but feel free to file a bug in Firefox::General.
|
||
Comment 7•5 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #3)
- https://abs.twimg.com/responsive-web/web/icon-ios.8ea219d4.png (white bird, blue background)
- https://abs.twimg.com/favicons/twitter.ico (blue bird, transparent background)
Off-hand the former is a touch icon, and it should be used when showing a large tile (like the new tab page large tiles), while the latter is a normal favicon. Places stores both, but for icons (tabs/bookmarks) it won't use the touch icons because they would be downscaled too much. The Address bar clearly prefers small icons.
New Tab at this time is probably the only consumer that uses touch icons, because it can generate tiles. Though it should not use touch icons for small favicon needs because of the downscaling problem.
It's possible to get the right icon from Places specifying the expected icon size, though that means the TopSites list should have both a tileicon and a favicon properties, or one should be able to request a specific size of icon as argument.
|
||
Comment 8•5 years ago
|
||
(In reply to Will from comment #5)
Created attachment 9128295 [details]
fficonbug.pngAlso, is this another different bug in Firefox? Go to https://abs.twimg.com/favicons/twitter.ico in both Chrome and Firefox, here's what I'm seeing (Firefox on the left, Chrome on the right). It's supposed to be transparent isn't it? Not a white square?
This is intentional and goes back to bug 754133 and bug 793366.
|
||
Comment 9•5 years ago
|
||
Top Sites purposefully uses rich icons as the new tab version shows a 96x96 icon. The largest favicon is requested here:
Bug 1606928 recently made the default top sites have the smaller icon, but for this case of actual history, maybe it's worthwhile to separately request the smaller icon from favicon service?
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 10•5 years ago
|
||
|
||
Updated•5 years ago
|
|
||
Comment 11•5 years ago
|
||
|
|
||
Comment 12•5 years ago
|
||
|
||
Comment 13•5 years ago
|
||
Backed out 2 changesets (Bug 1617333) for xpcshell failures at test_NewTabUtils.js.
https://hg.mozilla.org/integration/autoland/rev/f58f490db16763834d282651e5848e3f67a0df1e
Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=292321719&repo=autoland&lineNumber=2343
Also, please be aware of the ESlint error:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=292312368&repo=autoland&lineNumber=285
|
|
Assignee | |
Comment 14•5 years ago
|
||
There's a new revision up on Phabricator. Try run in progress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0429a1535f41b91c5b0ce7e8e37185124f7fd40d. I've since fixed the lint issue in that run.
|
|
||
Comment 15•5 years ago
|
||
|
||
Comment 16•5 years ago
|
||
Backed out changeset 52947d2e8692 (Bug 1617333) for causing xpcshell failures at gre/modules/NewTabUtils.jsm
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=292536525&repo=autoland&lineNumber=5941
[task 2020-03-10T20:09:45.543Z] 20:09:45 INFO - TEST-START | toolkit/modules/tests/xpcshell/test_timer.js
[task 2020-03-10T20:09:46.264Z] 20:09:46 WARNING - TEST-UNEXPECTED-FAIL | toolkit/modules/tests/xpcshell/test_timer.js | xpcshell return code: 0
[task 2020-03-10T20:09:46.265Z] 20:09:46 INFO - TEST-INFO took 721ms
[task 2020-03-10T20:09:46.265Z] 20:09:46 INFO - >>>>>>>
[task 2020-03-10T20:09:46.265Z] 20:09:46 INFO - toolkit/modules/tests/xpcshell/test_timer.js | xpcw: cd /sdcard/tests/xpc/toolkit/modules/tests/xpcshell
[task 2020-03-10T20:09:46.265Z] 20:09:46 INFO - toolkit/modules/tests/xpcshell/test_timer.js | xpcw: xpcshell -r /sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/geckoview-androidTest.apk -m -s -e const _HEAD_JS_PATH = "/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/sdcard/tests/xpc/p/mozinfo.json"; -e const _PREFS_FILE = "/sdcard/tests/xpc/user.js"; -e const _TESTING_MODULES_DIR = "/sdcard/tests/xpc/m"; -f /sdcard/tests/xpc/head.js -e const _HEAD_FILES = ["/sdcard/tests/xpc/toolkit/modules/tests/xpcshell/head.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_timer.js"]; -e const _TEST_NAME = "toolkit/modules/tests/xpcshell/test_timer.js"; -e _execute_test(); quit(0);
[task 2020-03-10T20:09:46.266Z] 20:09:46 INFO - toolkit/modules/tests/xpcshell/test_timer.js | [8684, Unnamed thread 7b30fcb21020] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/checkouts/gecko/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2020-03-10T20:09:46.266Z] 20:09:46 INFO - toolkit/modules/tests/xpcshell/test_timer.js | [8684, Unnamed thread 7b30fcb21020] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/checkouts/gecko/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2020-03-10T20:09:46.266Z] 20:09:46 INFO - toolkit/modules/tests/xpcshell/test_timer.js | [8684, Unnamed thread 7b30fcb21020] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/checkouts/gecko/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2020-03-10T20:09:46.270Z] 20:09:46 INFO - toolkit/modules/tests/xpcshell/test_timer.js | [8684, Unnamed thread 7b30fcb21020] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/checkouts/gecko/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2020-03-10T20:09:46.270Z] 20:09:46 INFO - toolkit/modules/tests/xpcshell/test_timer.js | [8684, Main Thread] WARNING: No Android crash handler set: file /builds/worker/checkouts/gecko/toolkit/crashreporter/nsExceptionHandler.cpp, line 1899
[task 2020-03-10T20:09:46.270Z] 20:09:46 INFO - toolkit/modules/tests/xpcshell/test_timer.js | [8684, Main Thread] WARNING: GetDnsSuffixList is not supported without a bridge connection: file /builds/worker/checkouts/gecko/netwerk/system/android/nsAndroidNetworkLinkService.cpp, line 138
[task 2020-03-10T20:09:46.270Z] 20:09:46 INFO - toolkit/modules/tests/xpcshell/test_timer.js | [8684, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/checkouts/gecko/toolkit/crashreporter/nsExceptionHandler.cpp, line 2930
[task 2020-03-10T20:09:46.270Z] 20:09:46 INFO - toolkit/modules/tests/xpcshell/test_timer.js | [8684, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /builds/worker/checkouts/gecko/startupcache/StartupCacheUtils.cpp, line 155
[task 2020-03-10T20:09:46.270Z] 20:09:46 INFO - NS_ERROR_FILE_NOT_FOUND:
[task 2020-03-10T20:09:46.270Z] 20:09:46 INFO - @resource://gre/modules/NewTabUtils.jsm:59:46
[task 2020-03-10T20:09:46.270Z] 20:09:46 INFO - @/sdcard/tests/xpc/toolkit/modules/tests/xpcshell/head.js:1:35
[task 2020-03-10T20:09:46.270Z] 20:09:46 INFO - load_file@/sdcard/tests/xpc/head.js:684:11
[task 2020-03-10T20:09:46.270Z] 20:09:46 INFO - _load_files@/sdcard/tests/xpc/head.js:696:10
[task 2020-03-10T20:09:46.270Z] 20:09:46 INFO - _execute_test@/sdcard/tests/xpc/head.js:543:14
[task 2020-03-10T20:09:46.271Z] 20:09:46 INFO - @-e:1:1
[task 2020-03-10T20:09:46.271Z] 20:09:46 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2020-03-10T20:09:46.271Z] 20:09:46 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2020-03-10T20:09:46.271Z] 20:09:46 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2020-03-10T20:09:46.271Z] 20:09:46 INFO - running event loop
[task 2020-03-10T20:09:46.271Z] 20:09:46 INFO - toolkit/modules/tests/xpcshell/test_timer.js | Starting test_setTimeout
[task 2020-03-10T20:09:46.271Z] 20:09:46 INFO - (xpcshell/head.js) | test test_setTimeout pending (2)
|
|
||
Comment 17•5 years ago
|
||
|
|
Assignee | |
Comment 18•5 years ago
|
||
There's a new revision up. Try running including the failed Android tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db832b814c04c0b3dfc69f5d81fc9fe22e001373
|
|
||
Comment 19•5 years ago
|
||
| bugherder | ||
|
Reporter | |
Comment 20•5 years ago
|
||
Can this be brought up to 75 beta?
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 21•5 years ago
|
||
Comment on attachment 9129598 [details]
Bug 1617333 - Show appropriately-sized favicons in the Urlbar for Top Sites with history visits. r?mak,Mardak!
Beta/Release Uplift Approval Request
- User impact if declined: We'll use the wrong icons for Top Sites in the new address bar experience.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: Yes
- If yes, steps to reproduce: 1. Open Firefox with a fresh profile. Observe the state of Top Sites on the New Tab page.
- Open Top Sites in the address bar by clicking the address bar. Observe that the favicons for the default sites are different from those shown on the New Tab: they are smaller and match the icons usually used for sites in address bar results, instead of the larger square icons we use for Top Sites on the New Tab.
- Visit kottke.org and bookmark it.
- Open a new tab.
- Observe that kottke.org shows up in Top Sites and that the "K" in its icon is hollow.
- Click on the address bar. Observe that kottke.org shows up in the Top Sites results and that the "K" in the icon there is filled in.
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Minor UI change.
- String changes made/needed:
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
Reporter | |
Comment 22•5 years ago
|
||
weird little issue I just noticed in 75.0b4: on top is the top sites megabar, on bottom is when I actually type "w" into the URL bar. Notice the Twitter icon is smaller in the top sites version. (the entries are identical, I cut off everything except "Ryan" for privacy reasons). And it's not because it's bookmarked, I deleted the bookmark and the top sites list version of the twitter icon remains small. Can anyone else repro?
|
||
Comment 23•5 years ago
|
||
Comment on attachment 9129598 [details]
Bug 1617333 - Show appropriately-sized favicons in the Urlbar for Top Sites with history visits. r?mak,Mardak!
this feels like it can ride the trains. (P3, doesn't seem to be a huge problem)
|
||
Comment 24•5 years ago
|
||
We reproduced this issue using Fx 75.0a1(2020-02-21) on macOS 10.13.6.
I can confirm this issue is fixed on Fx 76.0a1(2020-03-18), we verified using Ubuntu 18.04, macOS 10.13 and Windows 10 x64.
We also noticed the default Top Sites Twitter icon is different from the icon we get if we dismiss the default Twitter icon, then visit twitter.com/firefox. Is this known/intended?
|
|
Assignee | |
Comment 25•5 years ago
|
||
The icon used in the default set of Urlbar Top Sites is a static icon in-tree. It is 32x32. The icons fetched in this patch are 16 pixels square, times the window's devicePixelRatio. The icon on the top in the screenshot in comment 22 is likely 16px square and the icon on the bottom, being fetched by Places, is 32px square. I'm on a @2x screen and I can't reproduce this issue, so presumably both Places and the code introduced in this patch are fetching icons that are 32px square.
Will, are you using a @1x screen? Most of our users do, so this is probably worth fixing (if this is indeed the issue).
|
|
Reporter | |
Comment 26•5 years ago
|
||
(In reply to Harry Twyford [:harry] from comment #25)
The icon used in the default set of Urlbar Top Sites is a static icon in-tree. It is 32x32. The icons fetched in this patch are 16 pixels square, times the window's devicePixelRatio. The icon on the top in the screenshot in comment 22 is likely 16px square and the icon on the bottom, being fetched by Places, is 32px square. I'm on a @2x screen and I can't reproduce this issue, so presumably both Places and the code introduced in this patch are fetching icons that are 32px square.
Will, are you using a @1x screen? Most of our users do, so this is probably worth fixing (if this is indeed the issue).
Are you referring to the OS display scaling? My laptop is set to 175% display scaling (Windows 10). If this is only an issue with strange display scalings like 175%, I still think it should be fixed especially because I can't repro in release Firefox/non-quantumbar
|
|
Assignee | |
Comment 27•5 years ago
|
||
No, I'm referring to the pixel density of the screen. If you open the browser console (I believe it's Ctrl + Shift + J on Windows, or it's in the Tools > Web Developer menu) and type window.devicePixelRatio, what is the output?
|
|
Reporter | |
Comment 28•5 years ago
|
||
(In reply to Harry Twyford [:harry] from comment #27)
No, I'm referring to the pixel density of the screen. If you open the browser console (I believe it's
Ctrl + Shift + Jon Windows, or it's in the Tools > Web Developer menu) and typewindow.devicePixelRatio, what is the output?
Nothing is coming up in the browser console...
If you want to know the pixel density of the screen, it's 201 PPI (it's a microsoft "Surface Laptop"). Windows 10 OS display resolution is set to the same as the display (2256 x 1504) and the OS displaying scaling is 175%.
|
|
Reporter | |
Comment 29•5 years ago
|
||
I just changed my laptop to 200% display scaling, restarted and the issue remains. The URL is https://twitter.com/firefox
|
|
||
Updated•5 years ago
|
Description
•