Bug 88270 - EventHandler shouldn't dispatch fake mousemove events when scrolling on devices that don't have a mouse
Summary: EventHandler shouldn't dispatch fake mousemove events when scrolling on devic...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords: InRadar
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-06-04 17:06 PDT by Adam Barth
Modified: 2012-06-06 02:45 PDT (History)
13 users (show)

See Also:


Attachments
Patch (6.47 KB, patch)
2012-06-04 17:08 PDT, Adam Barth
jamesr: review-
Details | Formatted Diff | Diff
Patch (12.51 KB, patch)
2012-06-04 17:58 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (12.71 KB, patch)
2012-06-04 18:16 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-06-04 17:06:41 PDT
EventHandler shouldn't dispatch fake mouse events on devices that don't have a mouse
Comment 1 Adam Barth 2012-06-04 17:08:17 PDT
Created attachment 145660 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-04 17:09:48 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 James Robinson 2012-06-04 17:13:34 PDT
Comment on attachment 145660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145660&action=review

> Source/WebCore/ChangeLog:3
> +        EventHandler shouldn't dispatch fake mouse events on devices that don't have a mouse

this title is a bit misleading - it seems that the code change is only changing synthetic mouse move events, which is a much more limited thing.  we still want to fire (say) "click" events on taps, right?

> Source/WebCore/ChangeLog:8
> +        This patch adds a setting analogous to deviceSupportsTouch to determine

where da tests at, homie?
Comment 4 James Robinson 2012-06-04 17:13:54 PDT
Code change seems quite reasonable, but tests are the bests
Comment 5 Alexandre Elias 2012-06-04 17:16:59 PDT
Thanks for the quick patch.

One nuance is that some Android devices support plugging in a mouse, for example the Xoom.  Is dynamically toggling a setting permitted, or must it be on startup?

I'm also concerned like James that the setting as currently named may end up overused to disable more functionality than we intended.
Comment 6 James Robinson 2012-06-04 17:24:48 PDT
Comment on attachment 145660 [details]
Patch

Alex raises a good point about dynamically changing - Settings is normally something that can't change over the lifetime of a Page, but this definitely does need to change.  That indicates that it probably should be routed in a different way.

