The default bug view has changed. See this FAQ.

Disable 'click and drag' text selection in nsFrame.cpp for touch enable devices

RESOLVED FIXED in mozilla11

Status

()

Core
Selection
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Trunk
mozilla11
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Attachment #579109 - Flags: review?(roc)
Summary: selection → Disable 'click and drag' text selection in nsFrame.cpp for touch enable devices
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
Created attachment 582239 [details] [diff] [review]
Patch

This version use a preference called browser.ignoreNativeFrameTextSelection.
Assignee: nobody → 21
Attachment #579109 - Attachment is obsolete: true
Attachment #579109 - Flags: review?(roc)
Attachment #582239 - Flags: review?(roc)
(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"...
Attachment #582239 - Flags: review?(roc) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/7ad814d9c44e
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla11
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/7ad814d9c44e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.