Only handle mouse left button for Scrollbar

This issue is caused by:

1. Press on any mouse button will update the last_scrollbar_under_mouse_ in
   EventHandler. last_scrollbar_under_mouse_ will set to null and call
   Scrollbar::MouseExited() when mouse middle click out of scrollbar.
2. Release any mouse button will check last_scrollbar_under_mouse_ for cleanup
   the mouse press state on Scrollbar.

Because mouse middle button press happens before any mouse button release,
last_scrollbar_under_mouse_ is null then release mouse button will not clear
the scrollbar press state.

There is another related issue here. We can drag the scrollbar by pressing the
middle button.

The root cause of the bug is not checking for the left mouse button before
updating last_scrollbar_under_mouse_ and calling Scrollbar::MouseDown.

In this patch, we only handle mouse left button for Scrollbar.

Bug: 721303
Change-Id: I714eca2bbdd9dd1628e596dc6b9119bb5de85cc3
Reviewed-on: https://chromium-review.googlesource.com/1048389
Commit-Queue: Jianpeng Chao <chaopeng@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558172}

content/test/test_blink_web_unit_test_support.cc

@@ -116,6 +116,26 @@ class WebURLLoaderFactoryWithMock : public blink::WebURLLoaderFactory {
   DISALLOW_COPY_AND_ASSIGN(WebURLLoaderFactoryWithMock);
 };
 
+class MockWebScrollbarBehavior : public blink::WebScrollbarBehavior {
+ public:
+  MockWebScrollbarBehavior() {}
+  ~MockWebScrollbarBehavior() override {}
+
+  bool ShouldCenterOnThumb(blink::WebPointerProperties::Button mouseButton,
+                           bool shiftKeyPressed,
+                           bool altKeyPressed) override {
+    return false;
+  }
+  bool ShouldSnapBackToDragOrigin(const blink::WebPoint& eventPoint,
+                                  const blink::WebRect& scrollbarRect,
+                                  bool isHorizontal) override {
+    return false;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockWebScrollbarBehavior);
+};
+
 #if defined(V8_USE_EXTERNAL_STARTUP_DATA)
 #if defined(USE_V8_CONTEXT_SNAPSHOT)
 constexpr gin::V8Initializer::V8SnapshotFileType kSnapshotType =
@@ -204,6 +224,8 @@ TestBlinkWebUnitTestSupport::TestBlinkWebUnitTestSupport()
   // Test shell always exposes the GC.
   std::string flags("--expose-gc");
   v8::V8::SetFlagsFromString(flags.c_str(), static_cast<int>(flags.size()));
+
+  web_scrollbar_behavior_ = std::make_unique<MockWebScrollbarBehavior>();
 }
 
 TestBlinkWebUnitTestSupport::~TestBlinkWebUnitTestSupport() {
@@ -381,4 +403,8 @@ void TestBlinkWebUnitTestSupport::BindClipboardHost(
       blink::mojom::ClipboardHostRequest(std::move(handle)));
 }
 
+blink::WebScrollbarBehavior* TestBlinkWebUnitTestSupport::ScrollbarBehavior() {
+  return web_scrollbar_behavior_.get();
+}
+
 }  // namespace content

content/test/test_blink_web_unit_test_support.h

@@ -14,6 +14,7 @@
 #include "build/build_config.h"
 #include "content/child/blink_platform_impl.h"
 #include "content/test/mock_webblob_registry_impl.h"
