Last Comment Bug 707734 - Disable 'click and drag' text selection in nsFrame.cpp for touch enable devices
: Disable 'click and drag' text selection in nsFrame.cpp for touch enable devices
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla11
Assigned To: Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-05 10:02 PST by Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
Modified: 2011-12-19 08:36 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
disable click and drag selection (6.54 KB, patch)
2011-12-05 10:02 PST, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch (3.04 KB, patch)
2011-12-16 04:13 PST, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
roc: review+
Details | Diff | Splinter Review

Description Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-05 10:02:19 PST
Created attachment 579109 [details] [diff] [review]
disable click and drag selection

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 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-05 18:19:54 PST
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?
Comment 2 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-05 18:45:58 PST
(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.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-05 19:33:50 PST
(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.
Comment 4 Justin Lebar (not reading bugmail) 2011-12-05 22:03:34 PST
> 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.
Comment 5 Wesley Johnston (:wesj) 2011-12-07 23:18:50 PST
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.
Comment 6 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-15 02:06:49 PST
(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
Comment 7 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-16 04:13:59 PST
Created attachment 582239 [details] [diff] [review]
Patch

This version use a preference called browser.ignoreNativeFrameTextSelection.
Comment 8 Wesley Johnston (:wesj) 2011-12-16 05:58:52 PST
(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 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-16 21:11:13 PST
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"...
Comment 10 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-19 01:12:08 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/7ad814d9c44e
Comment 11 Marco Bonardo [::mak] 2011-12-19 08:36:11 PST
https://hg.mozilla.org/mozilla-central/rev/7ad814d9c44e

Note You need to log in before you can comment on or make changes to this bug.