Do you think this is still a bug once it is possible to exit fullscreen by tapping on the contracted top toolbar?

We reset the fullscreen state upon a main frame navigation, so this is occurring because the navigation is occurring in an iframe.  I'm not sure if there's any way to get a signal for these navigations, so we might have to rely on the gesture Gauthier mentioned in Comment 1.

cc. Michael + Eugene to see if intercepting this navigation is possible.

WKWebView calls decidePolicyForNavigationResponse: for iframes and if Chrome responds with WKNavigationActionPolicyAllow then navigation happens. Also WKWebView.loading sometimes flips to YES for iframes navigation. Kurt, do you think these signals can be useful for AMP issue? If yes, we can later figure out how to pipe those signals to FullScreen controller.

Piping WKWebView.loading through to fullscreen would be easiest since there's already a WSO feeing info to the model.  You mentioned that this only occurs for *some* iframe navigations though; do you think this is reliable enough to use as a solution here?  If not, then I'm guessing we'd have to add a new WSO callback to notify fullscreen, which seems like a more significant change.

We should also keep in mind that it doesn't seem like we have enough information about the source of the navigation response or loading state change, so using this to show the toolbar might be a little aggressive since it will be shown for any iframe load, not just AMP pages.

I don't know how reliable WKWebView.loading is. Danyao recently told me that WKWebView.loading is changed for iframes navigations. Can we test WKWebView.loading with AMP?

Do we want to special case AMP? If so, then we can use WebViewWebStatePolicyDecider::ShouldAllowResponse and exit fullscreen if response came from AMP and iframe. Special casing AMP is terrible, but that's what AMP architecture forces us to do :(

> Can we test WKWebView.loading with AMP?
Any results from this ? 

AMP pages are getting more and more popular as you can see so it would be good to address this in a clean way. 

Question: Pull-to-refresh doesn't work in AMP, is that related to this issue too?

Some exploration shows that WKWebView.loading is updated pretty consistently for navigations to AMP pages.  I've created crrev.com/c/1048635 to show the toolbar for this signal.  This brings functionality back to the behavior before the UI refresh, where the toolbar is shown (but not scrolled away) for AMP pages.

 Issue 835033  can be used to track actually hooking AMP pages into fullscreen.  It's currently marked as WontFix since our behavior matches that of Safari and it would be a significant change to rewrite fullscreen to hook into the pan gesture rather than the content offset.  However, if this is a high enough priority, I can start looking into how that would work.

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

commit f16dcd5e5d8c2f72d0f86df310130abb7e5581d6
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Tue May 08 19:02:09 2018

[iOS] Show toolbar for load events when UI refresh flag is enabled.

In the legacy UI, fullscren was disabled when the WKWebView was loading
so that the URL of the page is always visible.  After the UI refresh,
the URL is always visible, so fullscreen does not need to be disabled.
However, the toolbar should still be shown (but not disabled) upon load
events to ensure that navigation to AMP pages allows access to the
navigation controls on these pages.

Bug:  837254 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Icc9e54cd6972b481c9041fb547f4ab5e8ba40b81
Reviewed-on: https://chromium-review.googlesource.com/1048635
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556908}
[modify] https://crrev.com/f16dcd5e5d8c2f72d0f86df310130abb7e5581d6/ios/chrome/browser/ui/fullscreen/fullscreen_web_state_observer.mm

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

commit 4c1a71cd708b375d17049978e321edba6cb6f473
Author: Rohit Rao <rohitrao@chromium.org>
Date: Thu May 10 17:39:21 2018

Revert "[iOS] Show toolbar for load events when UI refresh flag is enabled."

This reverts commit f16dcd5e5d8c2f72d0f86df310130abb7e5581d6.

Reason for revert: Breaks Fullscreen EGtests when UIRefresh is enabled.

Original change's description:
> [iOS] Show toolbar for load events when UI refresh flag is enabled.
> 
> In the legacy UI, fullscren was disabled when the WKWebView was loading
> so that the URL of the page is always visible.  After the UI refresh,
> the URL is always visible, so fullscreen does not need to be disabled.
> However, the toolbar should still be shown (but not disabled) upon load
> events to ensure that navigation to AMP pages allows access to the
> navigation controls on these pages.
> 
> Bug:  837254 
> Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
> Change-Id: Icc9e54cd6972b481c9041fb547f4ab5e8ba40b81
> Reviewed-on: https://chromium-review.googlesource.com/1048635
> Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
> Reviewed-by: Justin Cohen <justincohen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#556908}

