Merge to M65: Have GestureScrollBegin ACK consider local child frames.
TBR=creis@chromium.org
Have GestureScrollBegin ACK consider local child frames.
Consider a page with a nested iframe such that the main frame is
navigated to site A, the outer iframe is site B, and the inner iframe
is site B. With Site Isolation, the outer iframe is an OOPIF and the
inner frame is local to said OOPIF.
When attempting to scroll in the inner iframe,
ScrollManager::HandleGestureScrollBegin for the outer frame passes
the event along to its local child, but ignores the return value
which indicates whether the child has a non-empty scroll chain. It goes
on to create a scroll chain for the outer frame and bases the event
ACK on that. Hence, if a local child frame is scrollable in a given
direction, but the OOPIF containing it isn't, then the GSB will be
acknowledged as unconsumed which will cause the browser to bubble
scroll to an ancestor of the OOPIF.
We now use the return value for the local child to inform the GSB
ACK. In order to do this, we also no longer unconditionally add the
document element for a local subframe to the scroll chain.
(cherry picked from commit 612feb71496d3492dadd3d60cb61414d987797be)
Bug: 807683
Change-Id: I4cd76888f8362e14aa660d7727dfcb0d24323a40
Reviewed-on: https://chromium-review.googlesource.com/939522
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#539955}
Reviewed-on: https://chromium-review.googlesource.com/946548
Cr-Commit-Position: refs/branch-heads/3325@{#646}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
index 0565e9d..645960a 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -1983,6 +1983,77 @@
}
}
+// Ensure that the scrollability of a local subframe in an OOPIF is considered
+// when acknowledging GestureScrollBegin events sent to OOPIFs.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, ScrollLocalSubframeInOOPIF) {
+ ui::GestureConfiguration::GetInstance()->set_scroll_debounce_interval_in_ms(
+ 0);
+
+ // This must be tall enough such that the outer iframe is not scrollable.
+ GURL main_url(embedded_test_server()->GetURL(
+ "a.com", "/frame_tree/page_with_tall_positioned_frame.html"));
+ EXPECT_TRUE(NavigateToURL(shell(), main_url));
+
+ // It is safe to obtain the root frame tree node here, as it doesn't change.
+ FrameTreeNode* root = web_contents()->GetFrameTree()->root();
+ ASSERT_EQ(1U, root->child_count());
+
+ FrameTreeNode* parent_iframe_node = root->child_at(0);
+ GURL outer_frame_url(embedded_test_server()->GetURL(
+ "baz.com", "/frame_tree/page_with_positioned_frame.html"));
+ NavigateFrameToURL(parent_iframe_node, outer_frame_url);
+
+ // This must be tall enough such that the inner iframe is scrollable.
+ FrameTreeNode* nested_iframe_node = parent_iframe_node->child_at(0);
+ GURL inner_frame_url(
+ embedded_test_server()->GetURL("baz.com", "/tall_page.html"));
+ NavigateFrameToURL(nested_iframe_node, inner_frame_url);
+
+ ASSERT_EQ(
+ " Site A ------------ proxies for B\n"
+ " +--Site B ------- proxies for A\n"
+ " +--Site B -- proxies for A\n"
+ "Where A = http://a.com/\n"
+ " B = http://baz.com/",
+ DepictFrameTree(root));
+
+ RenderWidgetHostViewBase* rwhv_child = static_cast<RenderWidgetHostViewBase*>(
+ nested_iframe_node->current_frame_host()
+ ->GetRenderWidgetHost()
+ ->GetView());
+
+ WaitForChildFrameSurfaceReady(parent_iframe_node->current_frame_host());
+
+ // When we scroll the inner frame, we should have the GSB be consumed.
+ // The outer iframe not being scrollable should not cause the GSB to go
+ // unconsumed.
+ InputEventAckWaiter ack_observer(
+ parent_iframe_node->current_frame_host()->GetRenderWidgetHost(),
+ base::BindRepeating([](content::InputEventAckSource,
+ content::InputEventAckState state,
+ const blink::WebInputEvent& event) {
+ return event.GetType() == blink::WebGestureEvent::kGestureScrollBegin &&
+ state == content::INPUT_EVENT_ACK_STATE_CONSUMED;
+ }));
+
+ // Wait until renderer's compositor thread is synced. Otherwise the non fast
+ // scrollable regions won't be set when the event arrives.
+ MainThreadFrameObserver observer(rwhv_child->GetRenderWidgetHost());
+ observer.Wait();
+
+ // Now scroll the inner frame downward,
+ blink::WebMouseWheelEvent scroll_event(
+ blink::WebInputEvent::kMouseWheel, blink::WebInputEvent::kNoModifiers,
+ blink::WebInputEvent::kTimeStampForTesting);
+ scroll_event.SetPositionInWidget(90, 110);
+ scroll_event.delta_x = 0.0f;
+ scroll_event.delta_y = -50.0f;
+ scroll_event.phase = blink::WebMouseWheelEvent::kPhaseBegan;
+ scroll_event.has_precise_scrolling_deltas = true;
+ rwhv_child->ProcessMouseWheelEvent(scroll_event, ui::LatencyInfo());
+ ack_observer.Wait();
+}
+
// This test verifies that scrolling an element to view works across OOPIFs.
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, ScrollElementIntoView) {
GURL url_domain_a(
diff --git a/content/test/data/frame_tree/page_with_tall_positioned_frame.html b/content/test/data/frame_tree/page_with_tall_positioned_frame.html
new file mode 100644
index 0000000..10923c8
--- /dev/null
+++ b/content/test/data/frame_tree/page_with_tall_positioned_frame.html
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<style>
+iframe {
+ position:absolute;
+ top: 50px;
+ left: 50px;
+ width: 100px;
+ height: 10000px;
+}
+</style>
+<html>
+<body>
+<iframe src="/cross-site/baz.com/title1.html"></iframe>
+This page contains a positioned cross-origin iframe.
+</body>
+</html>
diff --git a/testing/buildbot/filters/mojo.fyi.viz.content_browsertests.filter b/testing/buildbot/filters/mojo.fyi.viz.content_browsertests.filter
index d161f43..f0d75aee4 100644
--- a/testing/buildbot/filters/mojo.fyi.viz.content_browsertests.filter
+++ b/testing/buildbot/filters/mojo.fyi.viz.content_browsertests.filter
@@ -57,6 +57,7 @@
-SitePerProcessBrowserTest.ScrollBubblingFromNestedOOPIFTest
-SitePerProcessBrowserTest.ScrollBubblingFromOOPIFTest
-SitePerProcessBrowserTest.ScrollEventToOOPIF
+-SitePerProcessBrowserTest.ScrollLocalSubframeInOOPIF
-SitePerProcessBrowserTest.SubframeTouchEventRouting
-SitePerProcessBrowserTest.SurfaceHitTestPointerEventsNone
-SitePerProcessBrowserTest.SurfaceHitTestTest
diff --git a/testing/buildbot/filters/viz.content_browsertests.filter b/testing/buildbot/filters/viz.content_browsertests.filter
index 2aecf220..7fc24ea 100644
--- a/testing/buildbot/filters/viz.content_browsertests.filter
+++ b/testing/buildbot/filters/viz.content_browsertests.filter
@@ -60,6 +60,7 @@
-SitePerProcessBrowserTest.ScrollBubblingFromNestedOOPIFTest
-SitePerProcessBrowserTest.ScrollBubblingFromOOPIFTest
-SitePerProcessBrowserTest.ScrollEventToOOPIF
+-SitePerProcessBrowserTest.ScrollLocalSubframeInOOPIF
-SitePerProcessBrowserTest.SubframeTouchEventRouting
-SitePerProcessBrowserTest.SurfaceHitTestPointerEventsNone
-SitePerProcessBrowserTest.SurfaceHitTestTest
diff --git a/third_party/WebKit/Source/core/input/ScrollManager.cpp b/third_party/WebKit/Source/core/input/ScrollManager.cpp
index a843c09..da0e0b4 100644
--- a/third_party/WebKit/Source/core/input/ScrollManager.cpp
+++ b/third_party/WebKit/Source/core/input/ScrollManager.cpp
@@ -159,12 +159,11 @@
if (IsViewportScrollingElement(current_element) ||
current_element == *(frame_->GetDocument()->documentElement())) {
- if (!frame_->Tree().Parent() || frame_->Tree().Parent()->IsLocalFrame())
+ if (frame_->IsMainFrame())
return true;
- // For oopif the viewport is added to scroll chain only if it can actually
- // consume some delta hints.
- DCHECK(frame_->Tree().Parent()->IsRemoteFrame());
+ // For subframes, the viewport is added to the scroll chain only if it can
+ // actually consume some delta hints.
scrollable_area =
frame_->View() ? frame_->View()->GetScrollableArea() : nullptr;
}
@@ -338,8 +337,8 @@
!scroll_gesture_handling_node_->GetLayoutObject())
return WebInputEventResult::kNotHandled;
- PassScrollGestureEvent(gesture_event,
- scroll_gesture_handling_node_->GetLayoutObject());
+ WebInputEventResult child_result = PassScrollGestureEvent(
+ gesture_event, scroll_gesture_handling_node_->GetLayoutObject());
RecordScrollRelatedMetrics(gesture_event.source_device);
@@ -360,8 +359,12 @@
ScrollState* scroll_state = ScrollState::Create(std::move(scroll_state_data));
RecomputeScrollChain(*scroll_gesture_handling_node_.Get(), *scroll_state,
current_scroll_chain_);
- if (current_scroll_chain_.empty())
- return WebInputEventResult::kNotHandled;
+
+ if (current_scroll_chain_.empty()) {
+ // If a child has a non-empty scroll chain, we need to consider that instead
+ // of simply returning WebInputEventResult::kNotHandled.
+ return child_result;
+ }
CustomizedScroll(*scroll_state);