A similar argument could apply (although it's much less common) to deviceSupportsTouch - someone _could_ plug a touchscreen monitor in while a browser is running, but that seems pretty far fetched.
Comment 7 Adam Barth 2012-06-04 17:31:04 PDT
You can change Settings whenever you want.  A setting like this one is going to be queried each time its needed rather than cached, so it should work fine.
Comment 8 Alexandre Elias 2012-06-04 17:32:41 PDT
Sounds good.  Since this will change in a very modal way, I'm fine with it being a setting if that fits WebKit conventions.
Comment 9 James Robinson 2012-06-04 17:37:59 PDT
Hmm, that's not generally true for Settings - various bits of code stash settings bits in various places without re-checking or handling transitions, so it's really hard for someone looking at the code or looking at an embedding layer to know if it's safe to change a Setting or not..  I think we've discussed this dichotomy before but didn't get anywhere satisfactory.

Anyway, with tests this patch would be fine IMO.
Comment 10 Adam Barth 2012-06-04 17:53:35 PDT
I have a test locally, I'm just making sure it passes.  :)
Comment 11 Adam Barth 2012-06-04 17:58:23 PDT
Created attachment 145668 [details]
Patch
Comment 12 James Robinson 2012-06-04 18:06:34 PDT
Comment on attachment 145668 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145668&action=review

> LayoutTests/fast/events/touch/scroll-without-mouse-lacks-mousemove-events.html:13
> +function listener() {

I'm not sure that this test would actually fail without your code change (or without the window.internals.settings call), since synthetic mouse moves are dispatched asynchronously and by default DRT ends the test as soon as the load event fires - so there wouldn't be any opportunity to fire the event that fails the test.
Comment 13 Adam Barth 2012-06-04 18:08:54 PDT
(In reply to comment #12)
> (From update of attachment 145668 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145668&action=review
> 
> > LayoutTests/fast/events/touch/scroll-without-mouse-lacks-mousemove-events.html:13
> > +function listener() {
> 
> I'm not sure that this test would actually fail without your code change (or without the window.internals.settings call), since synthetic mouse moves are dispatched asynchronously and by default DRT ends the test as soon as the load event fires - so there wouldn't be any opportunity to fire the event that fails the test.

You're right.  I tested it in the browser, but not in DRT.  One moment.
Comment 14 Adam Barth 2012-06-04 18:16:19 PDT
Created attachment 145672 [details]
Patch
Comment 15 Adam Barth 2012-06-04 18:17:00 PDT
It's not clear how to write a great test in this case since we're trying to prove that an async even never happens.  This test should pass reliably though.
Comment 16 James Robinson 2012-06-04 18:23:59 PDT
Comment on attachment 145672 [details]
Patch

You have to race it, yeah.  This seems pretty good.
Comment 17 WebKit Review Bot 2012-06-05 00:39:16 PDT
Comment on attachment 145672 [details]
Patch

Clearing flags on attachment: 145672

Committed r119465: <http://trac.webkit.org/changeset/119465>
Comment 18 WebKit Review Bot 2012-06-05 00:39:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Antonio Gomes 2012-06-05 04:36:31 PDT
Sorry, for jumping in late, but I am not sure this is the best way to handle this problem.

We (blackberry port) also suffer from this, but it is not desirable to fire mouse move events when (and only when) the user it performing a scrolling or zooming action. On the other hand, when the user is interacting with a webpage that handles the mouse-move events (with a listener/callback for example), the events should be fired.
Comment 20 Rick Byers 2012-06-05 09:43:52 PDT
(In reply to comment #6)
> (From update of attachment 145660 [details])
> Alex raises a good point about dynamically changing - Settings is normally something that can't change over the lifetime of a Page, but this definitely does need to change.  That indicates that it probably should be routed in a different way.
> 
> A similar argument could apply (although it's much less common) to deviceSupportsTouch - someone _could_ plug a touchscreen monitor in while a browser is running, but that seems pretty far fetched.

A less far-fetched scenario here that we'd like to support is moving a window between a touch screen and 2nd monitor / projector that doesn't have touch.  Ideally pointer/hover media queries should update in that case.  This has been on my TODO list - I filed https://bugs.webkit.org/show_bug.cgi?id=88339 to track.
Comment 21 Rick Byers 2012-06-05 09:47:34 PDT
(In reply to comment #19)
> Sorry, for jumping in late, but I am not sure this is the best way to handle this problem.
> 
> We (blackberry port) also suffer from this, but it is not desirable to fire mouse move events when (and only when) the user it performing a scrolling or zooming action. On the other hand, when the user is interacting with a webpage that handles the mouse-move events (with a listener/callback for example), the events should be fired.

We believe firing mouse move events for touch in general is not possible.  Among other issues, it causes ambiguity around scrolling behavior.  If a page does preventDefault on mousemove should that prevent scrolling?  If not, how do they move the mouse up/down relative to a page with touch that supports scrolling?

In general, we (folks working on touch for chromium) believe that trying to emulate a mouse with touch (beyond click generation already done today) will break more things than it'll help.  More details here: http://code.google.com/p/chromium/issues/detail?id=115485
Comment 22 Antonio Gomes 2012-06-05 10:02:15 PDT
> We believe firing mouse move events for touch in general is not possible.  Among other issues, it causes ambiguity around scrolling behavior.  If a page does preventDefault on mousemove should that prevent scrolling?  If not, how do they move the mouse up/down relative to a page with touch that supports scrolling?

In our model it does prevent scrolling. That is how we support most of the mouse-oriented browsers with our touch-screen browser. We also have in blackberry devices

touch screens and track pads (mouse-like pointer input).

> In general, we (folks working on touch for chromium) believe that trying to emulate a mouse with touch (beyond click generation already done today) will break more things than it'll help.  More details here: http://code.google.com/p/chromium/issues/detail?id=115485

Well, the solution as one pointed out is based on a Setting that should in theory no change in the life time of a Page. In this case, we need it at least to be dynamic.
Comment 23 Eli Fidler 2012-06-05 10:04:55 PDT
We (BlackBerry) have been emulating mouse with touch for a while now, so this change breaks us. It's definitely tricky to get reasonable behaviour out of pages designed for mouse interaction using touch, but I don't agree that "firing mouse move events for touch in general is not possible" or that "trying to emulate a mouse with touch (beyond click generation already done today) will break more things than it'll help".
Comment 24 Rick Byers 2012-06-05 10:23:53 PDT
(In reply to comment #23)
> We (BlackBerry) have been emulating mouse with touch for a while now, so this change breaks us. It's definitely tricky to get reasonable behaviour out of pages designed for mouse interaction using touch, but I don't agree that "firing mouse move events for touch in general is not possible" or that "trying to emulate a mouse with touch (beyond click generation already done today) will break more things than it'll help".

Cool!  In the short term, it sounds like we need to make sure WebCore supports both policies then.  In the longer term we should try to figure out what the right model for the web is.  Let's follow-up outside the context of this bug on that - I'll send you guys an e-mail.
Comment 25 James Robinson 2012-06-05 10:52:44 PDT
(In reply to comment #23)
> We (BlackBerry) have been emulating mouse with touch for a while now, so this change breaks us.

This is impossible - this patch only changes behavior if you set a non-default Setting, which you can't already be doing since it just landed yesterday.
Comment 26 Antonio Gomes 2012-06-05 11:10:57 PDT
(In reply to comment #25)
> (In reply to comment #23)
> > We (BlackBerry) have been emulating mouse with touch for a while now, so this change breaks us.
> 
> This is impossible - this patch only changes behavior if you set a non-default Setting, which you can't already be doing since it just landed yesterday.

That is not the point, James.

If we do not set the setting (that landed yesterday) as you said, we will still get the mouse move events being fired when we pan or flick scroll, for example, that we do not want, which is what the bug is about. But if we set it, it will break our behavior.

So the point is, patch does not actually addresses the issue in a way that it works for us (the blackberry port), which I think is a valid reason to have this discussion.
Comment 27 James Robinson 2012-06-05 11:18:41 PDT
(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #23)
> > > We (BlackBerry) have been emulating mouse with touch for a while now, so this change breaks us.
> > 
> > This is impossible - this patch only changes behavior if you set a non-default Setting, which you can't already be doing since it just landed yesterday.
> 
> That is not the point, James.
> 

You said this patch breaks you.  This patch doesn't change any behavior for you, so that's clearly incorrect.  Please be precise.

> If we do not set the setting (that landed yesterday) as you said, we will still get the mouse move events being fired when we pan or flick scroll, for example, that we do not want, which is what the bug is about. But if we set it, it will break our behavior.
> 
> So the point is, patch does not actually addresses the issue in a way that it works for us (the blackberry port), which I think is a valid reason to have this discussion.

I think it's fine to discuss better ways to address this problem.  Please file a new bug describing the behaviors you want and we can figure out how to get a sane, consistent behavior.
Comment 28 Antonio Gomes 2012-06-05 11:21:38 PDT
> You said this patch breaks you.  This patch doesn't change any behavior for you, so that's clearly incorrect.  Please be precise.

Could you point out where I mentioned it breaks the BlackBerry port?
 
> > So the point is, patch does not actually addresses the issue in a way that it works for us (the blackberry port), which I think is a valid reason to have this discussion.
> 
> I think it's fine to discuss better ways to address this problem.  Please file a new bug describing the behaviors you want and we can figure out how to get a sane, consistent behavior.

Fair
Comment 29 James Robinson 2012-06-05 11:27:00 PDT
(In reply to comment #23) (from Eli Fidler)
> We (BlackBerry) have been emulating mouse with touch for a while now, so this change breaks us.
Comment 30 Adam Barth 2012-06-05 11:39:21 PDT
I'm sorry, but I don't understand the discussion that ensued after this patch landed.  If there was something important for me to understand, would you be willing to explain it again?  As far as I can tell, this change can't possibly cause any problems for anyone.
Comment 31 Andy Estes 2012-06-05 17:07:13 PDT
Could we improve this by stopping m_fakeMouseMoveEventTimer from even being scheduled if our platform doesn't support mouse input? Seems unnecessary to schedule events that we know will be no-ops.
Comment 32 James Robinson 2012-06-05 17:11:35 PDT
(In reply to comment #31)
> Could we improve this by stopping m_fakeMouseMoveEventTimer from even being scheduled if our platform doesn't support mouse input? Seems unnecessary to schedule events that we know will be no-ops.

That sounds like a great thing for you to file a new bug about.
Comment 33 Andy Estes 2012-06-05 17:16:25 PDT
(In reply to comment #32)
> (In reply to comment #31)
> > Could we improve this by stopping m_fakeMouseMoveEventTimer from even being scheduled if our platform doesn't support mouse input? Seems unnecessary to schedule events that we know will be no-ops.
> 
> That sounds like a great thing for you to file a new bug about.

Gladly: https://bugs.webkit.org/show_bug.cgi?id=88379
Comment 34 David Kilzer (:ddkilzer) 2012-06-06 02:45:34 PDT
<rdar://problem/11602415>