+#include "third_party/blink/public/platform/web_scrollbar_behavior.h"
 #include "third_party/blink/public/platform/web_url_loader_mock_factory.h"
 
 namespace blink {
@@ -70,6 +71,8 @@ class TestBlinkWebUnitTestSupport : public BlinkPlatformImpl {
   service_manager::Connector* GetConnector() override;
   blink::InterfaceProvider* GetInterfaceProvider() override;
 
+  blink::WebScrollbarBehavior* ScrollbarBehavior() override;
+
  private:
   void BindClipboardHost(mojo::ScopedMessagePipeHandle handle);
 
@@ -82,6 +85,7 @@ class TestBlinkWebUnitTestSupport : public BlinkPlatformImpl {
   std::unique_ptr<blink::scheduler::WebMainThreadScheduler>
       main_thread_scheduler_;
   std::unique_ptr<blink::WebThread> web_thread_;
+  std::unique_ptr<blink::WebScrollbarBehavior> web_scrollbar_behavior_;
 
   base::WeakPtrFactory<TestBlinkWebUnitTestSupport> weak_factory_;
 

third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-mousedown-move-mouseup-expected.txt

 This is a scrollable div.
 
-PASS events['scrollme'].length is 4
+PASS events['scrollme'].length is 2
 PASS events['notscrollme'].length is 0
 PASS events['scrollme'][0].type is "mousedown"
 PASS events['scrollme'][0].which is 1
 PASS events['scrollme'][1].type is "mouseup"
 PASS events['scrollme'][1].which is 1
-PASS events['scrollme'][2].type is "mousedown"
-PASS events['scrollme'][2].which is 2
-PASS events['scrollme'][3].type is "mouseup"
-PASS events['scrollme'][3].which is 2
 PASS successfullyParsed is true
 
 TEST COMPLETE

third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-mousedown-move-mouseup.html

@@ -19,16 +19,12 @@
         }
 
         function finish() {
-            shouldBe("events['scrollme'].length", "4");
+            shouldBe("events['scrollme'].length", "2");
             shouldBe("events['notscrollme'].length", "0");
             shouldBeEqualToString("events['scrollme'][0].type", "mousedown");
             shouldBe("events['scrollme'][0].which", "1");
             shouldBeEqualToString("events['scrollme'][1].type", "mouseup");
             shouldBe("events['scrollme'][1].which", "1");
-            shouldBeEqualToString("events['scrollme'][2].type", "mousedown");
-            shouldBe("events['scrollme'][2].which", "2");
-            shouldBeEqualToString("events['scrollme'][3].type", "mouseup");
-            shouldBe("events['scrollme'][3].which", "2");
             finishJSTest();
         }
 
@@ -43,10 +39,6 @@
                 eventSender.mouseDown();
                 eventSender.mouseMoveTo(d2.offsetLeft + d2.offsetWidth - 4, d2.offsetTop + 4);
                 eventSender.mouseUp();
-                eventSender.mouseMoveTo(d1.offsetLeft + d1.offsetWidth - 4, d1.offsetTop + 4);
-                eventSender.mouseDown(1);
-                eventSender.mouseMoveTo(d2.offsetLeft + d2.offsetWidth - 4, d2.offsetTop + 4);
-                eventSender.mouseUp(1);
                 finish();
             } else
                 debug('This test requires eventSender. Click the scrollbar to play manually.');

third_party/blink/renderer/core/input/event_handler.cc

@@ -967,7 +967,9 @@ WebInputEventResult EventHandler::HandleMouseReleaseEvent(
         EventTypeNames::mouseup, mouse_event);
   }
 
