Go forward does not work for interstitials that are in the middle of navigation stack |
|||||
Issue descriptionApp Version (from "Chrome Settings > About Chrome"): 47 iOS Version: iOS 10.2 Device: iPhone SE Steps to reproduce: 1.) Turn on Airplane Mode 2.) Go to expired.badssl.com 3.) Go to google.com 4.) Go Back 5.) Go Back 6.) Turn off Airplane Mode 7.) Go Forward 8.) Go Forward Observed behavior: NTP is loaded (app went back) Expected behavior: google.com is loaded (app should go back)
,
Dec 27 2016
This is different from crbug.com/677190 and has a different root cause. crbug.com/677190 caused by 2 different issues (one is GetIndexForOffset, another is same as root cause for this bug).
,
Feb 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b2517f141263ee8913024a3a96cd909423ad7214 commit b2517f141263ee8913024a3a96cd909423ad7214 Author: eugenebut <eugenebut@chromium.org> Date: Tue Feb 14 03:21:43 2017 Fixed navigation for displayed SSL interstitials. Navigation did not work for interstitials for 2 reasons which are fixed in this CL: 1.) |WebInterstitialImpl::DontProceed| should not reload. Reloading stops pending navigation and reloads last committed item, which is not always correct target item. On other platforms InterstitialPageImpl::DontProceed does not reload. 2.) |clearTransientContentView| removes pending item, so it should be called before pending navigation is initiated. So next |clearTransientContentView| call will be no-op. This CL changes the behavior, so when user taps "BACK TO SAFETY" the page is not reloaded. This is fine, but in order to update omnibox URL, tab should subscribe to webStateDidDismissInterstitial: callback and update the toolbar. BUG= 677193 , 677190 , 688009 Review-Url: https://codereview.chromium.org/2687343003 Cr-Commit-Position: refs/heads/master@{#450220} [modify] https://crrev.com/b2517f141263ee8913024a3a96cd909423ad7214/ios/chrome/browser/tabs/tab.mm [modify] https://crrev.com/b2517f141263ee8913024a3a96cd909423ad7214/ios/web/interstitials/web_interstitial_impl.mm [modify] https://crrev.com/b2517f141263ee8913024a3a96cd909423ad7214/ios/web/web_state/ui/crw_web_controller.mm
,
Feb 14 2017
,
Feb 15 2017
Verified this on M58.0.3013.0 canary After step#7 from the original steps to reproduce, i see expired.badssl.com is loaded and security interstitial is displayed. But the expected behavior states that google.com should be loaded. @eugenebut can you confirm what is the expected behavior here?
,
Feb 15 2017
I remember that forward button did not work correctly in M47 when current page was SSL interstitial. I think |Step 4| is redundant in STR, but I don't have the old build to confirm that.
,
Feb 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9674693c37dc34ad55e2f21f19050ed2938d6d1c commit 9674693c37dc34ad55e2f21f19050ed2938d6d1c Author: eugenebut <eugenebut@chromium.org> Date: Thu Feb 16 01:36:27 2017 Fixed navigation for displayed SSL interstitials. Navigation did not work for interstitials for 2 reasons which are fixed in this CL: 1.) |WebInterstitialImpl::DontProceed| should not reload. Reloading stops pending navigation and reloads last committed item, which is not always correct target item. On other platforms InterstitialPageImpl::DontProceed does not reload. 2.) |clearTransientContentView| removes pending item, so it should be called before pending navigation is initiated. So next |clearTransientContentView| call will be no-op. This CL changes the behavior, so when user taps "BACK TO SAFETY" the page is not reloaded. This is fine, but in order to update omnibox URL, tab should subscribe to webStateDidDismissInterstitial: callback and update the toolbar. BUG= 677193 , 677190 , 688009 TBR=kkhorimoto@chromium.org NOTRY=true NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2687343003 Cr-Commit-Position: refs/heads/master@{#450220} (cherry picked from commit b2517f141263ee8913024a3a96cd909423ad7214) Review-Url: https://codereview.chromium.org/2694623013 Cr-Commit-Position: refs/branch-heads/2987@{#537} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/9674693c37dc34ad55e2f21f19050ed2938d6d1c/ios/chrome/browser/tabs/tab.mm [modify] https://crrev.com/9674693c37dc34ad55e2f21f19050ed2938d6d1c/ios/web/interstitials/web_interstitial_impl.mm [modify] https://crrev.com/9674693c37dc34ad55e2f21f19050ed2938d6d1c/ios/web/web_state/ui/crw_web_controller.mm
,
Feb 21 2017
,
Feb 21 2017
Verified as per the bug #1 after taking to Eugene.Looks good. Tested on M58.0.3019.0 Canary on iPhone 6+ iOS 10.2 , iPad Pro iOS 10.1.1
,
Feb 22 2017
Verified on chrome beta version 57.0.2987.72 on iPhone 7 plus with iOS 10.2.1, iPad Air 10.2.1 following steps mentioned in comment #0. google.com is displayed at step #8. Looks good. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 Deleted