The favicon is not displayed after opening multiple tabs

VERIFIED FIXED

Status

()

Firefox for iOS
Favicons
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: SimonB, Assigned: Maurya Talisetti)

Tracking

unspecified
All
iOS

Firefox Tracking Flags

(fxios-v3.0 affected, fxios-v4.0 affected, fxios-v5.0 verified, fxios-v6.0 fixed, fxios5.0+)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8741692 [details]
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
Rank: 6
tracking-fxios: ? → +

Updated

2 years ago
Whiteboard: [good first bug]
(Assignee)

Updated

2 years ago
Assignee: nobody → maurya1985
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
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
Flags: needinfo?(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
Flags: needinfo?(bnicholson)
Hey Maurya, any luck?
Flags: needinfo?(maurya1985)
(Assignee)

Comment 4

2 years ago
Hi Brian, I was out on vacation last week. I'll try it out this week and update you :)
Flags: needinfo?(maurya1985)
(Assignee)

Comment 5

2 years ago
Created attachment 8758522 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1869
Attachment #8758522 - Flags: review?(bnicholson)
(Assignee)

Comment 6

2 years ago
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.
Flags: needinfo?(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.
Flags: needinfo?(bnicholson)
Attachment #8758522 - Flags: review?(bnicholson) → feedback+
Also, apologies for the slow response. I was on vacation myself this week!
We should land this in 5.0 since the bug also results in tab URLs not being updated in the background.
tracking-fxios: + → 5.0+

Updated

2 years ago
Whiteboard: [good first bug] → [good first bug][needsuplift]
Hey Maurya, any progress with this one? It'd be great if we could get this fix in 5.0 if possible!
Flags: needinfo?(maurya1985)
(Assignee)

Comment 11

2 years ago
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.
Flags: needinfo?(maurya1985)
(Assignee)

Comment 12

2 years ago
Created attachment 8766181 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1961
Attachment #8758522 - Attachment is obsolete: true
Attachment #8766181 - Flags: review?(bnicholson)
Comment on attachment 8766181 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1961

Awesome. Thanks!
Attachment #8766181 - Flags: review?(bnicholson) → review+
master: https://github.com/mozilla/firefox-ios/commit/76c60f58b9cdf6da65397d819c9cdaf555632b11
v5.x: 7fde357
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-fxios-v5.0: --- → fixed
status-fxios-v6.0: --- → fixed
Resolution: --- → FIXED
Whiteboard: [good first bug][needsuplift] → [good first bug]
(Reporter)

Comment 15

2 years ago
Verifying as fix on 5.0b14.
Status: RESOLVED → VERIFIED
status-fxios-v5.0: fixed → verified
You need to log in before you can comment on or make changes to this bug.