Closed Bug 815943 Opened 12 years ago Closed 11 years ago

prevent drag detection if preventDefault is invoked on a touch move event

Categories

(Core :: General, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: schien, Assigned: schien)

References

Details

Attachments

(1 file, 9 obsolete files)

1.25 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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.
Attachment #685947 - Flags: review?(bugs)
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.
Attachment #685947 - Flags: review?(bugs) → review-
(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
Attachment #685947 - Attachment is obsolete: true
Attachment #688684 - Flags: review?(bugs)
Attachment #688684 - Flags: feedback?(roc)
wrap modification with #ifdef MOZ_B2G for beta and aurora.
Attachment #688688 - Flags: review?(bugs)
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
Attachment #688684 - Flags: review?(bugs)
(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? :-(
Attachment #688684 - Attachment is obsolete: true
Attachment #688684 - Flags: feedback?(roc)
Attachment #689167 - Flags: feedback?(roc)
Attachment #689167 - Flags: feedback?(bugs)
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?
Attachment #689167 - Flags: feedback?(roc) → feedback+
(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.
Attachment #689167 - Attachment is obsolete: true
Attachment #689167 - Flags: feedback?(bugs)
Attachment #689644 - Flags: review?(bugs)
Wrap modification for B2G project only in aurora and beta.
Attachment #688688 - Attachment is obsolete: true
Attachment #688688 - Flags: review?(bugs)
Attachment #689647 - Flags: review?(bugs)
Update bug title because mouse move event is not cancel-able.
Summary: prevent drag detection if preventDefault is invoked on a mouse move event → prevent drag detection if preventDefault is invoked on a touch move event
Update patch comment because mouse event is not cancel-able.
Attachment #689644 - Attachment is obsolete: true
Attachment #689644 - Flags: review?(bugs)
Attachment #689650 - Flags: review?(bugs)
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.
Attachment #689647 - Flags: review?(bugs) → review+
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.
Attachment #689650 - Flags: review?(bugs) → review-
(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.
Attachment #689650 - Attachment is obsolete: true
Attachment #689647 - Attachment description: [m-b, m-a] prevent drag detection while preventDefault on mouse move event, v3 → prevent drag detection while preventDefault on touch move event, v3
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.
Attachment #690339 - Flags: review?(bugs)
Merge test cases with original patch since the memory leakage problem is resolved.

This patch is for aurora and beta.
Attachment #689647 - Attachment is obsolete: true
Attachment #690339 - Attachment is obsolete: true
Attachment #690339 - Flags: review?(bugs)
Attachment #690340 - Flags: review?(bugs)
Merge test cases with original patch since memory leakage is resolved.

This patch is for mozilla-central.
Attachment #690341 - Flags: review?(bugs)
(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.
blocking-basecamp: --- → +
Priority: -- → P1
Target Milestone: --- → B2G C2 (20nov-10dec)
Whiteboard: [target:12/21]
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.
Attachment #689647 - Attachment is obsolete: false
Attachment #689647 - Flags: checkin?
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.
Attachment #690340 - Attachment is obsolete: true
Attachment #690340 - Flags: review?(bugs)
Attachment #690341 - Attachment is obsolete: true
Attachment #690341 - Flags: review?(bugs)
https://hg.mozilla.org/mozilla-central/rev/08d0d2e50d5b
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #689647 - Flags: checkin?
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: