Offline articles are not loaded with slim-navigation-manger enabled. |
|||
Issue descriptionApp Version: 66.0.3359.12 beta iOS Version: 10.3.3, 11.2.6 Device: iPhone, iPad URL: any Precondition: Enable #slim-navigation-manager from about://flags Steps to reproduce: 1. Launch Google Chrome 2. Navigate to any webpage and add to reading list 3. Wait for the article tobe ready for offline view 4. Turn on Airplane mode and open the article from reading list Observed results: No internet connection error page is displayed. Expected results: Article should be displayed for offline view. Note: Article loads fine when long tap and select Veiw Offline version in new tab. But again reloading this page doesn't work Number of times you were able to reproduce: 5/5 Bug reproducible after clean install: Yes Bug reproducible after clearing cache and cookies: Yes Bug reproducible on Chrome Mobile on Android: NA Bug reproducible on Safari/Firefox: Firefox: NA, Safari: NA Bug reproducible on current stable build (App Version, iOS Version): M65 NA New feature Bug reproducible on the current beta channel build (App Version, iOS Version): M66 YES Link to video/image: https://drive.google.com/file/d/1A11JbAPBxZ-1ZqYSfCjVHPis5ZUezgMO/view
,
Mar 27 2018
This is caused by a conflict between native error view and reading list. The following happens after step 4:
5. Original URL of the reading list item is loaded in web view
6. Since we're in Airplane Mode, |webView:didFailProvisionalNavigation| is called.
7. Native error handling triggers a load of placeholder URL (about:blank?for=originalURL) in the web view, in preparation for presenting native error
8. |webView:didFinishNavigation| is called for placeholder URL. It calls CRWWebController's |didFinishNavigation|, which calls WebState::OnPageLoaded().
9. ReadingListWebStateObserver::PageLoaded() is called with *load_completion_status = SUCCESS*, which early exits before loading offline content.
In step 9, load_completion_status should be FAILURE. Changing this will cause ReadingListWebStateObserver::LoadOfflineReadingListEntry() to run (good). However, LoadOfflineReadingListEntry() also modifies the NavigationItem's URL (from originalURL to chrome://offline...) and reloads the item. Because WKBasedNavigationManager delegates to WKWebView for reload, changing NavigationItem's URL has no effect, so about:blank?for=originalURL is loaded again. When it reaches the |webView:didFinishNavigation| stage, it early exists (incorrectly) at this condition:
} else if (item->GetURL() != originalURL) {
// The |didFinishNavigation| callback can arrive after another
// navigation has started. Abort in this case.
return;
}
What it should do is recognize that this is an offline content load and directly triggers the offline content native controller. Care needs to be taken to make sure the NavigationItem's ErrorRetryState stays in DisplayingErrorForFailedNavigation, such that a subsequent reload will attempt to reload the original URL again (this requires ReadingListWebStateObserver to not change the navigation item's URL).
,
Apr 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e423142a5bdd917f867ee23c3f36158183b0b8b6 commit e423142a5bdd917f867ee23c3f36158183b0b8b6 Author: Danyao Wang <danyao@google.com> Date: Thu Apr 05 16:44:05 2018 [Nav Experiment] Fix offline reading list for WKBasedNavigationManager. When handling a no-Internet-connection error, WKBasedNavigationManager incorrectly triggers WebState::OnPageLoaded with load completion status SUCCESS when the native error view is loaded. This prevented ReadingListWebStateObserver from triggering and loading offline content and is now fixed. Since offline reading list is essentially another type of native error, we must not update the navigation item's |errorRetryState| when loading a chrome://offline URL. This is so that a reload of the page will retry loading the original URL. This is fixed by only updating the error retry state machine when the URL is a web URL. This is OK because app-specific URL should never fail to load. Bug: 819805 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I870ef99522ba6f5aeb4d37c58289155aa8cb79c9 Reviewed-on: https://chromium-review.googlesource.com/996434 Commit-Queue: Danyao Wang <danyao@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#548454} [modify] https://crrev.com/e423142a5bdd917f867ee23c3f36158183b0b8b6/ios/web/web_state/ui/crw_web_controller.mm
,
Apr 5 2018
,
Apr 10 2018
Verified on chrome canary version 67.0.3393.0 on iPhone 8 plus with iOS 11.4 beta 1 and iPad Pro with iOS 11.2.6 following the steps mentioned in comment #0. Article are displayed in offline view. Looks good. |
|||
►
Sign in to add a comment |
|||
Comment 1 by huangml@chromium.org
, Mar 8 2018Owner: danyao@chromium.org
Status: Assigned (was: Untriaged)