Tabs opened in background have no thumbnail

VERIFIED FIXED

Status

()

VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: bnicholson, Assigned: jhugman)

Tracking

unspecified
All
iOS

Firefox Tracking Flags

(fxios-v1.1 fixed, fxios1.1+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

48 bytes, text/x-github-pull-request
bnicholson
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
Long press a link, open in it in the background, then open the tabs tray. The newly opened link has no screenshot.

Similar to bug 1178394, except this is about the newly opened tabs not having a screenshot as opposed to the old tab.

Comment 1

3 years ago
On Google Chrome's iOS app, I noticed that when they open a background tab, Chrome is loading up the URL for the background tab even as you stay on the original tab. That way, when you switch tabs, the newly opened tab has a thumbnail so you can see what's on the tab before you click it. It also has the other effect of making your new tab preloaded so you don't have to wait for it.

Is that something we would consider doing too or should we hold off on that? That would fix this bug. However, on Safari's iOS app, it doesn't do that and just forces a newly opened tab to come to the foreground so that the problem of background tabs not having a thumbnail is non applicable.
Well now they should get a favicon in their tile, I assume that works now, Brian?
(Reporter)

Comment 3

3 years ago
Yeah, we already load the page immediately (you don't have to switch to it to load it). The title and favicon work already; we just need to throw in logic to take a screenshot somewhere.

Comment 4

3 years ago
Created attachment 8635645 [details] [review]
Pull request
Attachment #8635645 - Flags: review?(bnicholson)
(Reporter)

Updated

3 years ago
Assignee: nobody → allenngn
Status: NEW → ASSIGNED
(Reporter)

Comment 5

3 years ago
Comment on attachment 8635645 [details] [review]
Pull request

Thanks for the pull request! For performance reasons, I don't think we want to take a full screenshot of every tab for every single page load, so we should figure out a way to narrow the scope of this fix.

Right now, we take a screenshot of the active tab every time we switch to the tab tray. What if iterate through all of the tabs and take screenshots of those that have a nil thumbnail?
Attachment #8635645 - Flags: review?(bnicholson) → feedback+

Comment 6

3 years ago
It's not a performance hit. The webView.didFinishNavigation() method gets called by delegation and only that specific webView it's being called for takes a screenshot. The code I put in doesn't iterate over the tabManager to take a screenshot of each tab. It only takes a screenshot once it gets the go ahead from whatever tab says it is done loading.

Iterating through all of the tabs every time the user switches to the tab tray and taking a screenshot of all tabs that have a nil thumbnail runs into the problem of prematurely screenshotting. Say a user opens a link in a background tab, then switches to the tab tray immediately before the new URL can be loaded. The webView may not have finished loading its web content, but then we will still take a screenshot anyway, resulting in a blank white screenshot for the tab. That would complicate it even more. At least with a nil thumbnail, we know that the tab doesn't have a valid thumbnail. But if the blank white screenshot takes the place, we then can't test for nil since it will have a valid memory address (which holds a screenshot of a UIView that hasn't loaded anything).
Going to throw this into the nom queue for discussion
tracking-fxios: --- → ?
tracking-fxios: ? → 1.1+
Flags: needinfo?(bnicholson)
(Reporter)

Comment 8

3 years ago
Sorry to take so long for these responses -- I must have missed the bugmail!

(In reply to allenngn from comment #6)
> It's not a performance hit. The webView.didFinishNavigation() method gets
> called by delegation and only that specific webView it's being called for
> takes a screenshot.

Well, that's the performance hit I'm talking about :) Taking a screenshot for every page load is not free; doing some brief testing, I see that one screenshot takes 100+ ms on my iPod touch. It happens on the UI thread, so this can cause a hiccup when scrolling or interacting with the browser.

> The code I put in doesn't iterate over the tabManager to
> take a screenshot of each tab. It only takes a screenshot once it gets the
> go ahead from whatever tab says it is done loading.

Right -- when I said "every tab for every single page load", I just meant that all tabs are hooked into this delegate, and that every navigation change triggers a screenshot (as opposed to iterating through every tab for every navigation change). Sorry, poorly worded comment.

> Iterating through all of the tabs every time the user switches to the tab
> tray and taking a screenshot of all tabs that have a nil thumbnail runs into
> the problem of prematurely screenshotting.

Agreed, and I have seen blank thumbnails more than once, so let's roll with your change here. I think empty thumbnails are probably more user-visible than the delay mentioned above, and this is the only reliable way to fix it. Worth keeping an eye on perf regressions, though.

So, just to summarize, we now take screenshots in two different places:
1) After every page load (this bug), which should give a good base screenshot for every tab.
2) When going to the tabs tray, we take a screenshot of the active tab, plus any other tabs that don't have screenshots. Given #1, the "other tabs" part of this shouldn't be very common (and should be limited to newly created tabs that haven't fully loaded yet).
Flags: needinfo?(bnicholson)
(Reporter)

Comment 9

3 years ago
Allen, I rebased your changes and created a branch here: https://github.com/thebnich/firefox-ios/tree/background-tabs-screenshot

Unfortunately, your fix doesn't seem to work with the steps I'm using:
1) Long press a link to add a tab in the background.
2) Open the tab tray before that page loads. You'll see a gray thumbnail.
3) Wait a few seconds (or set a breakpoint) until the background page is finished loading.
4) Tap the thumbnail of the tab you came from, then open the tab tray again. The other thumbnail is still gray.

