1192810 - Home panel flashes before content loads on cold boot

Status: RESOLVED FIXED

(Firefox for iOS :: Home screen, defect)

Description

[Stephan Leroux [:sleroux]]

STR: 

1. Visit mozilla.org
2. Hard close the app
3. Open app

Expected:

Should see white content while page is loading in

Actual:

Home panel/top sites flashes in before web content appears

Comment 1

[Richard Newman [:rnewman]]

Part of Bug 1191368?

Comment 2

[Stephan Leroux [:sleroux]]

No this looks like a regression. I don't remember the top sites panel flashing in before rendering content.

Comment 3

[Brian Nicholson (:bnicholson)]

I've noticed this for awhile, but forgot to file a bug for it. I think this happened after bug 1182274, which changed the way we detect URL changes on the webview. Previously, we used didCommitNavigation to look for URL changes, but that wasn't reliable since it wasn't fired for all cases. Using KVO to monitor the web view's URL is more reliable -- so reliable, in fact, that we we catch the very brief period between the web view being created with the initial about:home URL and setting the follow-up URL!

Restored tabs are created without specifying a request [1], so they'll use the default about:home request before loading the sessionData. The fix might be as simple as skipping the default request if "restoring" is true.

[1] https://github.com/mozilla/firefox-ios/blob/12ae8d357272b073490cf5bd048dd3e6729337a1/Client/Frontend/Browser/TabManager.swift#L374

Comment 4

[Bryan Munar (:bkmunar)]

I shall take once done with presentación

Comment 5

[Richard Newman [:rnewman]]

Bryan: take it when you're done with the other two+ bugs :)

Comment 6

[Stephan Leroux [:sleroux]]

After some digging, it looks like the reason the home pane view controller flashes is because we're resetting the page's push state to the home URL first, then navigating to their latest page:

https://github.com/mozilla/firefox-ios/blob/master/Client/Assets/SessionRestore.html#L35

The first url is usually always about:home which, when replaceState gets called, loads into the web view which in turn updates WKWebView's URL property which triggers the KVO observer. I don't know what this is supposed to do but is there something we can do about this?

Comment 7

[Brian Nicholson (:bnicholson)]

We have to call replaceState (rather than pushState) with the first URL to be restored since that removes the SessionRestore.html loader page from the back/forward history. We then have to use pushState in order of the history entries, so I'm not sure we can avoid this on the JS end.

One way to fix this would be to delay attaching the KVO observers until the restore has happened, which kind of makes sense anyway since we don't want the restore to fire these events. Not a super simple fix like I was hoping, though.

Comment 8

[Stephan Leroux [:sleroux]]

Attachment: https://github.com/mozilla/firefox-ios/pull/916

Found a simple workaround for this. In the KVO observer, we check to see if the previous URL was session restore. If so, skip the reader mode/home panel checks.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8647104 [details] [review]
https://github.com/mozilla/firefox-ios/pull/916

Heh, this is the approach I was going to suggest first, but I don't think it will work in the case where about:home is actually the active restored URL. That is: create a new tab (which opens about:home), then close/reopen the browser. I think that will break the home panel appearing since the previous page was the session restore URL. Please reflag review if I'm wrong!

Comment 10

[Stephan Leroux [:sleroux]]

Ya looks like this breaks in this scenario - I'll see what else I can do :(

Comment 11

[Stephan Leroux [:sleroux]]

Comment on attachment 8647104 [details] [review]
https://github.com/mozilla/firefox-ios/pull/916

I've added a quick/dirty workaround for the restore->home issue

https://github.com/mozilla/firefox-ios/commit/fd39d237eb9c3ad1cb4a33d4f90dea476b9a6925

Thoughts?

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 8647104 [details] [review]
https://github.com/mozilla/firefox-ios/pull/916

Well, this is kind of gross, but I guess it's good enough for now :)

Comment 13

[Stephan Leroux [:sleroux]]

Attachment: https://github.com/mozilla/firefox-ios/pull/916

Okay - I implemented a proper way of doing this instead of a lazy hack. I now delay KVO registration until restoration is complete.

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 8647526 [details] [review]
https://github.com/mozilla/firefox-ios/pull/916

I think this looks OK for restored tabs, but this won't work for new tabs that aren't restored. For example, try creating a tab the navigation to a new URL -- the home panel won't disappear.

Maybe add a restoring boolean to Browser that we set to true at the end of this if condition [1], then set to false after firing didRestoreSessionForBrowser? Then you could do if !browser.restoring { webView.addObserver(...) } in didCreateWebView. I think that might also allow you to drop the try/catch if you wanted since it would be safe to call the webView.removeObserver(...) as long browser.restoring is false.

[1] https://github.com/mozilla/firefox-ios/blob/ca7fcb5a2038ec4ba6c8fc7ba293ec91305b6a0b/Client/Frontend/Browser/Browser.swift#L99

Comment 15

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #14)
> For example, try creating a tab the navigation to a new URL

Typos. This should have been: "For example, try creating a tab, then navigating to a new URL...".

Comment 16

[Stephan Leroux [:sleroux]]

Ya that would work. Bah and I was trying to avoid adding more boolean state.

Comment 17

[Stephan Leroux [:sleroux]]

Comment on attachment 8647526 [details] [review]
https://github.com/mozilla/firefox-ios/pull/916

Updated with the restoration flag and it seems to be working fine for me (new tab + restoration)

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 8647526 [details] [review]
https://github.com/mozilla/firefox-ios/pull/916

Nice, thanks for taking this!

Comment 19

[Stephan Leroux [:sleroux]]

Merged/Resolved