Got time to run tools/bisect-builds.py?

thestig@: This is the regression range https://chromium.googlesource.com/chromium/src/+log/bd745c89802067c732b9858960cb4834e4b6b10d..69b27e85cdf52da2d0be4f011b92b77c461e12ab

May be https://codereview.chromium.org/1895323002 is the culprit?

What do you think?

Possible.

And thanks for bisecting.

bokan@ can you please take the first attempt at this. With the bisect range your change is highly suspect.

Seems likely to be me. I'll take a look.

I've confirmed it's caused by my patch but I'm not sure why. Everything seems to be flowing through the system correctly but we're not overscrolling because the wheel events have no phase data and I don't know how that could be connected to my patch. I'll keep digging but I'm out for the next few days so a fix isn't imminent.

I think I've traded down the problem, this was broken originally by my patch in https://codereview.chromium.org/1895323002, the problem there was that a position: fixed element has the document *node* (not documentElement) as it's containing block. When we try to scroll that, we wouldn't execute scroll customization callbacks which is how we do overscroll. This has since been fixed, but it looks like the overscrolling logic broke in another way in the interm.

It looks to me like the wheel events sent to the PDF embedder process don't have the appropriate phase info set. This makes the overscroll controller drop them rather than process as overscroll. I'll try to get a fix in this week.

I've narrowed down the problem:

When the embedder handles the initial wheel event (EventHandler::handleWheelEvent), it dispatches a DOM WheelEvent to the page. It does this by creating a blink::WheelEvent from the PlatformWheelEvent, losing Phase information. When the plugin element handles the WheelEvent, it converts it into a WebInputEvent via WebMouseWheelEventBuilder (in BrowserPlugin::handleInputEvent) and sends it to the Guest process but because WheelEvent has no phase information, the phase and momentumPhase information is lost. This then propagates the NULL phase fields into the GestureScrollBegin/Update/End gestures that we synthesize for the wheel event on the browser side. The InputScrollElasticityController, the class responsible for the overscroll effect on Mac, then can't do anything with the incoming phaseless events.

Assigning to wjmaclean@ since he's done some recent work with gesture routing between processes and might know what's changed. Adding phase fields to the blink::WheelEvent class and propagating them to the guest seems to fix this issue. I'm not sure how it used to work though, perhaps it broke with some of the recent OOPIF work. Let me know if you need more details.

A lot of stuff with MouseWheel events changed recently with (1) changes to RenderWidgetHostInputEventRouter's handling of Touchpad gestures, and with the advent of GesturePinch for trackpad (although I think that's just CrOS). Not sure where this went off the rails, but it looks like it might be just a simple to add the necessary plumbing and move the information forward (since converting from WebInputEvent to PlatformEvent to blink::*Event -> WebInputEvent without preserving *all* the information has always been a bit of a bug factory ...).

I'll try and put a CL together in the next few days.
 

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

commit 0e5881c9fb0e35da5c5854ccc1056ffda357a58a
Author: wjmaclean <wjmaclean@chromium.org>
Date: Thu Aug 11 00:27:30 2016

Plumb phase/momentum for WheelEvent.

A simple patch to plumb the MacOS rubber-banding parameters from
PlatformWheelEvent to WheelEvent, and from WheelEvent to
WebMouseWheelEvent.
This CL also removes the canRubberbandX pathways, as they are not used.

BUG= 628742 

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

[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.h
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/content/browser/renderer_host/input/web_input_event_builders_mac.h
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/content/browser/renderer_host/input/web_input_event_builders_mac.mm
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/content/common/input/web_input_event_traits.cc
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/content/public/browser/render_widget_host_view_mac_delegate.h
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/third_party/WebKit/Source/core/events/WheelEvent.cpp
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/third_party/WebKit/Source/core/events/WheelEvent.h
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/third_party/WebKit/Source/platform/PlatformWheelEvent.h
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/third_party/WebKit/Source/web/AssertMatchingEnums.cpp
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/third_party/WebKit/Source/web/WebInputEventConversion.cpp
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/third_party/WebKit/Source/web/tests/WebInputEventConversionTest.cpp
[modify] https://crrev.com/0e5881c9fb0e35da5c5854ccc1056ffda357a58a/third_party/WebKit/public/platform/WebInputEvent.h

Coming back to this: the CL landed above seems to partially re-enable rubberbanding, in that the first rubberband overscroll effect (after loading the document) seems to work ok, but after that the effect fails. Reloading the documents resets so the next overscroll works, but subsequent ones fail.

I'm looking into this to see what happens.

bokan@ - When you re-tested after your experiment with restoring the phase information, did you retest on a PDF, or just with a regular webpage?

This definitely appears to have a generic WebView aspect, as it also affects the signin page.

I believe it was with a PDF.

wjmaclean@ what is the status of this regression?

I have a solution for it, just need to find the time to get a CL up ... hopefully within the coming week.

Sounds like this affects OOPIFs as well, but feel free to correct me if I'm mistaken.

wjmaclean@ what is the status of this issue?

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

commit bf5beebbd5dea348bda215061cea04289d86e9f1
Author: mcnee <mcnee@chromium.org>
Date: Mon May 15 16:47:20 2017

Don't drop mouse wheel events with phase ending information in RWHVCF.

Currently, RenderWidgetHostViewChildFrame::ProcessMouseWheelEvent drops
mouse wheel events that have deltas of 0. However, we may see mouse
wheel events carrying phase ending information that have deltas of 0.
When these are dropped, MouseWheelEventQueue is not informed that
the user's gesture has ended. So when it sees subsequent wheel events,
it considers them all part of the same gesture.

By allowing events with phase ending information through,
MouseWheelEventQueue is able to properly start and end gesture scrolls.

BUG= 694393 ,  628742 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/bf5beebbd5dea348bda215061cea04289d86e9f1/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/bf5beebbd5dea348bda215061cea04289d86e9f1/content/browser/site_per_process_mac_browsertest.mm

As of my CL, rubber-banding is fixed for PDF (and GuestViews in general), however it is still broken for OOPIFs.

Assigning over to mcnee@ who's been working on this for status update.

Yeah, so I've been looking at the overscroll navigation with OOPIFs, but not specifically at this rubber banding effect.

The good news is that when scroll latching (crbug.com/526463) is enabled, the rubber banding with the cursor inside an OOPIF works.

Without latching, it's still broken. I'll see if we can do something for this case.

The issue is due to how GestureScrollBegins are bubbled with the non-latching behaviour. The original GSB is dropped in the child and upon receiving a bubbled GestureScrollUpdate, RenderWidgetHostInputEventRouter creates a new GSB (RenderWidgetHostInputEventRouter::SendGestureScrollBegin). This new GSB does not have the phase information expected by the InputScrollElasticityController.

Given that we've lived with this for over a year already, we might as well just wait until latching ships. We're hoping to do that this quarter.

sahel@ - Can you please verify that scroll latching will fix this? Is there anything else we need to do to make sure we can close this bug on M-65?

I couldn't reproduce the bug with neither oopif nor pdf viewer. I tried it both with latching disabled and enabled and Chromium 64.0.3272.0. I remember seeing the regression before, maybe this is a recent fix. Can anyone else also confirm that the regression doesn't exist anymore?

I just tried this again (on a checkout that's about a week old) and the bug still happens when latching is disabled. I can update and double check.

Yep, this bug still happens when latching is disabled.

So long as it is resolved when latching is enabled, I think we're fine.

Enabling the wheel scroll latching by default fixes the issue:
https://chromium-review.googlesource.com/857341