Please navigate to below link for log's and video--
go/chrome-androidlogs/825765

Anybody who works on Interstitials know why having the renderer report the control container height/top controls offset rather than dropping the initial update would cause problems?

The renderer is definitely asking for the controls to be hidden upon loading the interstitial page... Which was previously 'erroneously' being ignored incidentally...

I think this might be related to OnFrameMetadataUpdated being called with 0 for top_controls_pix on interstitials, which itself seems to be related to interstitials being non-committed navigations, but overlays, so I'm guessing the renderer doesn't have that information.

Tried to reproduce this from chrome://interstitials, and the interstitials from there get the correct value for top_controls_pix (196 in my nexus 6p case) and they work correctly. As a side note chrome://interstitials itself (which is not an interstitial, just a webui page with links to demo interstitials) works correctly, but if you navigate to an interstitial, and then directly to chrome://interstitials, the touch offset also happens on chrome://interstitials.

Currently interstitials are being refactored into committed navigations (go/committed-interstitials) to solve several bugs that exist due to interstitials being overlays, and this bug seems to be fixed with committed interstitials, however committed interstitials probably won't be ready to launch until 68. 

Sounds like this is a complicated issue... is this something we want me to throw in a dirty hack for in M67? I could special case fixing this to being in VR mode or something... and leave the old broken behavior in place for things like interstitials?

Or are there things other than interstitials that also fall into this path where the controls are set wrong and we should fix this more generally?

From the interstitial end I don't really oppose special casing it out, since it would only be while committed interstitials are done (if you do that, feel free to add a TODO(carlosil) and file a bug detailing that the special case should be removed once committed interstitials launch). However, I don't know if this affects any other web ui in a similar way. 

Since this CL was merged in M66, we need a fix there as well.  Changing milestone to M66.

Is it possible to get a revert?

I should be able to post a trivial change tomorrow that keeps the VR fix, but reverts the breakage for non-VR cases.

A revert would re-introduce the original bug, so I'd really rather avoid that as both bugs are pretty bad here.

Fix is here: https://chromium-review.googlesource.com/c/chromium/src/+/982095

Issue 825760 has been merged into this issue.

 mthiesse@ please land the fix in #11 as soon as possible

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

commit 80c0484b0db211d4b6bec84f41778460c9df14e6
Author: Michael Thiessen <mthiesse@chromium.org>
Date: Tue Mar 27 21:03:37 2018

Make intial top controls offset fix specific to VR (targeting M66).

https://chromium-review.googlesource.com/c/chromium/src/+/961265 fixed
a bug where the browser would drop the initial top controls offset
message if the top controls were initially hidden, leading to a mismatch
in state between the browser and renderer.

That fix broke interstitials which depend on this erroneous mismatch
for the time being. Interstitials are in the process of being fixed,
targeting M68, and this CL should be reverted/undone at that time.

This CL restricts that fix to VR, which needs the fix in place to be
able to launch Chrome in VR.

Bug:  825765 
Change-Id: I7d32d46fb35c66b3aa098817f4fd3a69f9251c1b
Reviewed-on: https://chromium-review.googlesource.com/982095
Reviewed-by: Carlos IL <carlosil@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546255}
[modify] https://crrev.com/80c0484b0db211d4b6bec84f41778460c9df14e6/content/browser/renderer_host/render_widget_host_view_android.cc

 Issue 825881  has been merged into this issue.

Verified on canary.

This bug requires manual review: M66 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Confirming we're seeing same thing in M66.0.3359.46 on Android 8.1 (Pixel 1 and 2, etc.). All page rendering seems to have vertical offset issues and touch input is equally offset making many page unusable.

NOTE: This doesn't "seem" to impact webvr (referring to comment 7 above).

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

commit 70782c147e81908e0a8b2e59f0b7f54b962ffc3e
Author: Michael Thiessen <mthiesse@chromium.org>
Date: Thu Mar 29 01:05:31 2018

Make intial top controls offset fix specific to VR (targeting M66).

https://chromium-review.googlesource.com/c/chromium/src/+/961265 fixed
a bug where the browser would drop the initial top controls offset
message if the top controls were initially hidden, leading to a mismatch
in state between the browser and renderer.

That fix broke interstitials which depend on this erroneous mismatch
for the time being. Interstitials are in the process of being fixed,
targeting M68, and this CL should be reverted/undone at that time.

This CL restricts that fix to VR, which needs the fix in place to be
able to launch Chrome in VR.

TBR=mthiesse@chromium.org

(cherry picked from commit 80c0484b0db211d4b6bec84f41778460c9df14e6)

