Prevent ctrl+wheel from scrolling in more places

Prevent ctrl+wheel from scrolling in more places

Chromium now sends ctrl+wheel events to webkit first before using them for zooming. Rather than prevent scrolling for these events in EventHandler::handleWheel, push that check a couple levels deeper into ScrollableArea::handleWheelEvent.  In addition to EventHandler, this code is also called by WebPluginScrollbarImpl and so will fix some additional cases.

This is mostly already tested by the fast/event/wheelevent-ctrl.html test I added in http://crrev.com/183403002.  I've extended that test to cover document and div scrolling cases and ensured it fails reliably if either the ScrollableArea or EventHandler::defaultWheelEventHandler check is removed. 

BUG=352474, 111059
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170238

[Rick Byers, 2014-03-26 15:14:46]

Rob,
Can you please review this little update to the ctrl+wheel handling you recently
reviewed?

Thanks,
   Rick

[rjkroege, 2014-03-26 20:03:25]

https://codereview.chromium.org/212913002/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/212913002/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollAnimator.cpp:79: // ctrl+wheel events are used to
trigger zooming, not scrolling.
i am not convinced that this is the right place to do this. Why can't the
decision happen in EventHandler?

ScrollAnimator is (at least in my opinion) only about scrolling.

[Rick Byers, 2014-03-26 20:14:51]

On 2014/03/26 20:03:25, rjkroege wrote:
>
https://codereview.chromium.org/212913002/diff/1/Source/platform/scroll/Scrol...
> File Source/platform/scroll/ScrollAnimator.cpp (right):
> 
>
https://codereview.chromium.org/212913002/diff/1/Source/platform/scroll/Scrol...
> Source/platform/scroll/ScrollAnimator.cpp:79: // ctrl+wheel events are used to
> trigger zooming, not scrolling.
> i am not convinced that this is the right place to do this. Why can't the
> decision happen in EventHandler?

Because there are other paths for wheel events to trigger scrolling that don't
involve EventHandler, for example plugins that use WebPluginScrollbarImpl (such
as the PDF plugin).  I could put it in ScrollableArea::handleWheelEvent instead
(it appears to be the only caller of ScrollAnimator::handleWheelEvent).  I don't
think there's any existing easy way to test at that layer outside of layout
tests and eventSender (which I already have a test doing that failed to catch
the plugin issue), but perhaps we should just stick with the existing layout
test (as apparently most other code here does).  Or I could build something new
if necessary.

> ScrollAnimator is (at least in my opinion) only about scrolling.

Perhaps it shouldn't take a WebMouseWheelEvent then?  I was looking for the
lowest point that still dealt directly with wheel event objects (all the points
above just pass it down).

[rjkroege, 2014-03-26 20:26:11]

On 2014/03/26 20:14:51, Rick Byers wrote:
> On 2014/03/26 20:03:25, rjkroege wrote:
> >
>
https://codereview.chromium.org/212913002/diff/1/Source/platform/scroll/Scrol...
> > File Source/platform/scroll/ScrollAnimator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/212913002/diff/1/Source/platform/scroll/Scrol...
> > Source/platform/scroll/ScrollAnimator.cpp:79: // ctrl+wheel events are used
to
> > trigger zooming, not scrolling.
> > i am not convinced that this is the right place to do this. Why can't the
> > decision happen in EventHandler?
> 
> Because there are other paths for wheel events to trigger scrolling that don't
> involve EventHandler, for example plugins that use WebPluginScrollbarImpl
(such
> as the PDF plugin).

gack. That sounds like a layer violation. How does fling on PDF plugins work
then?

>  I could put it in ScrollableArea::handleWheelEvent 

yes. This seems better. It is more likely to survive ScrollAnimator refactoring.

>  instead
> (it appears to be the only caller of ScrollAnimator::handleWheelEvent).  I
don't
> think there's any existing easy way to test at that layer outside of layout
> tests and eventSender (which I already have a test doing that failed to catch
> the plugin issue), but perhaps we should just stick with the existing layout
> test (as apparently most other code here does).  Or I could build something
new
> if necessary.
> 
> > ScrollAnimator is (at least in my opinion) only about scrolling.
> 
> Perhaps it shouldn't take a WebMouseWheelEvent then?

I already proposed this a long time ago and never got to it. I can discuss
offline. ScrollAnimator (if it should exist at all) should just transform its
associated ScrollableArea.

