Bug 88270

Created attachment 145660 [details] Patch

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 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?

Code change seems quite reasonable, but tests are the bests

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 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.

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.

Sounds good. Since this will change in a very modal way, I'm fine with it being a setting if that fits WebKit conventions.

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.

I have a test locally, I'm just making sure it passes. :)

Created attachment 145668 [details] Patch

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.

(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.

Created attachment 145672 [details] Patch

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 on attachment 145672 [details] Patch You have to race it, yeah. This seems pretty good.

Comment on attachment 145672 [details] Patch Clearing flags on attachment: 145672 Committed r119465: <http://trac.webkit.org/changeset/119465>

All reviewed patches have been landed. Closing bug.

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.

(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.

(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

> 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.

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".

(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.

(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.

(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.

(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.

> 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

(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.

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.

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.

(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.

(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

<rdar://problem/11602415>