Closed Bug 1192810 Opened 5 years ago Closed 5 years ago

Home panel flashes before content loads on cold boot

Categories

(Firefox for iOS :: Home screen, defect)

All
iOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fxios + ---

People

(Reporter: sleroux, Assigned: sleroux)

Details

(Keywords: regression, reproducible, Whiteboard: [1.0?])

Attachments

(1 file, 1 obsolete file)

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
tracking-fxios: --- → ?
No this looks like a regression. I don't remember the top sites panel flashing in before rendering content.
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
I shall take once done with presentación
Bryan: take it when you're done with the other two+ bugs :)
Hardware: Other → All
Whiteboard: [1.0?]
Assignee: nobody → bmunar
Assignee: bmunar → 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?
Flags: needinfo?(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.
Flags: needinfo?(bnicholson)
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.
Attachment #8647104 - Flags: review?(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!
Attachment #8647104 - Flags: review?(bnicholson) → review-
Ya looks like this breaks in this scenario - I'll see what else I can do :(
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?
Attachment #8647104 - Flags: review- → review?(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 :)
Attachment #8647104 - Flags: review?(bnicholson) → review+
Attachment #8647104 - Attachment is obsolete: true
Okay - I implemented a proper way of doing this instead of a lazy hack. I now delay KVO registration until restoration is complete.
Attachment #8647526 - Flags: review?(bnicholson)
Status: NEW → ASSIGNED
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
Attachment #8647526 - Flags: review?(bnicholson) → feedback+
(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...".
Ya that would work. Bah and I was trying to avoid adding more boolean state.
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)
Attachment #8647526 - Flags: review?(bnicholson)
Comment on attachment 8647526 [details] [review]
https://github.com/mozilla/firefox-ios/pull/916

Nice, thanks for taking this!
Attachment #8647526 - Flags: review?(bnicholson)
Attachment #8647526 - Flags: review+
Attachment #8647526 - Flags: feedback+
Merged/Resolved
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.