>  I was looking for the
> lowest point that still dealt directly with wheel event objects (all the
points
> above just pass it down).

[Rick Byers, 2014-03-27 01:12:19]

On 2014/03/26 20:26:11, rjkroege wrote:
> On 2014/03/26 20:14:51, Rick Byers wrote:
> > On 2014/03/26 20:03:25, rjkroege wrote:
> > >
> >
>
https://codereview.chromium.org/212913002/diff/1/Source/platform/scroll/Scrol...
> > > File Source/platform/scroll/ScrollAnimator.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/212913002/diff/1/Source/platform/scroll/Scrol...
> > > Source/platform/scroll/ScrollAnimator.cpp:79: // ctrl+wheel events are
used
> to
> > > trigger zooming, not scrolling.
> > > i am not convinced that this is the right place to do this. Why can't the
> > > decision happen in EventHandler?
> > 
> > Because there are other paths for wheel events to trigger scrolling that
don't
> > involve EventHandler, for example plugins that use WebPluginScrollbarImpl
> (such
> > as the PDF plugin).
> 
> gack. That sounds like a layer violation. How does fling on PDF plugins work
> then?

I don't know, it seems to be working though.  Note that most plugins will use
the normal scrolling path, but PDF is special for some reason and registers to
handle wheel events itself.  It then handles them by passing them through to a
Pepper scrollbar widget which ultimately goes to
ScrollableArea::handleWheelEvent.

> >  I could put it in ScrollableArea::handleWheelEvent 
> 
> yes. This seems better. It is more likely to survive ScrollAnimator
refactoring.

Ok, done.

> >  instead
> > (it appears to be the only caller of ScrollAnimator::handleWheelEvent).  I
> don't
> > think there's any existing easy way to test at that layer outside of layout
> > tests and eventSender (which I already have a test doing that failed to
catch
> > the plugin issue), but perhaps we should just stick with the existing layout
> > test (as apparently most other code here does).  Or I could build something
> new
> > if necessary.

I looked for a bit and decided against writing a WebPluginScrollbar-specific
test - it looks like we don't have any existing tests for the plugin-specific
codepaths but just rely on the layout tests that use nearly identical code.  In
this case the difference is very minor so I don't think it warrants a completely
new type of test.  I did, however, beef up my existing layout test to cover both
the div and document scrolling cases (which hit slightly different codepaths)
and verified the test fails reliably if either the check in
EventHandler::defaultWheelEventHandler or the one in
ScrollableArea::handleWheelEvent gets removed.

> > 
> > > ScrollAnimator is (at least in my opinion) only about scrolling.
> > 
> > Perhaps it shouldn't take a WebMouseWheelEvent then?
> 
> I already proposed this a long time ago and never got to it. I can discuss
> offline. ScrollAnimator (if it should exist at all) should just transform its
> associated ScrollableArea.
> 
> >  I was looking for the
> > lowest point that still dealt directly with wheel event objects (all the
> points
> > above just pass it down).

[rjkroege, 2014-03-27 17:57:51]

lgtm

[Rick Byers, 2014-03-27 18:23:40]

Thanks!  +vollick for OWNERS

[Ian Vollick, 2014-03-27 19:31:15]

On 2014/03/27 18:23:40, Rick Byers wrote:
> Thanks!  +vollick for OWNERS

Unfortunately I'm not an OWNER of ScrollableArea.cpp. :(

[Rick Byers, 2014-03-27 21:29:41]

On 2014/03/27 19:31:15, Ian Vollick wrote:
> On 2014/03/27 18:23:40, Rick Byers wrote:
> > Thanks!  +vollick for OWNERS
> 
> Unfortunately I'm not an OWNER of ScrollableArea.cpp. :(

Whoops, missed-that - sorry!
  
+abarth for OWNERS instead

[abarth-chromium, 2014-03-27 21:32:42]

rslgtm

[Rick Byers, 2014-03-27 23:10:02]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2014-03-27 23:10:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/212913002/60001

[commit-bot: I haz the power, 2014-03-28 00:17:24]

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

[commit-bot: I haz the power, 2014-03-28 00:17:25]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Rick Byers, 2014-03-28 00:21:25]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2014-03-28 00:21:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/212913002/60001

[commit-bot: I haz the power, 2014-03-28 01:22:30]

Change committed as 170238