1264893 - The favicon is not displayed after opening multiple tabs

Status: VERIFIED FIXED

(Firefox for iOS :: Favicons, defect)

Description

[Simon Basca [:SimonB]]

Attachment: Screenshot of the issue

Build: 4.0.0b1

Steps to reproduce:
1. Open Firefox
2. Open a new tab and go to youtube.com ( do not wait for the page to load)
3. Open a new tab and go to amazon.com ( do not wait for the page to load)
4. Open a new tab and go to google.com ( do not wait for the page to load)
5. Open a new tab and go to wikipedia.com ( do not wait for the page to load)
6. Open a new tab and go to olx.ro ( do not wait for the page to load)
7. Open tabs tray and observe that the favicon is not displayed for amazon and olx pages.

Actual result:
- The favicon is not displayed although the page has been loaded

Expected result:
- The favicon should be displayed

Comment 1

[Maurya Talisetti]

Can anyone help me with this?

I reviewed the code and found out that some custom scripts are being executed after page load. When the tab is in the foreground, all these scripts are getting executed ok. But, when the tab is in the background, the Favicon script [1] is not sent to the HelperManager.userContentController() [2]. 

[1] https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Browser/FaviconManager.swift#L21

[2] https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Browser/Tab.swift#L475

Comment 2

[Brian Nicholson (:bnicholson)]

10:02:09 AM <bnicholson> maurya: we don't actually use page events to send the favicons message back to swift -- swift asks for the favicons explicitly in KVOLoading
10:02:34 AM <bnicholson> https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Browser/BrowserViewController.swift#L863
10:03:00 AM <bnicholson> note a few lines above that, where we back out of the KVO listener if the webview isn't the selected tab

Comment 3

[Brian Nicholson (:bnicholson)]

Hey Maurya, any luck?

Comment 4

[Maurya Talisetti]

Hi Brian, I was out on vacation last week. I'll try it out this week and update you :)

Comment 5

[Maurya Talisetti]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1869

Comment 6

[Maurya Talisetti]

Hi Brian,

I was wondering if we should remove the "selected-tab?" check from KVOCanGoForward/Back also. What do you think? It might not hurt to have the estimated progress also be updated regardless of whether it is the selected tab. So, we might as well remove the "selected-tab" check all together.

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 8758522 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1869

Thanks for the PR; this is looking good! I left a few comments in GitHub.

(In reply to Maurya Talisetti from comment #6)
> I was wondering if we should remove the "selected-tab?" check from
> KVOCanGoForward/Back also. What do you think? It might not hurt to have the
> estimated progress also be updated regardless of whether it is the selected
> tab. So, we might as well remove the "selected-tab" check all together.

I think we do want the check for back/forward, the progress bar, and also stop/reload (as mentioned in the PR comments). The reason is that these UI elements are reused among all tabs, and they act on the active tab. We wouldn't want a non-selected tab to change the UI since the UI doesn't reflect any tab other than the selected one.

Comment 8

[Brian Nicholson (:bnicholson)]

Also, apologies for the slow response. I was on vacation myself this week!

Comment 9

[Brian Nicholson (:bnicholson)]

We should land this in 5.0 since the bug also results in tab URLs not being updated in the background.

Comment 10

[Brian Nicholson (:bnicholson)]

Hey Maurya, any progress with this one? It'd be great if we could get this fix in 5.0 if possible!

Comment 11

[Maurya Talisetti]

Hi Brian, yeah I have it ready. I had a problem with my github account and so was waiting to push it. I'll push it soon.

Comment 12

[Maurya Talisetti]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1961

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 8766181 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1961

Awesome. Thanks!

Comment 14

[Brian Nicholson (:bnicholson)]

master: https://github.com/mozilla/firefox-ios/commit/76c60f58b9cdf6da65397d819c9cdaf555632b11
v5.x: 7fde357

Comment 15

[Simon Basca [:SimonB]]

Verifying as fix on 5.0b14.