Prevent Control+two-finger-scroll from zooming the page.

Prevent Control+two-finger-scroll from zooming the page.

Make WebContentsImpl avoid zooming in response to a
two-finger-scroll event while the Control key is held. It's
very easy to accidentally trigger this feature and difficult
to undo it when a windowed app is focused.

Control+scrollwheel is still supported for zooming.

BUG=323164
TBR=avi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265391

[Daniel Erat, 2014-04-21 23:26:39]

https://codereview.chromium.org/242733008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/242733008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:195:
const blink::WebInputEvent& input_event) OVERRIDE {
please let me know if there's a better way to capture the modified events

[Rick Byers, 2014-04-22 14:15:15]

Thanks for working on this.

Can you add a link to sheckylin's CL in the comments (so we have a trail of all
the review history)?

https://codereview.chromium.org/242733008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/242733008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.cc:2527:
mouse_wheel_event.modifiers = 0;
Now that it's possible for pages to receive ctrl+wheel events
(http://crrev.com/254559) it seems a shame to strip the control modifier here.

Thinking about this more, I think the fundamental problem is that the zoom
handling in WebContentsImpl::HandleWheelEvent isn't designed for devices with
precise scroll deltas - the logic assumes there are fixed ticks.  See the
trackpad pinch handling in WebContentsImpl::HandleGestureEvent for an example of
similar logic that is designed for precise deltas.

So I think the right fix (for now) is to modify
WebContentsImpl::HandleWheelEvent to ignore wheel events with the
'hasPreciseDeltas' flag set.

[Daniel Erat, 2014-04-22 15:27:03]

https://codereview.chromium.org/242733008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/242733008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.cc:2527:
mouse_wheel_event.modifiers = 0;
On 2014/04/22 14:15:15, Rick Byers wrote:
> Now that it's possible for pages to receive ctrl+wheel events
> (http://crrev.com/254559) it seems a shame to strip the control modifier here.
> 
> Thinking about this more, I think the fundamental problem is that the zoom
> handling in WebContentsImpl::HandleWheelEvent isn't designed for devices with
> precise scroll deltas - the logic assumes there are fixed ticks.  See the
> trackpad pinch handling in WebContentsImpl::HandleGestureEvent for an example
of
> similar logic that is designed for precise deltas.
> 
> So I think the right fix (for now) is to modify
> WebContentsImpl::HandleWheelEvent to ignore wheel events with the
> 'hasPreciseDeltas' flag set.

thanks, makes sense (and i agree that it seems much better to handle this in the
place where we actually do the zooming)! i'll ping again after i've made this
change.

[Daniel Erat, 2014-04-22 17:40:12]

okay, i think this is ready for another look.

https://codereview.chromium.org/112293004/ was shecky's original change,
although the current version of this change no longer has any overlap with it.

https://codereview.chromium.org/242733008/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl_unittest.cc (left):

https://codereview.chromium.org/242733008/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl_unittest.cc:2433: float dy = 1;
deleted this since i didn't see much point for it -- note that |dx| is specified
inline.

[sadrul, 2014-04-22 17:48:09]

Nice. LGTM

[Daniel Erat, 2014-04-22 17:51:56]

TBR avi@ for owner approval (synthetic_web_input_event_builders.h just fixes
some indenting that was probably broken by s/webkit/blink/)

[Rick Byers, 2014-04-22 17:55:44]

Yeah, I like this.  LGTM

I don't know if trackpads are the ONLY devices that set
hasPreciseScrollingDeltas (eg. presumably also devices like Apple MagicMouse),
but presumably all such devices would have browser zoom problems today.

I can imagine there's someone somewhere using their precise-deltas device to
trigger browser zoom (maybe one that isn't very sensitive), but the risk is
probably low.  Rather than invest in making browser zoom work well for such
devices, I think we should focus on moving any such device to support
pinch-style zooming.

[Daniel Erat, 2014-04-22 18:08:34]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-04-22 18:09:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/242733008/40001

[commit-bot: I haz the power, 2014-04-22 19:22:51]

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

[commit-bot: I haz the power, 2014-04-22 19:22:52]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[Avi (use Gerrit), 2014-04-22 19:43:41]

LGTM

[Daniel Erat, 2014-04-22 19:45:32]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-04-22 19:47:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/242733008/40001

[commit-bot: I haz the power, 2014-04-22 22:39:58]

Change committed as 265391