TBR=justincohen@chromium.org,kkhorimoto@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  837254 
Change-Id: Iee7aa1a1c157015ba238fa18dbfde82e672a8b14
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Reviewed-on: https://chromium-review.googlesource.com/1054047
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557572}
[modify] https://crrev.com/4c1a71cd708b375d17049978e321edba6cb6f473/ios/chrome/browser/ui/fullscreen/fullscreen_web_state_observer.mm

Issue 853564 has been merged into this issue.

Hey Kurt, 

What's the latest here ? 

If this will be fixed in the next couple weeks, will be good to know since we'll wait until it's fixed before sending Bijou to Google-wide dogfood. If it'll take longer than that, will send the dogfood email and list this as a known issue.

I've just finished a bunch of reading list work, so I have time to loop back around to fullscreen issues now.  We can definitely get this fixed for M69, so I'll add a blocker label.

Thank you, Kurt! Just wanted to mention also that if we're unable to reliably identify whether a page is AMP one or not, we should just exit out of FullScreen navigation.
AMP pages should not go to fullscreen obviously (current behaviour). 

Gabe: We will probably want to send that email out sooner rather than later, so it's fine to mention this as a known issue IMHO. 

Assigning to Gauthier who's currently looking into it. 

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

commit 2ce40f27646be98dcc4a117063a86d9b8b74804f
Author: Gauthier Ambard <gambard@chromium.org>
Date: Thu Jul 05 08:02:14 2018

[iOS] Show toolbar for load events

This CL makes sure that the toolbar is shown for load events when the
UI Refresh flag is enabled. This is necessary for loading AMP pages from
Google Search Result Page. As the navigation is occuring in the same
document, a special case for this is needed.

Bug:  837254 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I4f9fb5a9f57f2eb1f1f37215ccf9e3d89906c6af
Reviewed-on: https://chromium-review.googlesource.com/1125724
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572732}
[modify] https://crrev.com/2ce40f27646be98dcc4a117063a86d9b8b74804f/ios/chrome/browser/ui/fullscreen/fullscreen_web_state_observer.mm

I have landed a CL very similar to Kurt's. The bots seem fine so I am closing this issue.

Nice, I think this was due to rolling to a new version of EG where the original issue doesn't reproduce.  Thanks for taking this while I was OOO, Gauthier!

Verified on chrome canary version 69.0.3487.0 on iPhone x with iOS 11.4.1 and 11.4, following the steps mentioned in comment#0.  Page does not go in full screen mode.  Looks good.

kkhorimoto@
Just want to confirm that this site keeping the toolbar at the bottom is due to this workaround to the AMP issues, right?
yad2.co.il

A dogfooder called it out

Issue 864989 has been merged into this issue.

Issue 864989 has been filed, which makes it look like our previous solution was incomplete.  It seems that the SRP is doing some caching, so we aren't always getting the DidStartLoading() signal that we were using show the toolbars.  For a more robust solution, we'll probably have to special case some of the iframe logic occurring to load AMP pages.

ghendel@: That's a related issue.  We don't receive scroll events from AMP pages due to WKWebView implementation details, so we aren't able to hide the toolbars for AMP pages.

From my tests when I was looking at this issue, we now have the same behavior as the one in stable. In stable there is no way to get out of the fullscreen state, with UI Refresh it is possible to tap the toolbar.

I would either create a new bug or lower the priority of this one.

I've filed  Issue 866018  as a follow-up to this bug.  Since WKWebView.loading is not a good enough indicator, we'll have to dig into implementation details of AMP in order to find a better heuristic, probably implemented via JavaScript.

[Auto-generated comment by a script] We noticed that this issue is targeted for M-69; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-69 label, otherwise remove Merge-TBD label. Thanks.

Nothing to merge here.

verified on iPhone 6+ iOS 11.4, iPhone 7+ iOS 10.3.3 , iPad Pro 12'9 iOS 11.4.1 on  69.0.3497.50 Beta