Honor the wheel event canScroll bit instead of trying to infer it from the ctrl modifier

Ctrl key while 2-finger trackpad scrolling kills scrolling unit test

We added a flag in Blink to decide if Ctrl-wheel-scroll should scroll or zoom, we then replace in the Blink code with this flag and we test it in the unit test.

This patch is part of a series:

patch #1: https://codereview.chromium.org/759073002
patch #2: https://codereview.chromium.org/835523006
patch #3: This CL

BUG=397027, 378755
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189452

[lanwei, 2014-11-26 23:02:30]

lanwei@chromium.org changed reviewers:
+ rbyers@chromium.org, tdresser@chromium.org

[tdresser, 2014-11-27 15:06:09]

https://codereview.chromium.org/768443002/diff/20001/LayoutTests/fast/events/...
File LayoutTests/fast/events/wheelevent-ctrl.html (right):

https://codereview.chromium.org/768443002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/wheelevent-ctrl.html:21: debug('Test mousewheel events
over scrollable div');
Can we write a test which has different expectations per platform, indicating
the difference in behavior?

https://codereview.chromium.org/768443002/diff/20001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/768443002/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2030: // Method suppressScroll() is reserved
for triggering zoom in/out actions in Chromium.
Method suppressScroll() ->
When scrolling is suppressed, we trigger zoom in/out instead.

https://codereview.chromium.org/768443002/diff/20001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/768443002/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/ScrollableArea.cpp:242: // Method suppressScroll() are
used to trigger zooming, not scrolling.
Reword to:

Wheel events whose scrolling has been suppressed are used to trigger zooming,
not scrolling.

[Rick Byers, 2014-11-27 18:05:54]

Also please update the CL summary / issue subject.  Eg:
"Honor the wheel event canScroll bit instead of trying to infer it from the ctrl
modifier".

https://codereview.chromium.org/768443002/diff/20001/LayoutTests/fast/events/...
File LayoutTests/fast/events/wheelevent-ctrl.html (right):

https://codereview.chromium.org/768443002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/wheelevent-ctrl.html:21: debug('Test mousewheel events
over scrollable div');
On 2014/11/27 15:06:09, tdresser wrote:
> Can we write a test which has different expectations per platform, indicating
> the difference in behavior?

Layout tests really test only blink (at least in terms of the input code path),
and the blink behavior is identical between platforms.  So I don't think we want
different expectations per platform.

You could argue though that we should test all the different forms of input used
by different platforms.  Eg. here we're testing only ctrl==suppressScroll. 
Perhaps it would be good to add a 3rd case to emulate mac / CrOS touchpad with
ctrl set but NOT suppressScroll and verify it behaves identically to when ctrl
isn't set.  We don't normally write tests for code that could exist but doesn't
(eg. why not also check for Alt wheel?).  But in this case since we're changing
from one behavior to another, it's not unreasonable to verify that the old
behavior is no longer true.

Thoughts?

[tdresser, 2014-11-27 18:19:57]

