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}diff --git a/content/test/test_blink_web_unit_test_support.cc b/content/test/test_blink_web_unit_test_support.cc
index 1ef7a0f..5b474d9 100644
--- a/content/test/test_blink_web_unit_test_support.cc
+++ b/content/test/test_blink_web_unit_test_support.cc
@@ -116,6 +116,26 @@
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 @@
// 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 @@
blink::mojom::ClipboardHostRequest(std::move(handle)));
}
+blink::WebScrollbarBehavior* TestBlinkWebUnitTestSupport::ScrollbarBehavior() {
+ return web_scrollbar_behavior_.get();
+}
+
} // namespace content
diff --git a/content/test/test_blink_web_unit_test_support.h b/content/test/test_blink_web_unit_test_support.h
index 8b8b97a..3e9fdf2 100644
--- a/content/test/test_blink_web_unit_test_support.h
+++ b/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 @@
service_manager::Connector* GetConnector() override;
blink::InterfaceProvider* GetInterfaceProvider() override;
+ blink::WebScrollbarBehavior* ScrollbarBehavior() override;
+
private:
void BindClipboardHost(mojo::ScopedMessagePipeHandle handle);
@@ -82,6 +85,7 @@
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_;
diff --git a/third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-mousedown-move-mouseup-expected.txt b/third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-mousedown-move-mouseup-expected.txt
index 355659c..bf57267 100644
--- a/third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-mousedown-move-mouseup-expected.txt
+++ b/third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-mousedown-move-mouseup-expected.txt
@@ -1,15 +1,11 @@
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
diff --git a/third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-mousedown-move-mouseup.html b/third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-mousedown-move-mouseup.html
index bd2b7ac..5f9538f 100644
--- a/third_party/WebKit/LayoutTests/fast/scrolling/scrollbar-mousedown-move-mouseup.html
+++ b/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.');
diff --git a/third_party/blink/renderer/core/input/event_handler.cc b/third_party/blink/renderer/core/input/event_handler.cc
index 2069f52..5ee7cf61 100644
--- a/third_party/blink/renderer/core/input/event_handler.cc
+++ b/third_party/blink/renderer/core/input/event_handler.cc
@@ -967,7 +967,9 @@
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 @@
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);
diff --git a/third_party/blink/renderer/core/layout/scrollbars_test.cc b/third_party/blink/renderer/core/layout/scrollbars_test.cc
index 511f91b..9acae709 100644
--- a/third_party/blink/renderer/core/layout/scrollbars_test.cc
+++ b/third_party/blink/renderer/core/layout/scrollbars_test.cc
@@ -74,6 +74,24 @@
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 @@
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) {
