SelectionAutoscroll should not happen in user-select:none. (issue 2536083002 by sunyunjia@chromium.org)

[suny...@chromium.org]

Reviewers: bokan
CL: https://codereview.chromium.org/2536083002/

Message:
PTAL, thanks!

Description:
SelectionAutoscroll should not happen in user-select:none.

AutoscrollForSelection is to help users select content outside the current
viewport. However, the current implementation starts the autoscroll whenever
the mouse-drag event reaches the edge of the viewport, regardless of whether
the user is selecting or not. It makes no sense to start the autoscroll when
the element was not selectable at all. This patch allows the
AutoscrollForSelection fire only when the mouse is dragged across a selectable
element.

BUG=588448

Affected files (+35, -1 lines):
A third_party/WebKit/LayoutTests/fast/events/autoscroll-disabled-user-select-none.html
M third_party/WebKit/Source/core/input/MouseEventManager.cpp


Index: third_party/WebKit/LayoutTests/fast/events/autoscroll-disabled-user-select-none.html
diff --git a/third_party/WebKit/LayoutTests/fast/events/autoscroll-disabled-user-select-none.html b/third_party/WebKit/LayoutTests/fast/events/autoscroll-disabled-user-select-none.html
new file mode 100644
index 0000000000000000000000000000000000000000..689cdbec7abf9fb120f1b377319ade4039e10ed7
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fast/events/autoscroll-disabled-user-select-none.html
@@ -0,0 +1,33 @@
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<div id="container" style="height: 400px; overflow: auto;">
+ <div style="-webkit-user-select:none; height: 2000px; background-color: yellow">
+ Hello World! Hello Woorld! Hello Wooorld! Hello Woooorld!<br />
+ Hello World! Hello Woorld! Hello Wooorld! Hello Woooorld!<br />
+ Hello World! Hello Woorld! Hello Wooorld! Hello Woooorld!<br />
+ Hello World! Hello Woorld! Hello Wooorld! Hello Woooorld!<br />
+ Hello World! Hello Woorld! Hello Wooorld! Hello Woooorld!<br />
+ </div>
+</div>
+<script>
+var testSelectNone = async_test("Selection-autoscroll should not be triggered when the selection happens in an element with user-select:none");
+testSelectNone.step(function() {
+ if (!window.eventSender)
+ return;
+ var dragStartX = 50;
+ var dragStartY = 50;
+ var dragEndX = 60;
+ var dragEndY = 400;
+ var container = document.getElementById("container");
+
+ eventSender.dragMode = false;
+ eventSender.mouseMoveTo(dragStartX, dragStartY);
+ eventSender.mouseDown();
+ eventSender.mouseMoveTo(dragEndX, dragEndY);
+
+ requestAnimationFrame(function() {
+ assert_equals(container.scrollTop, 0);
+ testSelectNone.done();
+ });
+});
+</script>
\ No newline at end of file
Index: third_party/WebKit/Source/core/input/MouseEventManager.cpp
diff --git a/third_party/WebKit/Source/core/input/MouseEventManager.cpp b/third_party/WebKit/Source/core/input/MouseEventManager.cpp
index c38ab2fcb8db66b6adc03875a6e1446fdffb617a..aa178a3276abcdc653bc93d979c86176c10dbe98 100644
--- a/third_party/WebKit/Source/core/input/MouseEventManager.cpp
+++ b/third_party/WebKit/Source/core/input/MouseEventManager.cpp
@@ -733,7 +733,8 @@ WebInputEventResult MouseEventManager::handleMouseDraggedEvent(
m_mouseDownMayStartDrag = false;

if (m_mouseDownMayStartAutoscroll &&
- !m_scrollManager->middleClickAutoscrollInProgress()) {
+ !m_scrollManager->middleClickAutoscrollInProgress() &&
+ layoutObject->isSelectable()) {
if (AutoscrollController* controller =
m_scrollManager->autoscrollController()) {
controller->startAutoscrollForSelection(layoutObject);

[bo...@chromium.org]

https://codereview.chromium.org/2536083002/diff/80001/third_party/WebKit/Source/core/input/MouseEventManager.cpp
File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right):

https://codereview.chromium.org/2536083002/diff/80001/third_party/WebKit/Source/core/input/MouseEventManager.cpp#newcode737
third_party/WebKit/Source/core/input/MouseEventManager.cpp:737:
layoutObject->isSelectable()) {
Selection is created/added by the call below to handleMouseDraggedEvent
right? I'm wondering, would it work if we moved this scrolling to happen
after that and then just check whether or not we have a selection? I'm
worried that this check is too simplistic and there might be all sorts
of other cases where we can't select even though the layout object might
be selectable. It would be better if we can just rely on the selection
system to worry about all the edge cases and use its output to decide
whether we should scroll.

https://codereview.chromium.org/2536083002/

[suny...@chromium.org]

PTAL, thanks!

https://codereview.chromium.org/2536083002/diff/80001/third_party/WebKit/Source/core/input/MouseEventManager.cpp
File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right):

https://codereview.chromium.org/2536083002/diff/80001/third_party/WebKit/Source/core/input/MouseEventManager.cpp#newcode737
third_party/WebKit/Source/core/input/MouseEventManager.cpp:737:
layoutObject->isSelectable()) {

On 2016/12/02 17:32:38, bokan wrote:
> Selection is created/added by the call below to
handleMouseDraggedEvent right?
> I'm wondering, would it work if we moved this scrolling to happen
after that and
> then just check whether or not we have a selection? I'm worried that
this check
> is too simplistic and there might be all sorts of other cases where we
can't
> select even though the layout object might be selectable. It would be
better if
> we can just rely on the selection system to worry about all the edge
cases and
> use its output to decide whether we should scroll.

Done.

https://codereview.chromium.org/2536083002/

[bo...@chromium.org]

Question about change to test but otherwise lgtm.


https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html
File
third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html
(right):

https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html#newcode20
third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html:20:
eventSender.mouseMoveTo(40, h);
Why was this change necessary?

https://codereview.chromium.org/2536083002/

[suny...@chromium.org]

https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html
File
third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html
(right):

https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html#newcode20
third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html:20:
eventSender.mouseMoveTo(40, h);

On 2016/12/05 15:39:36, bokan wrote:
> Why was this change necessary?

Autoscroll can only be triggered AFTER some text has been selected.
Directly moving the mouse out of the border can't start autoscroll since
no text has been selected at this moment. We need to select something at
first.

https://codereview.chromium.org/2536083002/

[bo...@chromium.org]

https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html
File
third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html
(right):

https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html#newcode20
third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html:20:
eventSender.mouseMoveTo(40, h);

On 2016/12/05 15:44:39, sunyunjia wrote:
> On 2016/12/05 15:39:36, bokan wrote:
> > Why was this change necessary?
>
> Autoscroll can only be triggered AFTER some text has been selected.
Directly
> moving the mouse out of the border can't start autoscroll since no
text has been
> selected at this moment. We need to select something at first.

Yah, but wouldn't the mouseMove below cause selectioN?

https://codereview.chromium.org/2536083002/

[suny...@chromium.org]

https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html
File
third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html
(right):

https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html#newcode20
third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html:20:
eventSender.mouseMoveTo(40, h);

On 2016/12/05 15:45:54, bokan wrote:
> On 2016/12/05 15:44:39, sunyunjia wrote:
> > On 2016/12/05 15:39:36, bokan wrote:
> > > Why was this change necessary?
> >
> > Autoscroll can only be triggered AFTER some text has been selected.
Directly
> > moving the mouse out of the border can't start autoscroll since no
text has
> been
> > selected at this moment. We need to select something at first.
>
> Yah, but wouldn't the mouseMove below cause selectioN?

I did some more investigation. The autoscroll is triggered on the node
that contains the last known mouse position, if it hasn't been triggered
before.
In the original test, when things are selected, we've already moved the
mouse out of the text field, so the autoscroll is triggered on <body>
instead.

https://codereview.chromium.org/2536083002/

[bo...@chromium.org]

Ok, sounds good, thanks for looking into it.

https://codereview.chromium.org/2536083002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2536083002/120001


https://codereview.chromium.org/2536083002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #6 (id:120001)

https://codereview.chromium.org/2536083002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 6 (id:??) landed as
https://crrev.com/94c73aebe631326d3d09aed291a6c335040b5447
Cr-Commit-Position: refs/heads/master@{#436317}

https://codereview.chromium.org/2536083002/