On 2014/11/27 18:05:54, Rick Byers wrote:
> Also please update the CL summary / issue subject.  Eg:
> "Honor the wheel event canScroll bit instead of trying to infer it from the
ctrl
> modifier".
> 
>
https://codereview.chromium.org/768443002/diff/20001/LayoutTests/fast/events/...
> File LayoutTests/fast/events/wheelevent-ctrl.html (right):
> 
>
https://codereview.chromium.org/768443002/diff/20001/LayoutTests/fast/events/...
> LayoutTests/fast/events/wheelevent-ctrl.html:21: debug('Test mousewheel events
> over scrollable div');
> On 2014/11/27 15:06:09, tdresser wrote:
> > Can we write a test which has different expectations per platform,
indicating
> > the difference in behavior?
> 
> Layout tests really test only blink (at least in terms of the input code
path),
> and the blink behavior is identical between platforms.  So I don't think we
want
> different expectations per platform.
> 
> You could argue though that we should test all the different forms of input
used
> by different platforms.  Eg. here we're testing only ctrl==suppressScroll. 
> Perhaps it would be good to add a 3rd case to emulate mac / CrOS touchpad with
> ctrl set but NOT suppressScroll and verify it behaves identically to when ctrl
> isn't set.  We don't normally write tests for code that could exist but
doesn't
> (eg. why not also check for Alt wheel?).  But in this case since we're
changing
> from one behavior to another, it's not unreasonable to verify that the old
> behavior is no longer true.
> 
> Thoughts?

I think that would be worth adding.

[lanwei, 2014-12-02 15:46:55]

Patchset #3 (id:40001) has been deleted

[lanwei, 2014-12-02 15:48:11]

https://codereview.chromium.org/768443002/diff/20001/LayoutTests/fast/events/...
File LayoutTests/fast/events/wheelevent-ctrl.html (right):

https://codereview.chromium.org/768443002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/wheelevent-ctrl.html:21: debug('Test mousewheel events
over scrollable div');
On 2014/11/27 18:05:54, Rick Byers wrote:
> On 2014/11/27 15:06:09, tdresser wrote:
> > Can we write a test which has different expectations per platform,
indicating
> > the difference in behavior?
> 
> Layout tests really test only blink (at least in terms of the input code
path),
> and the blink behavior is identical between platforms.  So I don't think we
want
> different expectations per platform.
> 
> You could argue though that we should test all the different forms of input
used
> by different platforms.  Eg. here we're testing only ctrl==suppressScroll. 
> Perhaps it would be good to add a 3rd case to emulate mac / CrOS touchpad with
> ctrl set but NOT suppressScroll and verify it behaves identically to when ctrl
> isn't set.  We don't normally write tests for code that could exist but
doesn't
> (eg. why not also check for Alt wheel?).  But in this case since we're
changing
> from one behavior to another, it's not unreasonable to verify that the old
> behavior is no longer true.
> 
> Thoughts?

Done.

https://codereview.chromium.org/768443002/diff/20001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/768443002/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2030: // Method suppressScroll() is reserved
for triggering zoom in/out actions in Chromium.
On 2014/11/27 15:06:09, tdresser wrote:
> Method suppressScroll() ->
> When scrolling is suppressed, we trigger zoom in/out instead.

Done.

https://codereview.chromium.org/768443002/diff/20001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/768443002/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/ScrollableArea.cpp:242: // Method suppressScroll() are
used to trigger zooming, not scrolling.
On 2014/11/27 15:06:09, tdresser wrote:
> Reword to:
> 
> Wheel events whose scrolling has been suppressed are used to trigger zooming,
> not scrolling.

Done.

[tdresser, 2014-12-02 15:59:44]

This LGTM (pending the other two patches of course)

[Rick Byers, 2014-12-02 16:06:58]

On 2014/11/27 18:05:54, Rick Byers wrote:
> Also please update the CL summary / issue subject.  Eg:
> "Honor the wheel event canScroll bit instead of trying to infer it from the
ctrl
> modifier".

LGTM with nits and the above change (this CL is really about paying attention to
the canScroll bit, the unit test is just a side-effect of that - not the primary
point of the CL itself).

[Rick Byers, 2014-12-02 17:54:03]

Whoops, forgot to actually publish my nit

https://codereview.chromium.org/768443002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/wheelevent-ctrl.html (right):

https://codereview.chromium.org/768443002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/wheelevent-ctrl.html:63: debug('Now without ctrl and
suppressScroll set to be default true');
nit: canScroll, not suppressScroll

[lanwei, 2014-12-02 22:28:22]

https://codereview.chromium.org/768443002/diff/60001/LayoutTests/fast/events/...
File LayoutTests/fast/events/wheelevent-ctrl.html (right):

https://codereview.chromium.org/768443002/diff/60001/LayoutTests/fast/events/...
LayoutTests/fast/events/wheelevent-ctrl.html:63: debug('Now without ctrl and
suppressScroll set to be default true');
On 2014/12/02 17:54:02, Rick Byers wrote:
> nit: canScroll, not suppressScroll

Done.

[lanwei, 2014-12-04 05:03:22]

lanwei@chromium.org changed reviewers:
+ haraken@chromium.org

[lanwei, 2014-12-04 05:03:23]

haraken@chromium.org: Please review changes in 

Can you please review Source/platform/scroll/ScrollableArea.cpp?

Thank you.

[haraken, 2014-12-04 05:06:05]

platform/ LGTM

[lanwei, 2015-02-03 18:52:06]

Patchset #7 (id:140001) has been deleted

[lanwei, 2015-02-03 20:40:18]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2015-02-03 20:40:26]

The CQ bit was unchecked by lanwei@chromium.org

[lanwei, 2015-02-03 20:45:53]

The CQ bit was checked by lanwei@chromium.org

[commit-bot: I haz the power, 2015-02-03 20:46:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/768443002/160001

[commit-bot: I haz the power, 2015-02-03 20:46:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-03 20:46:39]

Failed to apply patch for Source/core/page/EventHandler.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/page/EventHandler.cpp
  Hunk #1 FAILED at 60.
  Hunk #2 FAILED at 73.
  2 out of 3 hunks FAILED -- saving rejects to file
Source/core/page/EventHandler.cpp.rej

Patch:       Source/core/page/EventHandler.cpp
Index: Source/core/page/EventHandler.cpp
diff --git a/Source/core/page/EventHandler.cpp
b/Source/core/page/EventHandler.cpp
index
850bdba7fad1bcaa7d325a5bc7ea2e38d98a7b02..9277ab80e73427bb2ac812ba3f3d7548c32aaa69
100644
--- a/Source/core/page/EventHandler.cpp
+++ b/Source/core/page/EventHandler.cpp
@@ -60,6 +60,8 @@
 #include "core/html/HTMLFrameSetElement.h"
 #include "core/html/HTMLInputElement.h"
 #include "core/inspector/InspectorController.h"
+#include "core/layout/HitTestRequest.h"
+#include "core/layout/HitTestResult.h"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/page/AutoscrollController.h"
@@ -73,8 +75,6 @@
 #include "core/page/Page.h"
 #include "core/page/SpatialNavigation.h"
 #include "core/page/TouchAdjustment.h"
-#include "core/rendering/HitTestRequest.h"
-#include "core/rendering/HitTestResult.h"
 #include "core/rendering/RenderLayer.h"
 #include "core/rendering/RenderPart.h"
 #include "core/rendering/RenderTextControlSingleLine.h"
@@ -2062,8 +2062,8 @@ void EventHandler::defaultWheelEventHandler(Node*
startNode, WheelEvent* wheelEv
     if (!startNode || !wheelEvent)
         return;
 
-    // Ctrl + scrollwheel is reserved for triggering zoom in/out actions in
Chromium.
-    if (wheelEvent->ctrlKey())
+    // When the wheelEvent do not scroll, we trigger zoom in/out instead.
+    if (!wheelEvent->canScroll())
         return;
 
     Node* stopNode = m_previousWheelScrolledNode.get();

[lanwei, 2015-02-04 01:03:33]

The CQ bit was checked by lanwei@chromium.org

[commit-bot: I haz the power, 2015-02-04 01:04:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/768443002/180001

[commit-bot: I haz the power, 2015-02-04 03:22:40]

Committed patchset #8 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189452