The default behavior for selecting text when there is a mousedown followed by a drag action does not make sense on device where this is the common action use to pan a page and where there is usually helpers for text selection. In order to disable this feature living on nsFrame.cpp::HandleEvent, the patch introduced a new build flag to disable it at build time. I have not used a pref because it's something that broke all the text selection if it's set by an addon at runtime and I've also though that it was a bad idea to check for pref for every nsFrame.

Comment on attachment 579109 [details] [diff] [review] disable click and drag selection Review of attachment 579109 [details] [diff] [review]: ----------------------------------------------------------------- checking a pref once per event should not be a performance issue. So I say, go with the pref. ::: layout/generic/nsFrame.cpp @@ +2145,5 @@ > + aEvent->message == NS_MOUSE_BUTTON_DOWN && > + static_cast<nsMouseEvent*>(aEvent)->button == nsMouseEvent::eLeftButton) > + HandlePress(aPresContext, aEvent, aEventStatus); > + return NS_OK; > +#endif I'm not sure why this block is needed. @@ +2544,5 @@ > + > + nsRefPtr<nsFrameSelection> fc = const_cast<nsFrameSelection*>(frameselection); > + bool shift = false; > + bool control = false; > +#endif I'm also not sure why this chunk is needed. Why aren't we just taking the "if (!selectable)" early-exit path here?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1) > Comment on attachment 579109 [details] [diff] [review] [diff] [details] [review] > disable click and drag selection > > Review of attachment 579109 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > checking a pref once per event should not be a performance issue. So I say, > go with the pref. Nice I will change that. > ::: layout/generic/nsFrame.cpp > @@ +2145,5 @@ > > + aEvent->message == NS_MOUSE_BUTTON_DOWN && > > + static_cast<nsMouseEvent*>(aEvent)->button == nsMouseEvent::eLeftButton) > > + HandlePress(aPresContext, aEvent, aEventStatus); > > + return NS_OK; > > +#endif > > I'm not sure why this block is needed. The mousedown needs to be handle to allow repositioning the caret while clicking. Without the frameSelection->HandleClick(...) call it's impossible to move it. > > @@ +2544,5 @@ > > + > > + nsRefPtr<nsFrameSelection> fc = const_cast<nsFrameSelection*>(frameselection); > > + bool shift = false; > > + bool control = false; > > +#endif > > I'm also not sure why this chunk is needed. > > Why aren't we just taking the "if (!selectable)" early-exit path here? see above. If this explanation make sense to you i will address your comments and upload a new patch for review.

(In reply to Vivien Nicolas (:vingtetun) from comment #2) > > ::: layout/generic/nsFrame.cpp > > @@ +2145,5 @@ > > > + aEvent->message == NS_MOUSE_BUTTON_DOWN && > > > + static_cast<nsMouseEvent*>(aEvent)->button == nsMouseEvent::eLeftButton) > > > + HandlePress(aPresContext, aEvent, aEventStatus); > > > + return NS_OK; > > > +#endif > > > > I'm not sure why this block is needed. > > The mousedown needs to be handle to allow repositioning the caret while > clicking. Without the frameSelection->HandleClick(...) call it's impossible > to move it. I still don't see why this block is needed. Without this block we'll call HandlePress for this event anyway. In HandlePress, I think it would be clearer to check for the "move caret only" pref and when it's set, explicitly call fc->HandleClick with false for aContinueSelection and aMultipleSelection, then return early. And add a comment explaining what's going on.

> checking a pref once per event should not be a performance issue. So I say, go with the > pref. You can also use Preferences::AddBoolVarCache.

For Native Fennec, I'm taking the approach of just not sending mouse events for touches on the screen UNLESS we are faking a click of some sort. To be honest, I think that makes more sense. Touches are not mouse events.

(In reply to Wesley Johnston (:wesj) from comment #5) > For Native Fennec, I'm taking the approach of just not sending mouse events > for touches on the screen UNLESS we are faking a click of some sort. To be > honest, I think that makes more sense. Touches are not mouse events. I thought the spec was saying that touchevents and mousevents should be fired unless event.preventDefault() is called on touchstart or on touchmove? http://www.w3.org/TR/2011/WD-touch-events-20110505/#mouse-events

This version use a preference called browser.ignoreNativeFrameTextSelection.

(In reply to Vivien Nicolas (:vingtetun) from comment #6) > (In reply to Wesley Johnston (:wesj) from comment #5) > > For Native Fennec, I'm taking the approach of just not sending mouse events > > for touches on the screen UNLESS we are faking a click of some sort. To be > > honest, I think that makes more sense. Touches are not mouse events. > > I thought the spec was saying that touchevents and mousevents should be > fired unless event.preventDefault() is called on touchstart or on touchmove? > > http://www.w3.org/TR/2011/WD-touch-events-20110505/#mouse-events Trying to not be stop energy (and was out last week). I took that as a "may" (which is what it says) and the real behavior just isn't well defined. I'm not sure I 100% understand what "Default Action" means in this chart, but I took it to be mouse events should only fire on touchend: http://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html#list-of-touchevent-types In XUL Fennec we sldo (typically) avoided sending most mouse events to pages unless we are performing a click.

Comment on attachment 582239 [details] [diff] [review] Patch Review of attachment 582239 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +2470,5 @@ > + > + if (NS_FAILED(rv)) > + return rv; > + > + return NS_OK; just return rv in all cases. or even just "return fc->HandleClick"...