1059470 - Fallback to favicon.ico if content doesnt define icon

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Description

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: Logcat reproducing the bug

Description:
This bug is created to better describe bug 1058266. It is a regression and is introduced with Browser2.

Repro Steps:
1) Update a Flame device to BuildID: 20140826180827
2) Connect to internet by any method
3) Launch Browser and access Facebook.com by typing it in URL field
4) Once Facebook loads, tap on the '...' icon to the right of URL field, select Add to Home Screen and confirm addition
5) Navigate to Homescreen and observe the shortcut icon being added

Expected: 
Shortcut is created with an icon related to the webpage

Actual:
Shortcut is created with the generic rockship icon

Environmental Variables:
Device: Flame 2.1 Master
BuildID: 20140826180827
Gaia: 6e804a42ab90f4251c7fe8c68731dc1c6abd8006
Gecko: 0753f7b93ab7
Version: 34.0a1 (2.1 Master)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0
  
Repro frequency: 3/3

Attaching: Logcat.

Comment 1

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: Screenshot of issue (bottom right)

Comment 2

[Pi Wei Cheng [:piwei] (inactive)]

Correction: On comment 1, the facebook icon is on bottom middle.

Comment 3

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
bad user experience/functionality with a core feature.

bug 1058266 isn't a regression because that issue reproduces since Browser2 was implemented (20140818173616 is the build id that has it first).

QAWanted to verify that facebook.com icon behaves in the same way as the other testing done in bug 1058266 and is also not a regression for Browser2 feature.

Comment 4

[Chris Kreinbring [:CKreinbring]]

The bug repros on Flame 2.1
Actual result: The Facebook bookmark has the generic rocket icon instead of the Facebook icon.

BuildID: 20140818173616
Gaia: b33b4d9558e0b9eabbfda7be23435e2b38fd40bf
Gecko: 111a1da2a95d
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Comment 5

[Joshua Mitchell (Inactive)]

issue repros on the first available Browser 2 build so it is not a regression

Comment 6

[yifan [:yifan][:yliao]]

Icons are saved by the system browser when handling mozbrowsericonchange events. When browsing to the mobile facebook page, the mozbrowericonchange event is not triggered by gecko so the system browser doesn't get the icon.

Here is the icon link in the mobile facebook web page:

<link href="https://fbstatic-a.akamaihd.net/rsrc.php/v2/y9/r/94cCwk8yDOQ.png" rel="apple-touch-icon-precomposed" sizes="114x114" />

Comment 7

[yifan [:yifan][:yliao]]

The "apple-touch-icon-precomposed" icon is not supported. See bug 921014 for more details.

https://bugzilla.mozilla.org/show_bug.cgi?id=921014#c47

Comment 8

[yifan [:yifan][:yliao]]

Hi Karl,

According to https://bugzilla.mozilla.org/show_bug.cgi?id=921014#c44 , the 'apple-touch-icon-precomposed' is not going to be supported on FxOS. Could we have web compatibility team's support on this icon issue? Or this should be handled by another team? Thank you. :)

Comment 9

[Karl Dubost💡 :karlcow]

Let's see…


When requesting Facebook home page, we are redirected to the mobile.

→ http -v GET https://m.facebook.com/ 'User-Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0'

GET / HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate
Host: m.facebook.com
User-Agent:  Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0



HTTP/1.1 200 OK
[… etc…]

    <meta name="viewport" content="initial-scale=1,maximum-scale=1,user-scalable=no" />
    <link href="https://fbstatic-a.akamaihd.net/rsrc.php/v2/ya/r/O2aKM2iSbOw.png" rel="apple-touch-icon-precomposed" sizes="196x196" />
[… etc…]

which has indeed only 1 link.
Wondering what is send to other browsers: 

Safari iOS 6
    <meta name="viewport" content="initial-scale=1,maximum-scale=1,user-scalable=no" />
    <link href="https://fbstatic-a.akamaihd.net/rsrc.php/v2/ya/r/O2aKM2iSbOw.png" rel="apple-touch-icon-precomposed" sizes="60x60" />

So from what I gather, all devices are receiving a version of an icon which is basically made for Apple, be chrome, safari, windows which is a bit surprising.

I guess it's worth contacting Facebook for it and see if we can motivate them to change it given that iOS7 and later do not add any effects to the icons.

Comment 10

[Karl Dubost💡 :karlcow]

Harald has good relationships with Facebook. So he might be able to accelerate the process.

Comment 11

[yifan [:yifan][:yliao]]

Thank you for the great help! :)

Comment 12

[Gregor Wagner [:gwagner]]