Bug:  825765 
Change-Id: I7d32d46fb35c66b3aa098817f4fd3a69f9251c1b
Reviewed-on: https://chromium-review.googlesource.com/982095
Reviewed-by: Carlos IL <carlosil@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#546255}
Reviewed-on: https://chromium-review.googlesource.com/985630
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#489}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/70782c147e81908e0a8b2e59f0b7f54b962ffc3e/content/browser/renderer_host/render_widget_host_view_android.cc

google@awe.media, when you say all page rendering, do you mean more than just interstitial pages are broken?

Also, this has nothing to do with WebVR, I was referring to VR browsing mode.

Issue got fixed as per the steps mentioned in the comment#0, verified on latest M66 & M67 builds. ADVANCED link & PROCEED TO WWW>TK>DE(UNSAGE)links working fine. Hence closing this issue. Thanks 

It was all html page rendering - but can confirm it's now fixed in 66.0.3359.67.

Thank you 8)

google@awe.media, is there anything about your setup that's special? I couldn't reproduce seeing this on all pages, and this issue is likely to recur once Carlos fixes the interstitial issue.

 Issue 826053  has been merged into this issue.

Issue seems to be reproduced again. Tapping on the intended position is not working, instead user has to tap below the link to get the required action done.

Steps Followed:
1. Launch Chrome.
2. Navigate to any URL. Eg.: expired.badssl.com
3. "Your connection is not private" warning message is displayed
4. Tried to tap on "ADVANCED" button, but no action takes place. However, if the user taps below the button, intended action takes place.

Chrome versions tested:
69.0.3447.0(Canary), 68.0.3440.8(Dev)

OS:
Android 8.1.0

Android Devices:
Pixel 2

Using the per-revision bisect providing the bisect results,
Good Build - 68.0.3440.0
Bad Build - 69.0.3441.0

Unable to provide the per-revision bisect as all the builds gave Good results. 

@mthiesse:  Could you please take a look into the issue. Let us know if any further information is required.

Updated the latest Logs and screen cast here:
go/chrome-androidlogs/825765

Issue is also observed in latest Dev #68.0.3440.8. Hence, marking as RegressedIn-68.

Thanks!

carlosil, do you know of any changes to interstitials that might have caused this? I don't think I changed anything that could have regressed this.

I couldn't repro on 69.0.3447.0, but I could on 68.0.3440.8.

O_o I'm seeing this on 67.0.3396.70

Whoa weird. From 67.0.3382.0/arm, if I install MonochromeCanary.apk I can't repro, but if I install MonochromeDev.apk I can repro.

Looks like there's something strange going on in just Dev builds that's been happening since at least M67? I can't repro on 67.0.3396.68 Stable or Canary.

Taking a look at this too, no changes from our end to interstitials that I can think of.

From #34, I wonder if this is something that's enabled (via Variations) for Dev but not Canary?

Meant from #33

Okay I can repro the bug in 67.0.3370.0 MonoChromeDev.apk, which predates my CL that regressed this in Canary in 67.0.3371.0 (https://chromiumdash-staging.googleplex.com/commit/da202f037c3a313b6be1a85c9e2789a2d8df8b40).

This means that there was a pre-existing issue with the exact same symptoms on Dev channel, but not stable or canary.

-cmasso, false alarm I think :P

Disabling variations (by setting the variations-server-url to invalid.test) stops the repro on ToT for me, looking into the diff of enabled variations to see if I can pinpoint to one experiment.

Adjusting to Dev channel milestone, please update if needed.

Probably shouldn't be ReleaseBlock-Stable if it affects Dev only?

Sorry, removing. Will keep the RBD for now for tracking purpose.

It seems the feature that causes this to happen is VizHitTestDrawQuad, adding riajiang who owns the experiment.

riajiang: It seems VizHitTestDrawQuad triggers a regression on interstitials where the hit testing is off on Android. Currently interstitials are not navigations, but an overlay, and they report their top controls height as 0 (which worked previously by ignoring the update that happens when an interstitial is first displayed).

This behavior will be changed once the committed interstitials refactor (which turns interstitials into regular navigations) launches, but that is currently blocked and will probably won't launch until at least 68 or 69. I'm not very familiar with the viz code, can you take a look to see if we can do something similar in the viz codepath? Thanks

VizHitTestDrawQuad is in finch trial for 50% users in canary (since April 13), dev (since April 25) and beta (since May 22) so it shouldn't only affect dev channel and seems to be turned on after this issue occurs (based on c#37).

carlosil@, I'm not familiar with the interstitials code, by "which worked previously by ignoring the update that happens when an interstitial is first displayed", do you mean we would be using the correct top controls height after it was first displayed? So we should be hit-testing correctly after we get the right top controls height? Also, sry I'm OOO so don't have a testing device - were you able to repro only with VizHitTestDrawQuad feature turned on on dev?

It looks related to hit testing - if I tap below the "Advanced" button, it works. If I tap on the "Advanced" button, it takes me back to the previous page. If I tap on the "Back on safety" button above the "Advanced" button, nothing happens. Once the advanced section shows, I have to tap a bit below the "Continue" link, too, for it to activate.

So it seems hit testing is off by a few dozen pixels in height.

Re: #45: Thanks for looking at this, I didn't specifically test 67.0.3370.0 again, but in all my tests that failed, the common feature was VizHitTestDrawQuad, then I tested ToT, with variations disabled, and enabling VizHitTestDrawQuad causes this to reproduce while disabling it fixes it (this is true in both channels and in the current Stable), so I'm fairly sure this is the feature causing it.

As for the interstitials hit testing, yeah, with the previous codepath, interstitials hit-testing worked fine, but any calls to OnTopControlsChanged caused it to misbehave, since interstitials for some reason report their top controls height as 0 (which won't be the case anymore once Committed Interstitials launch). 

Forgot to mention in the previous one, since this reproduces on Stable, if you want to see the issue in action and have access to any Android phone, you can test it by turning on the feature and navigating to expired.badssl.com

Thanks for the explanation! I can repro on stable with VizHitTestDrawQuad feature turned on.

We do use GetTopControlsHeight() in RWHVA as the offset from root view to root surface [1] to translate an event location to be in the coord-space of the root surface. For the interstitial case, DoBrowserControlsShrinkBlinkSize() returns false and GetTopControlsHeight() returns 0 (based on InterstitialPageImpl::InterstitialPageRVHDelegateView doesn't override these two functions from RenderViewHostDelegateView) IIUC so that's prob why there's a hit-testing offset. 

Is getting the correct top controls height for interstitials in RWHVA only possible after Committed Interstitials? I didn't see anything in InterstitialPageImpl, not sure if InterstitialPageImpl::frame_tree_ is useful to get the height here...

[1] https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_android.cc?l=1513

 Issue 850773  has been merged into this issue.

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

commit 690c84bda72144941f2e23fcc7b5460bee84e556
Author: Carlos IL <carlosil@chromium.org>
Date: Thu Jun 14 22:11:03 2018

Added overrides in interstitial_page_impl

Added overrides for GetTopControlsHeight, GetBottomControlsHeight, and
DoBrowserControlsShrinkBlinkSize to
InterstitalPageImpl::InterstitialPageRVHDelegateView. The lack of those
overrides was causing hit testing errors on Android for interstitials.

Bug:  825765 
Change-Id: If88506529a8eb3a8cb665edaa6416eeb4e3f8281
Reviewed-on: https://chromium-review.googlesource.com/1101397
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567439}
[modify] https://crrev.com/690c84bda72144941f2e23fcc7b5460bee84e556/content/browser/frame_host/interstitial_page_impl.cc
[modify] https://crrev.com/690c84bda72144941f2e23fcc7b5460bee84e556/content/browser/renderer_host/render_widget_host_view_android.cc

Verified c#52 on Android Canary 69.0.3461.0. Thanks Carlos!
Requesting a merge to M68 to fix hit-testing errors for security interstitials.

This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

I don't think we can merge until issue 853686 is fixed.

Fix for crbug.com/853686 is out for review in crrev.com/c/1105124

Fix for 853686 landed, do you want me to merge-request that one before this one gets merged?

Thanks! Yes that sounds good

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

commit cfae5018ffc2795c0b9e834b289bf888f769f8e2
Author: Carlos IL <carlosil@chromium.org>
Date: Fri Jun 22 01:06:18 2018

Added overrides in interstitial_page_impl

Added overrides for GetTopControlsHeight, GetBottomControlsHeight, and
DoBrowserControlsShrinkBlinkSize to
InterstitalPageImpl::InterstitialPageRVHDelegateView. The lack of those
overrides was causing hit testing errors on Android for interstitials.

TBR=carlosil@chromium.org

(cherry picked from commit 690c84bda72144941f2e23fcc7b5460bee84e556)

Bug:  825765 
Change-Id: If88506529a8eb3a8cb665edaa6416eeb4e3f8281
Reviewed-on: https://chromium-review.googlesource.com/1101397
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#567439}
Reviewed-on: https://chromium-review.googlesource.com/1111380
Reviewed-by: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#482}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/cfae5018ffc2795c0b9e834b289bf888f769f8e2/content/browser/frame_host/interstitial_page_impl.cc
[modify] https://crrev.com/cfae5018ffc2795c0b9e834b289bf888f769f8e2/content/browser/renderer_host/render_widget_host_view_android.cc