On 2014/02/15 10:57:45, robwu wrote: > Fixed the scrollwheel issue. Depends on a patch in ...
6 years, 10 months ago
(2014-02-16 23:06:57 UTC)
#4
On 2014/02/15 10:57:45, robwu wrote:
> Fixed the scrollwheel issue. Depends on a patch in Blink:
> https://codereview.chromium.org/168493002/
Thanks for looking at this!
I'm definitely no expert in this code so I'm not the right person to review, but
I had something similar when I drafted up a patch for this (yours looks much
more complete). Note though my patch for gtk. I believe we're switching to Aura
after the next release branch so it might not be necessary depending on when you
land this.
Note also that this doesn't fix pinch-zooming on Mac which is a bit sad. AFAICT
there's no way to capture pinch zoom on Mac.
See: https://codereview.chromium.org/169093002
robwu
On 2014/02/16 23:06:57, raymes wrote: > On 2014/02/15 10:57:45, robwu wrote: > > Fixed the ...
6 years, 10 months ago
(2014-02-17 00:10:30 UTC)
#5
On 2014/02/16 23:06:57, raymes wrote:
> On 2014/02/15 10:57:45, robwu wrote:
> > Fixed the scrollwheel issue. Depends on a patch in Blink:
> > https://codereview.chromium.org/168493002/
>
> Thanks for looking at this!
NP, I want the bug to be fixed.
>
> I'm definitely no expert in this code so I'm not the right person to review,
but
I added you because you were the owner of the bug. I thought that jam would be
qualified to review. When I'm wrong, please ping the right owner.
> I had something similar when I drafted up a patch for this (yours looks much
> more complete). Note though my patch for gtk. I believe we're switching to
Aura
> after the next release branch so it might not be necessary depending on when
you
> land this.
What is the relevance of GTK in this story?
>
> Note also that this doesn't fix pinch-zooming on Mac which is a bit sad.
AFAICT
> there's no way to capture pinch zoom on Mac.
I don't know about the bug on Mac (I am on Linux), so I didn't touch that logic.
>
> See: https://codereview.chromium.org/169093002
The change in the other commit won't work when the page contains scrollbars. I
experienced the same issue in my first CL. This must be fixed in Blink
(see my CL at https://codereview.chromium.org/168493002/).
robwu
My patch for Blink was accepted and included in the Blink roll of today. jam: ...
6 years, 10 months ago
(2014-02-20 17:01:23 UTC)
#6
My patch for Blink was accepted and included in the Blink roll of today.
jam: Could you review this CL? Manual test instructions are included in the CL
description.
raymes
On 2014/02/20 17:01:23, robwu wrote: > My patch for Blink was accepted and included in ...
6 years, 10 months ago
(2014-02-21 03:24:42 UTC)
#7
On 2014/02/20 17:01:23, robwu wrote:
> My patch for Blink was accepted and included in the Blink roll of today.
>
> jam: Could you review this CL? Manual test instructions are included in the CL
> description.
jam is OOO but should be back next week.
jam
+jdduke for review of input files Has anyone run this with UI leads to ensure ...
6 years, 10 months ago
(2014-02-26 00:02:37 UTC)
#8
+jdduke for review of input files
Has anyone run this with UI leads to ensure that they're ok with renderers
blocking this action?
jdduke (slow)
I'd also like to hear what the UI folks say before I make any detailed ...
6 years, 10 months ago
(2014-02-26 00:24:39 UTC)
#9
I'd also like to hear what the UI folks say before I make any detailed comments.
Can the analogous keyboard zoom accelerators (ctrl+shit+-/+) be
preventDefault'ed? If so, I don't see any particular harm in this (though I'm a
bit partial to the always responsive ctrl+mousewheel zoom).
jamesr@: Can you see any issues with this change?
raymes
Just for the record, ctrl++ and ctrl+- can be overridden. On Wed, Feb 26, 2014 ...
6 years, 10 months ago
(2014-02-26 00:27:13 UTC)
#10
Just for the record, ctrl++ and ctrl+- can be overridden.
On Wed, Feb 26, 2014 at 11:24 AM, <jdduke@chromium.org> wrote:
> I'd also like to hear what the UI folks say before I make any detailed
> comments.
> Can the analogous keyboard zoom accelerators (ctrl+shit+-/+) be
> preventDefault'ed? If so, I don't see any particular harm in this (though
> I'm a
> bit partial to the always responsive ctrl+mousewheel zoom).
>
> jamesr@: Can you see any issues with this change?
>
> https://codereview.chromium.org/165143003/
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
jamesr
It seems like this is mostly a UX question of whether the page should be ...
6 years, 10 months ago
(2014-02-26 23:51:51 UTC)
#11
On 2014/02/26 23:51:51, jamesr wrote: > It seems like this is mostly a UX question ...
6 years, 9 months ago
(2014-02-27 23:03:08 UTC)
#12
On 2014/02/26 23:51:51, jamesr wrote:
> It seems like this is mostly a UX question of whether the page should be
allowed
> to override this. I'm not qualified to answer that.
Ctrl + +/- can already be intercepted. Other browsers (Firefox, IE) allow web
pages to block Ctrl + mousewheel, FYI.
>
> Otherwise this seems fine.
>
>
https://codereview.chromium.org/165143003/diff/60001/content/renderer/input/i...
> File content/renderer/input/input_handler_proxy.cc (right):
>
>
https://codereview.chromium.org/165143003/diff/60001/content/renderer/input/i...
> content/renderer/input/input_handler_proxy.cc:119: // Ctrl + scrollwheel is
> reserved for zooming.
> is this true on all platforms? are there any other modifiers to worry about?
I have amended the CL to revert this change, because it is of no use (it was
added during an attempt to fix the scrollbar issue).
When I run Chromium through gdb (in single-process mode), the relevant methods
are called in the next sequence:
1. InputHandlerProxy::HandleInputEvent
2. WebContentsImpl::PreHandleWheelEvent
3. RenderWidget::OnHandleInputEvent
4. RenderWidgetHostImpl::OnWheelEventAck
After applying this CL, the code runs in the following sequence:
1. InputHandlerProxy::HandleInputEvent
2. RenderWidget::OnHandleInputEvent
3. RenderWidgetHostImpl::OnWheelEventAck
4. WebContentsImpl::HandleWheelEvent (renamed from PreHandleWheelEvent)
Since HandleInputEvent runs before the ctrl+mousewheel handler in both cases
(with and without CL), changing the method is probably unnecessary, so I
reverted the change.
jdduke (slow)
On 2014/02/27 23:03:08, robwu wrote: > I have amended the CL to revert this change, ...
6 years, 9 months ago
(2014-02-27 23:36:54 UTC)
#13
On 2014/02/27 23:03:08, robwu wrote:
> I have amended the CL to revert this change, because it is of no use (it was
> added during an attempt to fix the scrollbar issue).
> When I run Chromium through gdb (in single-process mode), the relevant methods
> are called in the next sequence:
> 1. InputHandlerProxy::HandleInputEvent
> 2. WebContentsImpl::PreHandleWheelEvent
> 3. RenderWidget::OnHandleInputEvent
> 4. RenderWidgetHostImpl::OnWheelEventAck
Hmm, that doesn't look quite right. For a given wheel event,
WebContentsImpl::PreHandleWheelEvent will always be called before
InputHandlerProxy::HandleInputEvent.
robwu
On 2014/02/27 23:36:54, jdduke wrote: > On 2014/02/27 23:03:08, robwu wrote: > > I have ...
6 years, 9 months ago
(2014-02-28 11:26:06 UTC)
#14
On 2014/02/27 23:36:54, jdduke wrote:
> On 2014/02/27 23:03:08, robwu wrote:
> > I have amended the CL to revert this change, because it is of no use (it was
> > added during an attempt to fix the scrollbar issue).
> > When I run Chromium through gdb (in single-process mode), the relevant
methods
> > are called in the next sequence:
> > 1. InputHandlerProxy::HandleInputEvent
> > 2. WebContentsImpl::PreHandleWheelEvent
> > 3. RenderWidget::OnHandleInputEvent
> > 4. RenderWidgetHostImpl::OnWheelEventAck
>
> Hmm, that doesn't look quite right. For a given wheel event,
> WebContentsImpl::PreHandleWheelEvent will always be called before
> InputHandlerProxy::HandleInputEvent.
Apologies, I have made an error in my previous notes. The sequence of events
matches your description:
// 1. RenderWidgetHostImpl::ForwardWheelEventWithLatencyInfo (at former
PreHandleWheelEvent location)
// 2. InputHandlerProxy::HandleInputEvent
// 3. RenderWidget::OnHandleInputEvent
// 4. RenderWidgetHostImpl::OnWheelEventAck
gdb --args ./Debug/chrome --user-data-dir=/tmp/dadada/ --no-first-run
--no-sandbox --disable-hang-monitor --single-process /tmp/manual-tests.html
dir /tmp/SRC
break content/browser/renderer_host/render_widget_host_impl.cc:975
break content/renderer/input/input_handler_proxy.cc:118
break content/renderer/render_widget.cc:1125 if input_event->type ==
WebInputEvent::MouseWheel
break content/browser/renderer_host/render_widget_host_impl.cc:2052
r
HandleInputEvent seems not to have any noticeable side effects (desktop Linux)
when it also called for Ctrl + mousewheel.
It returns DID_NOT_HANDLE, because scroll_status == ScrollOnMainThread
(http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/input/input_...),
which in turn is caused by have_wheel_event_handlers() being true
(http://src.chromium.org/viewvc/chrome/trunk/src/cc/layers/layer_impl.cc?revis...).
jdduke (slow)
On 2014/02/28 11:26:06, robwu wrote: > Apologies, I have made an error in my previous ...
6 years, 9 months ago
(2014-02-28 14:18:53 UTC)
#15
On 2014/02/28 11:26:06, robwu wrote:
> Apologies, I have made an error in my previous notes. The sequence of events
> matches your description:
> // 1. RenderWidgetHostImpl::ForwardWheelEventWithLatencyInfo (at former
> PreHandleWheelEvent location)
> // 2. InputHandlerProxy::HandleInputEvent
> // 3. RenderWidget::OnHandleInputEvent
> // 4. RenderWidgetHostImpl::OnWheelEventAck
> gdb --args ./Debug/chrome --user-data-dir=/tmp/dadada/ --no-first-run
> --no-sandbox --disable-hang-monitor --single-process /tmp/manual-tests.html
> dir /tmp/SRC
> break content/browser/renderer_host/render_widget_host_impl.cc:975
> break content/renderer/input/input_handler_proxy.cc:118
> break content/renderer/render_widget.cc:1125 if input_event->type ==
> WebInputEvent::MouseWheel
> break content/browser/renderer_host/render_widget_host_impl.cc:2052
> r
>
> HandleInputEvent seems not to have any noticeable side effects (desktop Linux)
> when it also called for Ctrl + mousewheel.
> It returns DID_NOT_HANDLE, because scroll_status == ScrollOnMainThread
>
(http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/input/input_...),
> which in turn is caused by have_wheel_event_handlers() being true
>
(http://src.chromium.org/viewvc/chrome/trunk/src/cc/layers/layer_impl.cc?revis...).
Right, but what about the case where the page does not have a wheel handler?
Then, ctrl+mousewheel will be handled as a regular wheel scroll by the
compositor, and as long as the page can be scrolled (by the compositor) you
won't be able to ctrl+mousewheel zoom.
robwu
On 2014/02/28 14:18:53, jdduke wrote: > On 2014/02/28 11:26:06, robwu wrote: > > Apologies, I ...
6 years, 9 months ago
(2014-02-28 17:34:51 UTC)
#16
On 2014/02/28 14:18:53, jdduke wrote:
> On 2014/02/28 11:26:06, robwu wrote:
> > Apologies, I have made an error in my previous notes. The sequence of events
> > matches your description:
> > // 1. RenderWidgetHostImpl::ForwardWheelEventWithLatencyInfo (at former
> > PreHandleWheelEvent location)
> > // 2. InputHandlerProxy::HandleInputEvent
> > // 3. RenderWidget::OnHandleInputEvent
> > // 4. RenderWidgetHostImpl::OnWheelEventAck
> > gdb --args ./Debug/chrome --user-data-dir=/tmp/dadada/ --no-first-run
> > --no-sandbox --disable-hang-monitor --single-process /tmp/manual-tests.html
> > dir /tmp/SRC
> > break content/browser/renderer_host/render_widget_host_impl.cc:975
> > break content/renderer/input/input_handler_proxy.cc:118
> > break content/renderer/render_widget.cc:1125 if input_event->type ==
> > WebInputEvent::MouseWheel
> > break content/browser/renderer_host/render_widget_host_impl.cc:2052
> > r
> >
> > HandleInputEvent seems not to have any noticeable side effects (desktop
Linux)
> > when it also called for Ctrl + mousewheel.
> > It returns DID_NOT_HANDLE, because scroll_status == ScrollOnMainThread
> >
>
(http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/input/input_...),
> > which in turn is caused by have_wheel_event_handlers() being true
> >
>
(http://src.chromium.org/viewvc/chrome/trunk/src/cc/layers/layer_impl.cc?revis...).
>
> Right, but what about the case where the page does not have a wheel handler?
> Then, ctrl+mousewheel will be handled as a regular wheel scroll by the
> compositor, and as long as the page can be scrolled (by the compositor) you
> won't be able to ctrl+mousewheel zoom.
I restored the check, and added #if-conditionals to exclude Mac (because Ctrl +
scrollwheel does not trigger zoom on Mac).
jdduke (slow)
Per recent discussion with rbyers@ I'll defer to his thoughts on this change.
6 years, 9 months ago
(2014-03-04 18:55:07 UTC)
#17
Per recent discussion with rbyers@ I'll defer to his thoughts on this change.
Rick Byers
On 2014/03/04 18:55:07, jdduke wrote: > Per recent discussion with rbyers@ I'll defer to his ...
6 years, 9 months ago
(2014-03-04 19:25:12 UTC)
#18
Issue 165143003: Handle Ctrl+Mousewheel after renderer
(Closed)
Created 6 years, 10 months ago by robwu
Modified 6 years, 9 months ago
Reviewers: jam, raymes, jdduke (slow), jamesr, Rick Byers
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 1