(In reply to Joshua Mitchell [:Joshua_M] from comment #5)
> issue repros on the first available Browser 2 build so it is not a regression

Did this work with the old browser?

Comment 13

[Ben Francis [:benfrancis]]

It did work on the old browser, but it used the favicon. Are we trying to load a precomposed apple-touch-icon and it's failing?

Comment 14

[Candice Serran (:cserran)]

Confirmed blocker: regression and definite icon bugs

Comment 15

[Ben Francis [:benfrancis]]

Dale, do you know whether we would fire an iconchange event for a precomposed apple-touch-icon that we can't use?

I would guess that either we're firing an event for an apple-touch-icon we can't use instead of firing the iconchange event with a standard rel=icon link for the icon we can use, or the window manager doesn't fall back to favicon.ico in the document root like the browser app did.

Comment 16

[Karl Dubost💡 :karlcow]

(In reply to Ben Francis [:benfrancis] from comment #15)
> Dale, do you know whether we would fire an iconchange event for a
> precomposed apple-touch-icon that we can't use?


See Comment #7

Comment 17

[Dale Harvey (:daleharvey)]

We wont fire for |rel="apple-touch-icon-precomposed"| as Karl clarified, and we currently dont fallback to the favicon at root, we should 1. ask facebook to fix it to remove -precomposed 2. add the fallback to favicon at root

Comment 18

[Ben Francis [:benfrancis]]

Alive, it sounds like we want to fall back to "favicon.ico" at the root of the domain if no icon is provided in a link tag. I will look at this when I get back from PTO on Wednesday if you or someone else doesn't steal it by then :)

Comment 19

[Karl Dubost💡 :karlcow]

Also note that when iOS (7.1 at least) devices do not find a suitable size for icons for the homescreen. Like for example the tiny 16x16px, they generate a partial screenshot of the home page that they use as an icon. 

See for example on Mozilla Web site where it is the case currently.
https://bug1045961.bugzilla.mozilla.org/attachment.cgi?id=8484668



(Wondering what Sean thinks about it)
Another strategy instead of the fallback to the "crappy" favicon 16x16px could be something like:

1. Parse the domain name      (ex: facebook.com)
2. Identify the first letter  (ex: F)
3. Compute the md5 hash of the domain in Hex
4. Take the 6 first characters of this hash and use it as a background-color
5. Create an icon with a white letter on top the background-color.


Some examples (using python):

>>> import md5
>>> md5.new('facebook.com').hexdigest()[0:6]
'2343ec'
>>> # so a white F on #2343EC
>>> md5.new('twitter.com').hexdigest()[0:6]
'7905d1'
>>> # so a white T on #7905D1
>>> md5.new('wikipedia.org').hexdigest()[0:6]
'465f7f'
>>> # so a white W on #465F7F
>>> md5.new('mozilla.org').hexdigest()[0:6]
'b5292b'
>>> # so a white M on #B5292B


The benefits:
* it avoids a pixelated icon on the home screen. 
* it makes the system a bit playful.

Comment 20

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Dale Harvey (:daleharvey) from comment #17)
> We wont fire for |rel="apple-touch-icon-precomposed"| as Karl clarified, and
> we currently dont fallback to the favicon at root, we should 1. ask facebook
> to fix it to remove -precomposed 2. add the fallback to favicon at root

I think we should do both. Ask facebook to improve and add the fallback.

Comment 21

[Jason Smith [:jsmith]]

Flagging regression to indicate it's a user facing regression from the old browser app --> new system browser.

Comment 22

[Ben Francis [:benfrancis]]

Update: I have a patch for this, but after chatting to Dale it seems to do this properly we need to modify IconHelper to work differently.

Dale suggests adding a getIcon(url, placeObj) method and then using this all over the system app.

Comment 23

[Ben Francis [:benfrancis]]

Hi Dale, could you provide a bit more information about the refactoring you think is necessary? I'm not sure I fully understand what you want to do here.

IconsHelper is in /shared and is used in multiple places in the system so there's a risk of regressions if we're not careful.

Comment 24

[Dale Harvey (:daleharvey)]

Pretty much as you said

IconHelper.getIcon = function(url, placeObj) { 
  if (!placeObj.icons) { 
    return url + favicon.ico
  } 
  return IconHelper.getBestIcon(placeObj);
}

along those lines, formatting the url correctly and fixing the callers

Comment 25

[Dale Harvey (:daleharvey)]

the placeObj will be optional, right now we pass the icons object through some objects and activities and I want to get rid of that, eventually getIcon can look like

getIcon = function(url, [place]) { 
  if (localCache(url)) { 
    return localCache[url];
  }
  // The optional place argument is a pure optimisation to avoid a read
  if (!place) { 
    place = readPlaces(url);
  }
  return getBestIcon(place); // and cache it
}

Which means everything gets cached locally and its consistent etc etc

Comment 27

[Dale Harvey (:daleharvey)]

Talked with ben and I will steal this one

Comment 28

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/24088

Comment 29

[Kevin Grandon :kgrandon]

Comment on attachment 8489922 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/24088

Makes sense to me. Thanks!

Comment 30

[Kevin Grandon :kgrandon]

In mater: https://github.com/mozilla-b2g/gaia/commit/679b6a1f3786d429419cd411693536b23785952b

Comment 31

[Kevin Grandon :kgrandon]

Comment on attachment 8489922 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/24088

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature implementation.
[User impact] if declined: Bad ux using the new system browser as often favicons will not be displayed.
[Testing completed]: Manual and unit tested.
[Risk to taking this patch] (and alternatives if risky): I would say low risk as it's already somewhat broken.
[String changes made]: None.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/74b03617b47f94743e03f288dc4d5c384ae22373

Comment 33

[Brogan Zumwalt [Inactive]]

Issue verified fixed on Flame 2.1 and Flame 2.2

Actual Results: Shortcut is created with an icon related to the webpage

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141023001201
Gaia: 1e48e3e40e0780c0cd07a3457e5fe2efeeb542d1
Gecko: 09fb60a37850
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
Build ID: 20141023040204
Gaia: 27a1d1baaa8e375b70e043efee67d5f2206c330b
Gecko: 88adcf8fef83
Version: 36.0a1 (Master)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Comment 34

[:Harald Kirschner :digitarald]

Remove ni?