New issue
Advanced search Search tips

Issue 677190 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Go back does not work for interstitials that are in the middle of navigation stack

Project Member Reported by eugene...@chromium.org, Dec 27 2016

Issue description

App Version (from "Chrome Settings > About Chrome"): TOT
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.) Turn off Airplane Mode
5.) Go Back
6.) Go Back

Observed behavior: 
google.com is loaded (app went forward)

Expected behavior: 
NTP is loaded (app should go back)

 
Description: Show this description
Owner: ----
Status: Available (was: Started)
GetIndexForOffset fix is here: https://codereview.chromium.org/2600263002/
The bug is still reproducible even after GetIndexForOffset fix.
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f276d123db9ccd82569be7cb8658fde3999ae59

commit 8f276d123db9ccd82569be7cb8658fde3999ae59
Author: eugenebut <eugenebut@chromium.org>
Date: Tue Dec 27 23:44:44 2016

[ios] Fixed GetIndexForOffset for pending transient items.

GetIndexForOffset() should decrease resulting index by 1 for pending
transient items to allow going back from interstitials that were added
to the middle of the stack.

BUG= 677190 

Review-Url: https://codereview.chromium.org/2600263002
Cr-Commit-Position: refs/heads/master@{#440808}

[modify] https://crrev.com/8f276d123db9ccd82569be7cb8658fde3999ae59/ios/web/navigation/navigation_manager_impl.mm
[modify] https://crrev.com/8f276d123db9ccd82569be7cb8658fde3999ae59/ios/web/navigation/navigation_manager_impl_unittest.mm

Project Member

Comment 4 by bugdroid1@chromium.org, 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

Owner: eugene...@chromium.org
Status: Fixed (was: Available)
Status: Verified (was: Fixed)
NTP is loaded correctly after step#6 form original steps to reproduce.
Verified on M58.0.3013.0 canary
Device: iPhone7, iOS: 10.3
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 16 2017

Labels: merge-merged-2987
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

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.  NTP is displayed after step #6.  Looks good.

Sign in to add a comment