Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
739013008:
Explicitly suppress scrolling for wheel events that will trigger zooming (Closed)
|
Created:
6 years ago by lanwei Modified:
5 years, 10 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, jdduke+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExplicitly suppress scrolling for wheel events that will trigger zooming
We added a flag in Blink to decide if Ctrl-wheel-scroll should scroll or zoom, and now we use this flag in chromium code.
This patch is part of a series:
patch #1: https://codereview.chromium.org/759073002
patch #2: This CL
patch #3: https://codereview.chromium.org/768443002
BUG=397027, 378755
Committed: https://crrev.com/b3361bcdd3766a67c702c19781154c895fee75ca
Cr-Commit-Position: refs/heads/master@{#309472}
Patch Set 1 : #
Total comments: 21
Patch Set 2 : #
Total comments: 10
Patch Set 3 : Change comments #
Total comments: 4
Patch Set 4 : format style change #Patch Set 5 : Unit tests #
Total comments: 10
Patch Set 6 : Add unit tests #
Total comments: 6
Patch Set 7 : Unittests #
Total comments: 4
Patch Set 8 : #
Total comments: 1
Patch Set 9 : #Patch Set 10 : rebase #Patch Set 11 : Modify event_sender.cc #Messages
Total messages: 47 (15 generated)
Patchset #1 (id:1) has been deleted
lanwei@chromium.org changed reviewers: + rbyers@chromium.org, tdresser@chromium.org
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
This should have some chrome side testing. https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:200: #if !defined(OS_MACOSX) Are _aura files compiled on Mac? Don't we want different behavior on ChromeOS and Windows? https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:1336: // gestures, which are regrettably easy to trigger accidentally. Indentation is wrong here, but I think this comment can be removed completely. https://codereview.chromium.org/739013008/diff/60001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/739013008/diff/60001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:2191: args->Skip(); Can't you replace args->PeekNext(); args->Skip(); with args->GetNext(); ?
You should also update web_input_event_traits.cc to accomodate this new bit. At least ApppendEventDetails and CanCoalesce. For testing you probably want at least: InputRouterImplTest::TouchpadPinchUpdate - update to check for bit WebInputEventTraitsTest - verify wheel events aren't coalesced when their canScroll bits don't match WebContentsImplTest::HandleWheelEvent - I think this might fail now (have you tried it?). You should probably verify that zoom is changed if and only if the canScroll bit is set to false. Something that verifies that canScroll does get set false on Aura for Ctrl+Wheel events. https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:202: !webkit_event.hasPreciseScrollingDeltas) { I don't think we should be duplicating the logic for when wheel events trigger zooming and when they don't. This feels like a policy that's likely to change over time, and so should be localized to one function somewhere. In some sense that's the underlying problem in the bugs we're trying to fix - I effectively duplicated the logic between chromium and blink, but even within chromium it shouldn't be duplicated. Today the logic is in WebContentsImpl::HandleWheelEvent. So perhaps we should extract it out to a new private function like 'bool WebContentsImpl::ShouldWheelEventZoom'. Then We could add a public API WebContents::ShouldWheelEventScroll which RenderWidgetHostImpl::ForwardWheel event calls in order to set the 'canScroll' bit on the WebMouseWheelEvent it's sending. Alternately, it might be cleaner (would let us remove an #ifdef!) to move the logic for deciding when wheel should zoom from WebContentsImpl to RenderWidgetHostView. Then RenderWidgetHostViewAura could make it's own platform-specific decision for when wheel events should trigger zooming (and Mac wouldn't get that behavior at all). It would use this logic both when calling MakeWebMouseWheelEvent and in RenderWidgetHostViewAura::WheelEventAck in order to trigger zooming. This would probably require adding a method like RenderViewHostDelegate::RequestZoom to get the zoom request back to the WebContentsImpl. https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:202: !webkit_event.hasPreciseScrollingDeltas) { Actually, now that I review the rest of your CL you're not duplicating the logic afterall - you're relying on this bit downstream. That's MUCH simpler than what I'm suggesting and I think nearly as good. It's a little weird to assume canScroll==false means must zoom, but this is all within content so given the simplicity I think it's fine. I'll leave the above comment since it took me awhile to figure out exactly what the alternatives would look like and it might still be worth revisiting at some point. https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:203: webkit_event.suppressScroll = true; You just need a comment here saying that by suppressing scrolling we're enabling WebContentsImpl::HandleWheelEvent to use the event to zoom the page. https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:203: webkit_event.suppressScroll = true; You need to set this bit in InputRouterImpl::SendSyntheticWheelEventForPinch also, otherwise touchpad pinch on Mac will cause scrolling! https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:1328: // On platforms other than Mac, control+mousewheel changes zoom. On Mac, this tweak this comment slightly to reflect the change in architecture. In particular replace "control+mousewheel changes zoom" to "control+mousewheel may change zoom". https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:1334: if (delegate_ && event.wheelTicksY && event.suppressScroll) { I think we should probably keep the ControlKey check here. I know this duplicates some logic, but it makes a bit more sense for WebContents to say that we'll zoom IF Control is pressed AND the platform has chosen to suppress scrolling for this event. It's not unreasonable that someone may choose to suppress scrolling in some other non-Ctrl case, and it would be odd if that triggered zooming on it's own. https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:1336: // gestures, which are regrettably easy to trigger accidentally. On 2014/11/27 15:06:08, tdresser wrote: > Indentation is wrong here, but I think this comment can be removed completely. The comment is still useful but now belongs in MakeWebMouseWheelEvent, so just move it there. https://codereview.chromium.org/739013008/diff/60001/content/renderer/input/i... File content/renderer/input/input_handler_proxy.cc (right): https://codereview.chromium.org/739013008/diff/60001/content/renderer/input/i... content/renderer/input/input_handler_proxy.cc:297: // Wheel events involving the control key never trigger scrolling, only Update this comment since it's no longer true - control key sometimes triggers scrolling now :-) Note that ideally we'd probably only forward to main when we know there's a wheel handler (eg. maybe we could still do the ScrollBegin below, just disable the call to ScrollBy). But Ctrl+Wheel is rare enough, I'm not too worried about optimizing it's performance.
On 2014/11/27 17:31:32, Rick Byers wrote: > You should also update web_input_event_traits.cc to accomodate this new bit. At > least ApppendEventDetails and CanCoalesce. > > For testing you probably want at least: > > InputRouterImplTest::TouchpadPinchUpdate - update to check for bit > > WebInputEventTraitsTest - verify wheel events aren't coalesced when their > canScroll bits don't match > > WebContentsImplTest::HandleWheelEvent - I think this might fail now (have you > tried it?). You should probably verify that zoom is changed if and only if the > canScroll bit is set to false. > > Something that verifies that canScroll does get set false on Aura for Ctrl+Wheel > events. > > https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... > File content/browser/renderer_host/web_input_event_aura.cc (right): > > https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... > content/browser/renderer_host/web_input_event_aura.cc:202: > !webkit_event.hasPreciseScrollingDeltas) { > I don't think we should be duplicating the logic for when wheel events trigger > zooming and when they don't. This feels like a policy that's likely to change > over time, and so should be localized to one function somewhere. In some sense > that's the underlying problem in the bugs we're trying to fix - I effectively > duplicated the logic between chromium and blink, but even within chromium it > shouldn't be duplicated. > > Today the logic is in WebContentsImpl::HandleWheelEvent. So perhaps we should > extract it out to a new private function like 'bool > WebContentsImpl::ShouldWheelEventZoom'. Then We could add a public API > WebContents::ShouldWheelEventScroll which RenderWidgetHostImpl::ForwardWheel > event calls in order to set the 'canScroll' bit on the WebMouseWheelEvent it's > sending. > > Alternately, it might be cleaner (would let us remove an #ifdef!) to move the > logic for deciding when wheel should zoom from WebContentsImpl to > RenderWidgetHostView. Then RenderWidgetHostViewAura could make it's own > platform-specific decision for when wheel events should trigger zooming (and Mac > wouldn't get that behavior at all). It would use this logic both when calling > MakeWebMouseWheelEvent and in RenderWidgetHostViewAura::WheelEventAck in order > to trigger zooming. This would probably require adding a method like > RenderViewHostDelegate::RequestZoom to get the zoom request back to the > WebContentsImpl. > > https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... > content/browser/renderer_host/web_input_event_aura.cc:202: > !webkit_event.hasPreciseScrollingDeltas) { > Actually, now that I review the rest of your CL you're not duplicating the logic > afterall - you're relying on this bit downstream. That's MUCH simpler than what > I'm suggesting and I think nearly as good. It's a little weird to assume > canScroll==false means must zoom, but this is all within content so given the > simplicity I think it's fine. > > I'll leave the above comment since it took me awhile to figure out exactly what > the alternatives would look like and it might still be worth revisiting at some > point. > > https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... > content/browser/renderer_host/web_input_event_aura.cc:203: > webkit_event.suppressScroll = true; > You just need a comment here saying that by suppressing scrolling we're enabling > WebContentsImpl::HandleWheelEvent to use the event to zoom the page. > > https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... > content/browser/renderer_host/web_input_event_aura.cc:203: > webkit_event.suppressScroll = true; > You need to set this bit in InputRouterImpl::SendSyntheticWheelEventForPinch > also, otherwise touchpad pinch on Mac will cause scrolling! > > https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... > File content/browser/web_contents/web_contents_impl.cc (right): > > https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... > content/browser/web_contents/web_contents_impl.cc:1328: // On platforms other > than Mac, control+mousewheel changes zoom. On Mac, this > tweak this comment slightly to reflect the change in architecture. In > particular replace "control+mousewheel changes zoom" to "control+mousewheel may > change zoom". > > https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... > content/browser/web_contents/web_contents_impl.cc:1334: if (delegate_ && > event.wheelTicksY && event.suppressScroll) { > I think we should probably keep the ControlKey check here. I know this > duplicates some logic, but it makes a bit more sense for WebContents to say that > we'll zoom IF Control is pressed AND the platform has chosen to suppress > scrolling for this event. It's not unreasonable that someone may choose to > suppress scrolling in some other non-Ctrl case, and it would be odd if that > triggered zooming on it's own. > > https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... > content/browser/web_contents/web_contents_impl.cc:1336: // gestures, which are > regrettably easy to trigger accidentally. > On 2014/11/27 15:06:08, tdresser wrote: > > Indentation is wrong here, but I think this comment can be removed completely. > > The comment is still useful but now belongs in MakeWebMouseWheelEvent, so just > move it there. > > https://codereview.chromium.org/739013008/diff/60001/content/renderer/input/i... > File content/renderer/input/input_handler_proxy.cc (right): > > https://codereview.chromium.org/739013008/diff/60001/content/renderer/input/i... > content/renderer/input/input_handler_proxy.cc:297: // Wheel events involving the > control key never trigger scrolling, only > Update this comment since it's no longer true - control key sometimes triggers > scrolling now :-) > > Note that ideally we'd probably only forward to main when we know there's a > wheel handler (eg. maybe we could still do the ScrollBegin below, just disable > the call to ScrollBy). But Ctrl+Wheel is rare enough, I'm not too worried about > optimizing it's performance. Also, again please update the CL title and issue summary to describe what you're changing in this particular CL. Eg. "Explicitly suppress scrolling for wheel events that will trigger zooming".
https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:200: #if !defined(OS_MACOSX) On 2014/11/27 15:06:08, tdresser wrote: > Are _aura files compiled on Mac? > Don't we want different behavior on ChromeOS and Windows? Done. https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:203: webkit_event.suppressScroll = true; On 2014/11/27 17:31:32, Rick Byers wrote: > You just need a comment here saying that by suppressing scrolling we're enabling > WebContentsImpl::HandleWheelEvent to use the event to zoom the page. Done. https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:203: webkit_event.suppressScroll = true; On 2014/11/27 17:31:32, Rick Byers wrote: > You need to set this bit in InputRouterImpl::SendSyntheticWheelEventForPinch > also, otherwise touchpad pinch on Mac will cause scrolling! Done. https://codereview.chromium.org/739013008/diff/60001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:203: webkit_event.suppressScroll = true; On 2014/11/27 17:31:32, Rick Byers wrote: > You need to set this bit in InputRouterImpl::SendSyntheticWheelEventForPinch > also, otherwise touchpad pinch on Mac will cause scrolling! Done. https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:1328: // On platforms other than Mac, control+mousewheel changes zoom. On Mac, this On 2014/11/27 17:31:32, Rick Byers wrote: > tweak this comment slightly to reflect the change in architecture. In > particular replace "control+mousewheel changes zoom" to "control+mousewheel may > change zoom". Done. https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:1334: if (delegate_ && event.wheelTicksY && event.suppressScroll) { On 2014/11/27 17:31:32, Rick Byers wrote: > I think we should probably keep the ControlKey check here. I know this > duplicates some logic, but it makes a bit more sense for WebContents to say that > we'll zoom IF Control is pressed AND the platform has chosen to suppress > scrolling for this event. It's not unreasonable that someone may choose to > suppress scrolling in some other non-Ctrl case, and it would be odd if that > triggered zooming on it's own. Done. https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:1336: // gestures, which are regrettably easy to trigger accidentally. On 2014/11/27 15:06:08, tdresser wrote: > Indentation is wrong here, but I think this comment can be removed completely. Done. https://codereview.chromium.org/739013008/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:1336: // gestures, which are regrettably easy to trigger accidentally. On 2014/11/27 15:06:08, tdresser wrote: > Indentation is wrong here, but I think this comment can be removed completely. Done. https://codereview.chromium.org/739013008/diff/60001/content/renderer/input/i... File content/renderer/input/input_handler_proxy.cc (right): https://codereview.chromium.org/739013008/diff/60001/content/renderer/input/i... content/renderer/input/input_handler_proxy.cc:297: // Wheel events involving the control key never trigger scrolling, only On 2014/11/27 17:31:32, Rick Byers wrote: > Update this comment since it's no longer true - control key sometimes triggers > scrolling now :-) > > Note that ideally we'd probably only forward to main when we know there's a > wheel handler (eg. maybe we could still do the ScrollBegin below, just disable > the call to ScrollBy). But Ctrl+Wheel is rare enough, I'm not too worried about > optimizing it's performance. > Done. https://codereview.chromium.org/739013008/diff/60001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/739013008/diff/60001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:2191: args->Skip(); On 2014/11/27 15:06:08, tdresser wrote: > Can't you replace > args->PeekNext(); args->Skip(); > with > args->GetNext(); > ? Done.
https://codereview.chromium.org/739013008/diff/80001/content/browser/renderer... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/739013008/diff/80001/content/browser/renderer... content/browser/renderer_host/input/input_router_impl.cc:460: // Avoid touchpad pinch on Mac OS to scroll. Is this Mac OS specific? Reword to: Prevent trackpad pinch from scrolling. OR Prevent trackpad pinch from scrolling on Mac OS. https://codereview.chromium.org/739013008/diff/80001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/739013008/diff/80001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:202: // event to zoom the page when MouseWheelEvent does not scroll. Can you clarify this? Perhaps assign hasPreciseScrollingDeltas to a local bool fromMouseWheel or fromTrackpad? I don't think you need to explicitly reference WebContentsImpl here. Say something more along the lines of: "Scroll events generated from the mouse wheel when the control key is held don't trigger scrolling. Instead, they may cause zooming." https://codereview.chromium.org/739013008/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/739013008/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:1328: // On platforms other than Mac, control+mousewheel may changes zoom. On Mac, may changes -> may change https://codereview.chromium.org/739013008/diff/80001/content/renderer/input/i... File content/renderer/input/input_handler_proxy.cc (right): https://codereview.chromium.org/739013008/diff/80001/content/renderer/input/i... content/renderer/input/input_handler_proxy.cc:297: // Wheel events involving the control key with canScroll false will not I don't think you need to reference both the control key and |canScroll|. I'd say something like: // Wheel events with |canScroll| == false will not trigger scrolling, .... https://codereview.chromium.org/739013008/diff/80001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/739013008/diff/80001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:2186: args->GetNext(&paged); Invert each of these conditions (including the ones you didn't write.) if (args->PeekNext().IsEmpty()) return; args->GetNext(&paged) if (args->PeekNext().IsEmpty()) return; args->GetNext(&has_precise_scrolling_deltas); if (args->PeekNext().IsEmpty()) return; v8::Handle<v8::Value> value; args->GetNext(&value); modifiers = GetKeyModifiersFromV8(value); if (args->PeekNext().IsEmpty()) return; args->GetNext(&can_scroll);
https://codereview.chromium.org/739013008/diff/80001/content/browser/renderer... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/739013008/diff/80001/content/browser/renderer... content/browser/renderer_host/input/input_router_impl.cc:460: // Avoid touchpad pinch on Mac OS to scroll. On 2014/12/02 15:37:37, tdresser wrote: > Is this Mac OS specific? > > Reword to: > Prevent trackpad pinch from scrolling. > OR > Prevent trackpad pinch from scrolling on Mac OS. Done. Yes, this is only for Mac OS. https://codereview.chromium.org/739013008/diff/80001/content/browser/renderer... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/739013008/diff/80001/content/browser/renderer... content/browser/renderer_host/web_input_event_aura.cc:202: // event to zoom the page when MouseWheelEvent does not scroll. On 2014/12/02 15:37:38, tdresser wrote: > Can you clarify this? > > Perhaps assign hasPreciseScrollingDeltas to a local bool fromMouseWheel or > fromTrackpad? > > I don't think you need to explicitly reference WebContentsImpl here. > Say something more along the lines of: > "Scroll events generated from the mouse wheel when the control key is held don't > trigger scrolling. Instead, they may cause zooming." Done. https://codereview.chromium.org/739013008/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/739013008/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:1328: // On platforms other than Mac, control+mousewheel may changes zoom. On Mac, On 2014/12/02 15:37:38, tdresser wrote: > may changes -> may change Done. https://codereview.chromium.org/739013008/diff/80001/content/renderer/input/i... File content/renderer/input/input_handler_proxy.cc (right): https://codereview.chromium.org/739013008/diff/80001/content/renderer/input/i... content/renderer/input/input_handler_proxy.cc:297: // Wheel events involving the control key with canScroll false will not On 2014/12/02 15:37:38, tdresser wrote: > I don't think you need to reference both the control key and |canScroll|. > > I'd say something like: > // Wheel events with |canScroll| == false will not trigger scrolling, .... Done. https://codereview.chromium.org/739013008/diff/80001/content/shell/renderer/t... File content/shell/renderer/test_runner/event_sender.cc (right): https://codereview.chromium.org/739013008/diff/80001/content/shell/renderer/t... content/shell/renderer/test_runner/event_sender.cc:2186: args->GetNext(&paged); On 2014/12/02 15:37:38, tdresser wrote: > Invert each of these conditions (including the ones you didn't write.) > > if (args->PeekNext().IsEmpty()) > return; > args->GetNext(&paged) > if (args->PeekNext().IsEmpty()) > return; > args->GetNext(&has_precise_scrolling_deltas); > if (args->PeekNext().IsEmpty()) > return; > v8::Handle<v8::Value> value; > args->GetNext(&value); > modifiers = GetKeyModifiersFromV8(value); > if (args->PeekNext().IsEmpty()) > return; > args->GetNext(&can_scroll); Done. Thanks for the suggestion, looks much clearer right now.
https://codereview.chromium.org/739013008/diff/100001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/739013008/diff/100001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:202: bool from_mouseWheel = !webkit_event.hasPreciseScrollingDeltas; Sorry, I used the wrong formatting in my comment. Should be from_mousewheel or from_mouse_wheel. I think that's a lot clearer now, thanks!
On 2014/12/03 13:45:05, tdresser wrote: > https://codereview.chromium.org/739013008/diff/100001/content/browser/rendere... > File content/browser/renderer_host/web_input_event_aura.cc (right): > > https://codereview.chromium.org/739013008/diff/100001/content/browser/rendere... > content/browser/renderer_host/web_input_event_aura.cc:202: bool from_mouseWheel > = !webkit_event.hasPreciseScrollingDeltas; > Sorry, I used the wrong formatting in my comment. > > Should be from_mousewheel or from_mouse_wheel. > > I think that's a lot clearer now, thanks! LGTM with the above nit.
On 2014/12/03 13:45:48, tdresser wrote: > On 2014/12/03 13:45:05, tdresser wrote: > > > https://codereview.chromium.org/739013008/diff/100001/content/browser/rendere... > > File content/browser/renderer_host/web_input_event_aura.cc (right): > > > > > https://codereview.chromium.org/739013008/diff/100001/content/browser/rendere... > > content/browser/renderer_host/web_input_event_aura.cc:202: bool > from_mouseWheel > > = !webkit_event.hasPreciseScrollingDeltas; > > Sorry, I used the wrong formatting in my comment. > > > > Should be from_mousewheel or from_mouse_wheel. > > > > I think that's a lot clearer now, thanks! > > LGTM with the above nit. (assuming rbyers' comments are addressed)
lanwei@chromium.org changed reviewers: + jdduke@chromium.org
Rubberstamp lgtm for content/browser/renderer_host/input and content/common/input, with nit. https://codereview.chromium.org/739013008/diff/100001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/739013008/diff/100001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:204: && from_mouseWheel) { Nit: The && should be at the end of the preceding line (unlike Java =/).
lanwei@chromium.org changed reviewers: + sievers@chromium.org
sievers@chromium.org: Please review changes in Can you please take a look at content/browser/renderer_host/web_input_event_aura.cc content/browser/web_contents/web_contents_impl.cc content/shell/renderer/test_runner/event_sender.cc? Thank you. https://codereview.chromium.org/739013008/diff/100001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/739013008/diff/100001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:202: bool from_mouseWheel = !webkit_event.hasPreciseScrollingDeltas; On 2014/12/03 13:45:05, tdresser wrote: > Sorry, I used the wrong formatting in my comment. > > Should be from_mousewheel or from_mouse_wheel. > > I think that's a lot clearer now, thanks! Done. https://codereview.chromium.org/739013008/diff/100001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:204: && from_mouseWheel) { On 2014/12/03 21:37:10, jdduke wrote: > Nit: The && should be at the end of the preceding line (unlike Java =/). Done. Yes, just realize they have different styles.
lgtm
On 2014/11/27 17:32:54, Rick Byers wrote: > Also, again please update the CL title and issue summary to describe what you're > changing in this particular CL. Eg. "Explicitly suppress scrolling for wheel > events that will trigger zooming". I've done this for you since it's trivial. Feel free to change the wording if you prefer.
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
Rick, can you please take a look at these, where do you think it is a good place to add a test to check if the canScroll is correctly setup? Thank you.
Patchset #5 (id:180001) has been deleted
Sorry for the delay. This is looking really good, just a couple more minor suggestions. https://codereview.chromium.org/739013008/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/739013008/diff/200001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:464: // Prevent trackpad pinch from scrolling on Mac OS. nit: This comment seems redundant with the code (and the intention is to use this code on ChromeOS someday too) - I'd just remove it. https://codereview.chromium.org/739013008/diff/200001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/739013008/diff/200001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:207: } I think you could write a unit test for this code similar to the code in RenderWidgetHostViewAuraTest::FinishCompositionByMouse. I.e. construct a fake Aura event, pass it to OnMouseEvent, then verify the canScroll bit in the outgoing IPC message. https://codereview.chromium.org/739013008/diff/200001/content/common/input/sy... File content/common/input/synthetic_web_input_event_builders.cc (right): https://codereview.chromium.org/739013008/diff/200001/content/common/input/sy... content/common/input/synthetic_web_input_event_builders.cc:71: if ((modifiers & blink::WebInputEvent::ControlKey) && !precise) { Which tests fail if you just unconditionally set canScroll to true here? It's a little odd that we're baking Aura's logic into the syntehtic event builders used by tests - even on Mac where the logic doesn't apply. Ideally your CL would need to update WebContentsImplTest::HandleWheelEvent to explicitly set canScroll to false on the event. Then you'd also be able to test that a canScroll=true event doesn't ever also cause zooming. In particular, look at the part of the test that says "Events containing precise scrolling deltas also shouldn't result in ...". Previously it was testing something valuable. Now after your CL it's really testing the behavior of the synthetic event building code here, not the code in the product that actually pays attention to hasPreciseDeltas! https://codereview.chromium.org/739013008/diff/200001/content/common/input/we... File content/common/input/web_input_event_traits_unittest.cc (right): https://codereview.chromium.org/739013008/diff/200001/content/common/input/we... content/common/input/web_input_event_traits_unittest.cc:55: static WebMouseWheelEvent CreateMouseWheel(WebInputEvent::Type type, no need to take a 'type' here - unlike touch/gesture there's only a single type of 'wheel' event. Also kind of weird to take 'canScroll' but nothing else. Maybe take deltaX/deltaY since those are the most important parts of a WebMouseWheelEvent? https://codereview.chromium.org/739013008/diff/200001/content/common/input/we... content/common/input/web_input_event_traits_unittest.cc:179: // WebMouseWheelEvent objects with different canScroll values should not please add a baseline that verifies CanCoalesce ever returns true in some case - eg. when everything matches except the deltas.
Rick, can you please take a look at the unit tests that I added. Thank you. https://codereview.chromium.org/739013008/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/739013008/diff/200001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:464: // Prevent trackpad pinch from scrolling on Mac OS. On 2014/12/12 22:20:29, Rick Byers wrote: > nit: This comment seems redundant with the code (and the intention is to use > this code on ChromeOS someday too) - I'd just remove it. Done. https://codereview.chromium.org/739013008/diff/200001/content/browser/rendere... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/739013008/diff/200001/content/browser/rendere... content/browser/renderer_host/web_input_event_aura.cc:207: } On 2014/12/12 22:20:29, Rick Byers wrote: > I think you could write a unit test for this code similar to the code in > RenderWidgetHostViewAuraTest::FinishCompositionByMouse. I.e. construct a fake > Aura event, pass it to OnMouseEvent, then verify the canScroll bit in the > outgoing IPC message. Done. https://codereview.chromium.org/739013008/diff/200001/content/common/input/sy... File content/common/input/synthetic_web_input_event_builders.cc (right): https://codereview.chromium.org/739013008/diff/200001/content/common/input/sy... content/common/input/synthetic_web_input_event_builders.cc:71: if ((modifiers & blink::WebInputEvent::ControlKey) && !precise) { On 2014/12/12 22:20:29, Rick Byers wrote: > Which tests fail if you just unconditionally set canScroll to true here? It's a > little odd that we're baking Aura's logic into the syntehtic event builders used > by tests - even on Mac where the logic doesn't apply. > > Ideally your CL would need to update WebContentsImplTest::HandleWheelEvent to > explicitly set canScroll to false on the event. Then you'd also be able to test > that a canScroll=true event doesn't ever also cause zooming. > > In particular, look at the part of the test that says "Events containing precise > scrolling deltas also shouldn't result in ...". Previously it was testing > something valuable. Now after your CL it's really testing the behavior of the > synthetic event building code here, not the code in the product that actually > pays attention to hasPreciseDeltas! Done. https://codereview.chromium.org/739013008/diff/200001/content/common/input/we... File content/common/input/web_input_event_traits_unittest.cc (right): https://codereview.chromium.org/739013008/diff/200001/content/common/input/we... content/common/input/web_input_event_traits_unittest.cc:55: static WebMouseWheelEvent CreateMouseWheel(WebInputEvent::Type type, On 2014/12/12 22:20:29, Rick Byers wrote: > no need to take a 'type' here - unlike touch/gesture there's only a single type > of 'wheel' event. > > Also kind of weird to take 'canScroll' but nothing else. Maybe take > deltaX/deltaY since those are the most important parts of a WebMouseWheelEvent? Done. https://codereview.chromium.org/739013008/diff/200001/content/common/input/we... content/common/input/web_input_event_traits_unittest.cc:179: // WebMouseWheelEvent objects with different canScroll values should not On 2014/12/12 22:20:29, Rick Byers wrote: > please add a baseline that verifies CanCoalesce ever returns true in some case - > eg. when everything matches except the deltas. Done.
Patchset #6 (id:220001) has been deleted
Rick, I change the mouseevent type in the unit test, now it works, can you please take a look? Thank you.
Rick, I change the mouseevent type in the unit test, now it works, can you please take a look? Thank you.
Great! One more little bit of missing test coverage and this is ready to land... https://codereview.chromium.org/739013008/diff/240001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/739013008/diff/240001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3030: EXPECT_FALSE(wheel_event->canScroll); Great, this is the key case you want to test. But you should also verify that you get TRUE when modifiers doesn't have CONTROL, and when it does but you have precise deltas. Then you will have tested all the important cases. https://codereview.chromium.org/739013008/diff/240001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/739013008/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2479: modifiers = WebInputEvent::ShiftKey | WebInputEvent::AltKey; nit: set the Ctrl modifier here too (to verify that the modifier alone isn't enough to change behavior). https://codereview.chromium.org/739013008/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2484: // But whenever the ctrl modifier is applied, they can increase/decrease zoom. nit: add "with canScroll=false" to this comment.
Rick, do you think the unit tests have a good coverage right now? Thank you. https://codereview.chromium.org/739013008/diff/240001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/739013008/diff/240001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3030: EXPECT_FALSE(wheel_event->canScroll); On 2014/12/18 15:18:14, Rick Byers wrote: > Great, this is the key case you want to test. But you should also verify that > you get TRUE when modifiers doesn't have CONTROL, and when it does but you have > precise deltas. Then you will have tested all the important cases. Done. https://codereview.chromium.org/739013008/diff/240001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/739013008/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2479: modifiers = WebInputEvent::ShiftKey | WebInputEvent::AltKey; On 2014/12/18 15:18:14, Rick Byers wrote: > nit: set the Ctrl modifier here too (to verify that the modifier alone isn't > enough to change behavior). Done. https://codereview.chromium.org/739013008/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2484: // But whenever the ctrl modifier is applied, they can increase/decrease zoom. On 2014/12/18 15:18:14, Rick Byers wrote: > nit: add "with canScroll=false" to this comment. Done.
Looks great, just one little bug https://codereview.chromium.org/739013008/diff/260001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/739013008/diff/260001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3033: // Ack'ing the outstanding event should flush the pending touch queue. nit: not "touch", maybe just "event"? https://codereview.chromium.org/739013008/diff/260001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3058: 0, 0, 5, 0, 5, 2); Looks like you forgot to actually set the ctrl modifier here?
Patchset #8 (id:280001) has been deleted
Patchset #8 (id:300001) has been deleted
https://codereview.chromium.org/739013008/diff/260001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/739013008/diff/260001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3033: // Ack'ing the outstanding event should flush the pending touch queue. On 2014/12/19 15:44:46, Rick Byers (Out until 12-5) wrote: > nit: not "touch", maybe just "event"? Done. https://codereview.chromium.org/739013008/diff/260001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3058: 0, 0, 5, 0, 5, 2); On 2014/12/19 15:44:46, Rick Byers (Out until 12-5) wrote: > Looks like you forgot to actually set the ctrl modifier here? Done.
Thanks, LGTM with nit I only re-reviewed the unit test, let me know if there have been changes in other files I should be re-reviewing. https://codereview.chromium.org/739013008/diff/320001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/739013008/diff/320001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3130: // Check if the canScroll set to true when ctrl-tpuchpad-scroll is generated nit: typo, "tpuchpad"
The CQ bit was checked by lanwei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/739013008/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lanwei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/739013008/360001
Message was sent while issue was closed.
Committed patchset #10 (id:360001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/b3361bcdd3766a67c702c19781154c895fee75ca Cr-Commit-Position: refs/heads/master@{#309472}
Message was sent while issue was closed.
On 2014/12/22 22:38:43, I haz the power (commit-bot) wrote: > Patchset 10 (id:??) landed as > https://crrev.com/b3361bcdd3766a67c702c19781154c895fee75ca > Cr-Commit-Position: refs/heads/master@{#309472} This patch seems to have broken a lot of layout tests using WheelEvent, c.f. http://build.chromium.org/p/chromium.webkit/builders/WebKit%20XP/builds/15918 I'm going to revert this patch to get the tree green.
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:360001) has been created in https://codereview.chromium.org/819993002/ by yutak@chromium.org. The reason for reverting is: Broke layout tests that use wheel events: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20XP/builds/15918. |