-  if (last_scrollbar_under_mouse_) {
+  // Only relase scrollbar when left button up.
+  if (mouse_event.button == WebPointerProperties::Button::kLeft &&
+      last_scrollbar_under_mouse_) {
     mouse_event_manager_->InvalidateClick();
     last_scrollbar_under_mouse_->MouseUp(mouse_event);
     return DispatchMousePointerEvent(
@@ -2065,6 +2067,10 @@ void EventHandler::CapsLockStateMayHaveChanged() {
 
 bool EventHandler::PassMousePressEventToScrollbar(
     MouseEventWithHitTestResults& mev) {
+  // Only handle mouse left button as press.
+  if (mev.Event().button != WebPointerProperties::Button::kLeft)
+    return false;
+
   Scrollbar* scrollbar = mev.GetScrollbar();
   UpdateLastScrollbarUnderMouse(scrollbar, true);
 

third_party/blink/renderer/core/layout/scrollbars_test.cc

@@ -74,6 +74,24 @@ class ScrollbarsTest : public testing::WithParamInterface<bool>,
     GetEventHandler().HandleMouseReleaseEvent(event);
   }
 
+  void HandleMouseMiddlePressEvent(int x, int y) {
+    WebMouseEvent event(
+        WebInputEvent::kMouseDown, WebFloatPoint(x, y), WebFloatPoint(x, y),
+        WebPointerProperties::Button::kMiddle, 0,
+        WebInputEvent::Modifiers::kMiddleButtonDown, CurrentTimeTicks());
+    event.SetFrameScale(1);
+    GetEventHandler().HandleMousePressEvent(event);
+  }
+
+  void HandleMouseMiddleReleaseEvent(int x, int y) {
+    WebMouseEvent event(
+        WebInputEvent::kMouseUp, WebFloatPoint(x, y), WebFloatPoint(x, y),
+        WebPointerProperties::Button::kMiddle, 0,
+        WebInputEvent::Modifiers::kMiddleButtonDown, CurrentTimeTicks());
+    event.SetFrameScale(1);
+    GetEventHandler().HandleMouseReleaseEvent(event);
+  }
+
   void HandleMouseLeaveEvent() {
     WebMouseEvent event(
         WebInputEvent::kMouseMove, WebFloatPoint(1, 1), WebFloatPoint(1, 1),
@@ -1968,6 +1986,92 @@ TEST_P(ScrollbarsTest, OverlayScrollbarHitTest) {
   EXPECT_FALSE(hit_test_result.GetScrollbar());
 }
 
+TEST_P(ScrollbarsTest, NotAllowMiddleButtonPressOnScrollbar) {
+  ScopedOverlayScrollbarsForTest overlay_scrollbars(false);
+  WebView().Resize(WebSize(200, 200));
+  SimRequest request("https://example.com/test.html", "text/html");
+  LoadURL("https://example.com/test.html");
+  request.Complete(R"HTML(
+    <!DOCTYPE html>
+    <style>
+    #big {
+      height: 800px;
+    }
+    </style>
+    <div id='big'>
+    </div>
+  )HTML");
+  Compositor().BeginFrame();
+
+  ScrollableArea* scrollable_area =
+      WebView().MainFrameImpl()->GetFrameView()->LayoutViewportScrollableArea();
+
+  Scrollbar* scrollbar = scrollable_area->VerticalScrollbar();
+  ASSERT_TRUE(scrollbar);
+  ASSERT_TRUE(scrollbar->Enabled());
+
+  // Not allow press scrollbar with middle button.
+  HandleMouseMoveEvent(195, 5);
+  HandleMouseMiddlePressEvent(195, 5);
+  EXPECT_EQ(scrollbar->PressedPart(), ScrollbarPart::kNoPart);
+  HandleMouseMiddleReleaseEvent(195, 5);
+}
+
+// Ensure Scrollbar not release press by middle click.
+TEST_P(ScrollbarsTest, MiddleClickShouldNotAffectScrollbarPress) {
+  ScopedOverlayScrollbarsForTest overlay_scrollbars(false);
+  WebView().Resize(WebSize(200, 200));
+  SimRequest request("https://example.com/test.html", "text/html");
+  LoadURL("https://example.com/test.html");
+  request.Complete(R"HTML(
+    <!DOCTYPE html>
+    <style>
+    #big {
+      height: 800px;
+    }
+    </style>
+    <div id='big'>
+    </div>
+  )HTML");
+  Compositor().BeginFrame();
+
+  ScrollableArea* scrollable_area =
+      WebView().MainFrameImpl()->GetFrameView()->LayoutViewportScrollableArea();
+
+  Scrollbar* scrollbar = scrollable_area->VerticalScrollbar();
+  ASSERT_TRUE(scrollbar);
+  ASSERT_TRUE(scrollbar->Enabled());
+
+  // Press on scrollbar then move mouse out of scrollbar and middle click
+  // should not release the press state. Then relase mouse left button should
+  // release the scrollbar press state.
+
+  // Move mouse to thumb.
+  HandleMouseMoveEvent(195, 5);
+  HandleMousePressEvent(195, 5);
+  EXPECT_EQ(scrollbar->PressedPart(), ScrollbarPart::kThumbPart);
+
+  // Move mouse out of scrollbar with press.
+  WebMouseEvent event(WebInputEvent::kMouseMove, WebFloatPoint(5, 5),
+                      WebFloatPoint(5, 5), WebPointerProperties::Button::kLeft,
+                      0, WebInputEvent::Modifiers::kLeftButtonDown,
+                      CurrentTimeTicks());
+  event.SetFrameScale(1);
+  GetEventHandler().HandleMouseLeaveEvent(event);
+  EXPECT_EQ(scrollbar->PressedPart(), ScrollbarPart::kThumbPart);
+
+  // Middle click should not release scrollbar press state.
+  HandleMouseMiddlePressEvent(5, 5);
+  EXPECT_EQ(scrollbar->PressedPart(), ScrollbarPart::kThumbPart);
+
+  HandleMouseMiddleReleaseEvent(5, 5);
+  EXPECT_EQ(scrollbar->PressedPart(), ScrollbarPart::kThumbPart);
+
+  // Relase mouse left button should release scrollbar press state.
+  HandleMouseReleaseEvent(5, 5);
+  EXPECT_EQ(scrollbar->PressedPart(), ScrollbarPart::kNoPart);
+}
+
 class ScrollbarTrackMarginsTest : public ScrollbarsTest {
  public:
   void PrepareTest(const String& track_style) {