In bug 814252, roc suggests that we can disable drag detection by invoking preventDefault on a mouse move event. It'll be a general change no matter bug 814252 is depend on it or not. reference: https://bugzilla.mozilla.org/show_bug.cgi?id=814252#c12

Check aEventStatus in nsFrame::HandleDrag and stop the drag detection if mouse move event has been consumed.

Have you tested what other browsers do if the web page calls preventDefault() ? The change is too scary for FF18, so if B2G wants the change for current Beta, it must be in #ifdef MOZ_B2G Coding style for new code is is (expr) { stmt; }

Oh, and for trunk, where this could be without #ifdef MOZ_B2G this needs tests.

Comment on attachment 685947 [details] [diff] [review] prevent drag detection while preventDefault on mouse move event Waiting for tests and information what other browsers do.

(In reply to Olli Pettay [:smaug] from comment #4) > Comment on attachment 685947 [details] [diff] [review] > prevent drag detection while preventDefault on mouse move event > > Waiting for tests and information what other browsers do. roc has done the experiment and here is the result: "preventDefault on a mousemove event disables selection in IE9, but it doesn't in Chrome and Opera." reference: https://bugzilla.mozilla.org/show_bug.cgi?id=814252#c13

(In reply to Olli Pettay [:smaug] from comment #2) > Have you tested what other browsers do if the web page calls > preventDefault() ? > > The change is too scary for FF18, so if B2G wants the change for current > Beta, it must be in #ifdef MOZ_B2G > > Coding style for new code is > is (expr) { > stmt; > } Per roc's suggestion, we should wrap this modification for both beta and aurora. I'll upload another patch for beta and aurora.

(In reply to Olli Pettay [:smaug] from comment #4) > Waiting for tests and information what other browsers do. Do we already have tests for preventDefault preventing selection on mousedown and mouseup? If so we should add tests for mousemove there. Otherwise we should add tests for all of them.

Shih-Chiang, let me know if you want me to find someone to finish this off for you.

roc, I think http://mxr.mozilla.org/mozilla-central/source/layout/generic/test/test_selection_expanding.html?force=1 are testing selection and dragging, maybe I can write test case based on it.

Yes. That particular test doesn't test preventDefault, but you should be able to write a test similar to that one that tests preventDefault for mousedown, mousemove and mouseup and how they affect selection. Thanks!!!

update patch with test case 1. for mousedown, prevent default action will cancel the selection. 2. for mousemove, w3c spec defines mouse move event is not cancelable, therefore, we are unable to cancel the selection. 3. for mouseup, selection state should not be affected since the selection process is finished in mousemove event. Chromium has the same behavior mentioned above. NOTE: touchmove event is cancelable, which is different from mousemove. http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-MouseEvent http://www.w3.org/TR/touch-events/#list-of-touchevent-types

wrap modification with #ifdef MOZ_B2G for beta and aurora.

Comment on attachment 688684 [details] [diff] [review] [m-c] prevent drag detection while preventDefault on mouse move event, v2 I assume there are new tests coming which actually test the changed behavior ;) Those tests will require use of touch events, if mousemove can't be prevented. Sorry, testing this is tricky. netscape.security.PrivilegeManager.enablePrivilege shouldn't be used, but SpecialPowers

(In reply to Shih-Chiang Chien [:schien] from comment #11) > 2. for mousemove, w3c spec defines mouse move event is not cancelable, > therefore, we are unable to cancel the selection. I think we should make it cancellable and get the spec changed. It's hard to imagine that anyone depends on mousemove *not* being cancellable, and here we have a valid use-case for it being cancellable. However, for now, let's just make this patch restrict its check to NS_TOUCH_MOVE events. I think that's all we need for B2G right now. Then the test will need to cover touch events.

note, currently mousemove.preventDefault() is no-op.

1. preventDefault only apply on touch move event 2. add test case for touch move NOTE: I encounter a memory leakage problem in touch move test case and I found that memory leakage is introduced by invoking |synthesizeTouch| if you assign event type in this function call. Do you guys have any suggestion about how to fix this memory leak? :-(

Comment on attachment 689167 [details] [diff] [review] WIP - prevent drag detection while preventDefault on mouse move event, v3 Review of attachment 689167 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand the memory leak problem. How did you detect the memory leak? Can you narrow down what kind of object is leaking?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17) > Comment on attachment 689167 [details] [diff] [review] > WIP - prevent drag detection while preventDefault on mouse move event, v3 > > Review of attachment 689167 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't understand the memory leak problem. How did you detect the memory > leak? Can you narrow down what kind of object is leaking? The memory leakage is done by mochitest framework. I don't have the log in hand, but the leakage includes HTML tags, CSS elements, XPConnect related objects, XPCOMs.

Comment on attachment 689167 [details] [diff] [review] WIP - prevent drag detection while preventDefault on mouse move event, v3 >+ function getSelectionForEditor(aEditorElement) >+ { >+ const nsIDOMNSEditableElement = >+ Components.interfaces.nsIDOMNSEditableElement; >+ return aEditorElement.QueryInterface(nsIDOMNSEditableElement).editor.selection; Does this actually work? At least in web console I get security error if I do all this - as expected.

(In reply to Olli Pettay [:smaug] from comment #19) > Comment on attachment 689167 [details] [diff] [review] > WIP - prevent drag detection while preventDefault on mouse move event, v3 > > > >+ function getSelectionForEditor(aEditorElement) > >+ { > >+ const nsIDOMNSEditableElement = > >+ Components.interfaces.nsIDOMNSEditableElement; > >+ return aEditorElement.QueryInterface(nsIDOMNSEditableElement).editor.selection; > Does this actually work? At least in web console I get security error if I > do all this - as expected. In this WIP, I move these two test case to chrome test, so there is no security error.

(In reply to Shih-Chiang Chien [:schien] from comment #18) > The memory leakage is done by mochitest framework. I don't have the log in > hand, but the leakage includes HTML tags, CSS elements, XPConnect related > objects, XPCOMs. Do you get the leak when you run other tests? Or if you modify your test to not test anything?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21) > (In reply to Shih-Chiang Chien [:schien] from comment #18) > > The memory leakage is done by mochitest framework. I don't have the log in > > hand, but the leakage includes HTML tags, CSS elements, XPConnect related > > objects, XPCOMs. > > Do you get the leak when you run other tests? Or if you modify your test to > not test anything? yep, no leakage when running other tests and removing all the |{type: "touchXXX"}| from synthesizeTouch() will make leakage disappear.

OK. I think we should separate your code change from the test, and check in your code change as soon as it can be reviewed. We don't need to hold that up while we chase memory leaks. It's pretty clear the code change can't cause a memory leak. For the memory leak, I think you should try to find a minimal test file that causes the leak.

Divide code modification and test cases into two patches, I'll create another bug for test cases check-in.

Wrap modification for B2G project only in aurora and beta.

Update bug title because mouse move event is not cancel-able.

Update patch comment because mouse event is not cancel-able.

Comment on attachment 689647 [details] [diff] [review] prevent drag detection while preventDefault on touch move event, v3 Land this to m-c too. I don't want to make this change on non-b2g-m-c without tests.

Comment on attachment 689650 [details] [diff] [review] [m-c] prevent drag detection while preventDefault on touch move event, v3 For m-c and for generic use we really should have tests.

(In reply to Olli Pettay [:smaug] from comment #28) > Comment on attachment 689647 [details] [diff] [review] > [m-b, m-a] prevent drag detection while preventDefault on mouse move event, > v3 > > Land this to m-c too. > I don't want to make this change on non-b2g-m-c without tests. Should I set this bug to bb+ (carry from blocked bug 814252), or request for approval-mozilla-aurora/approval-mozilla-beta?

Try run for b2d53048f2c9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=b2d53048f2c9 Results (out of 251 total builds): exception: 1 success: 189 warnings: 35 failure: 26 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-b2d53048f2c9

Try results don't look good, but I don't understand why those tests fail. Did you have a clean tree when you pushed to try?

(In reply to Olli Pettay [:smaug] from comment #32) > Try results don't look good, but I don't understand why those tests fail. > Did you have a clean tree when you pushed to try? I rebase to latest m-c and push to try server again. Here is the progress: https://tbpl.mozilla.org/?tree=Try&rev=3c037ae9a1d8 Most of the test cases are passed now, so those failures should be independent with this patch.

Try run for 2339bc3a9e2d is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=2339bc3a9e2d Results (out of 18 total builds): exception: 16 failure: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-2339bc3a9e2d

Every touch start event needs a touch end event for cleaning up, memory leakage problem is resolved by adding sufficient touch end event.

Merge test cases with original patch since the memory leakage problem is resolved. This patch is for aurora and beta.

Merge test cases with original patch since memory leakage is resolved. This patch is for mozilla-central.

(In reply to Shih-Chiang Chien [:schien] from comment #35) > Every touch start event needs a touch end event for cleaning up, memory > leakage problem is resolved by adding sufficient touch end event. Please file a bug for that, and it would be great if you can fix it too :-). It shouldn't leak.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38) > (In reply to Shih-Chiang Chien [:schien] from comment #35) > > Every touch start event needs a touch end event for cleaning up, memory > > leakage problem is resolved by adding sufficient touch end event. > > Please file a bug for that, and it would be great if you can fix it too :-). > It shouldn't leak. I filed bug 820228 for tracking this memory leakage issue.

This is needed for bug 814252, so updating tracking data to match.

Comment on attachment 689647 [details] [diff] [review] prevent drag detection while preventDefault on touch move event, v3 Please help check-in this patch to m-c, m-a, and m-b.

Comment on attachment 690340 [details] [diff] [review] [m-b, m-a] prevent drag detection while preventDefault on touch move event, v4 move the test case review to bug 820660, so we can land the required modification earlier.

https://hg.mozilla.org/releases/mozilla-aurora/rev/c73300a19695 https://hg.mozilla.org/releases/mozilla-b2g18/rev/0081c05725b2

It looks like this patch has regressed the synthetic 'contextmenu' event which B2G generates after 1 second of long-holding. Specicifically, the logic at: http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#1073 looks like: 1073 case NS_DRAGDROP_GESTURE: 1074 if (mClickHoldContextMenu) { 1075 // an external drag gesture event came in, not generated internally 1076 // by Gecko. Make sure we get rid of the click-hold timer. 1077 KillClickHoldTimer(); 1078 } and the logic inside nsEventStateManager::GenerateDragGesture at: http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#2076 looks like: 2076 // fire drag gesture if mouse has moved enough 2077 nsIntPoint pt = aEvent->refPoint + aEvent->widget->WidgetToScreenOffset(); 2078 if (std::abs(pt.x - mGestureDownPoint.x) > pixelThresholdX || 2079 std::abs(pt.y - mGestureDownPoint.y) > pixelThresholdY) { 2080 if (mClickHoldContextMenu) { 2081 // stop the click-hold before we fire off the drag gesture, in case 2082 // it takes a long time 2083 KillClickHoldTimer(); 2084 } I'm assuming the explicit decision of this patch to eat move events may be affecting one of these cases. This is causing bug 821864 for the Gaia e-mail app where a user just trying to scroll the message list ends up generating a 'contextmenu' event which causes us to kick into another UI mode. It's pretty funny if you're trying to reproduce the bug, but probably less amusing for people trying to use the app. It appears some other gaia apps may also listen for contextmenu, so they probably break too, but are less obvious about it.

Whats the plan here? This made a bunch of apps unusable. Are we backing out? Do we have a fix for the gaia apps?

if this patch has regressed functionality, why has it not been automatically backed out? it needs to get back out NOW please!

This isn't the bug you're looking for; that's bug 814252.

Try run for 2339bc3a9e2d is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=2339bc3a9e2d Results (out of 19 total builds): exception: 17 failure: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-2339bc3a9e2d

Try run for b2d53048f2c9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=b2d53048f2c9 Results (out of 266 total builds): exception: 1 success: 200 warnings: 38 failure: 27 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-b2d53048f2c9

Try run for 3c037ae9a1d8 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=3c037ae9a1d8 Results (out of 256 total builds): success: 233 warnings: 22 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/schien@mozilla.com-3c037ae9a1d8