Do you have a chance to look at this again?
Flags: needinfo?(allenngn)

Comment 10

3 years ago
Sorry, it's been awhile since I worked on this. The code base has changed a lot, and my life has gotten a lot busier. I don't think I'll be able to fix this bug.

That was the one scenario that I couldn't get to work. Opening the tab tray immediately or at least before the newly opened tab was able to load always seemed to mess up the snapshot.
Flags: needinfo?(allenngn)

Updated

3 years ago
Assignee: allenngn → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

3 years ago
Assignee: nobody → jhugman
Status: NEW → ASSIGNED
(Assignee)

Comment 11

3 years ago
Created attachment 8672803 [details] [review]
Pull request

I went with best-worst case: delay during potential scrolling, rather than delay on press for tab tray.
Attachment #8635645 - Attachment is obsolete: true
Attachment #8672803 - Flags: review?(bnicholson)
(Reporter)

Comment 12

3 years ago
Comment on attachment 8672803 [details] [review]
Pull request

Hm, this looks pretty much the same as the branch linked to in comment 9 (rebased commit at https://github.com/thebnich/firefox-ios/commit/42795bcc4b8decf3eb75293c22c88a6863367a8c). We didn't land that because of the issues mentioned in that comment, and I imagine this PR has the same issues. Can you try those STR and see if you can figure out what's going on?
Attachment #8672803 - Flags: review?(bnicholson) → feedback+
(Reporter)

Comment 13

3 years ago
Comment on attachment 8672803 [details] [review]
Pull request

This is still an improvement over what we have now since it still captures background screenshots for other cases. Filed bug 1215669 to look into the issue from comment 0.
Attachment #8672803 - Flags: feedback+ → review+
(Reporter)

Comment 14

3 years ago
(In reply to Brian Nicholson (:bnicholson) from comment #13)
> Filed bug 1215669 to look into the issue from comment 9.

Comment 9, rather.

https://github.com/mozilla/firefox-ios/commit/13d48fefa753547c7b2ad85149eace43f0478d1c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-fxios-v1.1: ? → fixed
Resolution: --- → FIXED
Verifying as fixed on build on current TestFlight build (1111)

Tested the following:
 - Open link in background in normal browsing > wait for the page to load > open tabs tray - the newly opened link has screenshot.
 - Open link in background in private browsing > wait for the page to load > open tabs tray - the newly opened link has screenshot
 - Open link in background in private browsing from normal browsing > wait for the page to load > open tabs tray - the newly opened link has screenshot.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.