Issues with errant selection when panning (ignoreNativeFrameTextSelection support)

RESOLVED INCOMPLETE

Status

defect
RESOLVED INCOMPLETE
6 years ago
2 years ago

People

(Reporter: jimm, Unassigned)

Tracking

Trunk
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=0)

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

6 years ago
This is currently a pref we read in the fly down in frame selection code. See bug 768503 for info. We want to be able to disable drag selection on the fly when the user is interacting with touch, and flip it back when the user is interacting with the mouse. 

I suppose we could just flip the pref back and forth. Although it might be better to expose this through some sort of an interface.
Reporter

Comment 1

6 years ago
Tweaking this should allow us to get rid of the this tap trap:

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/input.js#145

since selection won't get accidentally created when the user is interacting with touch.

Comment 2

6 years ago
Modifying a pref as an API is not great!  Let's add whatever API you guys need.
Reporter

Comment 3

6 years ago
(In reply to Jim Mathies [:jimm] from comment #1)
> Tweaking this should allow us to get rid of the this tap trap:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> input.js#145
> 
> since selection won't get accidentally created when the user is interacting
> with touch.

In testing this isn't true. Selection gets invoked here from a double tap. But I have seen errant drag selection sometimes when trying to pan, so I think we still want this.
Reporter

Comment 4

6 years ago
Kicking this out since it's not caret selection specific. If we find this to be a big problem we'll come back to it.
No longer blocks: caret-sel
Summary: toggle ignoreNativeFrameTextSelection support based on input type → Issues with errant selection when panning (ignoreNativeFrameTextSelection support)
Based on Comment 4 moving to v2 triage
Blocks: metrov2planning
No longer blocks: metrov1triage
What is the user impact here? Under what scenarios do we get a broken behavior and what are the user recovery options when we do break?
Reporter

Comment 7

6 years ago
(In reply to Asa Dotzler [:asa] from comment #6)
> What is the user impact here? Under what scenarios do we get a broken
> behavior and what are the user recovery options when we do break?

I haven't seen this in a while but it may still be a problem. The side effect would be trying to scroll using touch and inadvertently triggering content selection instead. I ran into this a lot before we did the events overlay overhaul. Now not so much.
Reporter

Updated

6 years ago
Whiteboard: [selection]
Reporter

Comment 8

6 years ago
Now that we've removed the selection event trap I'm seeing this a log more. Both for clicks and drag.
Reporter

Comment 9

6 years ago
Marco, we need to bring this out and add it to the next iteration.
Flags: needinfo?(mmucci)
Reporter

Comment 10

6 years ago
(In reply to Jim Mathies [:jimm] from comment #9)
> Marco, we need to bring this out and add it to the next iteration.

Actually never mind. I might be able to address the bug I'm working on in a different way.
Flags: needinfo?(mmucci)
Reporter

Updated

6 years ago
Blocks: 860248
Reporter

Comment 11

6 years ago
Filed bug 860248 which describes the problem. Once we define the behavior we want we can figure out how to fix it.
Reporter

Comment 12

6 years ago
Posted patch proposed interface (obsolete) — Splinter Review
If I do expose ignoreNativeFrameTextSelection, I'm thinking this is the type of interface I'd use. Front end would have to maintain proper state on all of its windows, but that shouldn't be too big a deal. (I've also taken the opportunity here to better comment nsISelectionController.)

Setting SELECTION_INPUT_DISABLECLICKDRAG equates with having 'browser.ignoreNativeFrameTextSelection' set provided the front end sets this mode for every window.
Attachment #735725 - Flags: feedback?(ehsan)
Reporter

Comment 13

6 years ago
Updated with new getter and setter methods for input mode.

Comment 14

6 years ago
Comment on attachment 735725 [details] [diff] [review]
proposed interface

Review of attachment 735725 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsISelectionController.idl
@@ +56,5 @@
> +   // still move the caret position and hence, alter selected content.
> +   const short SELECTION_INPUT_DISABLECLICKDRAG = 1;
> +   // Ignore mouse clicks. Useful on mobile devices that manipulate
> +   // selection from front end code.
> +   const short SELECTION_INPUT_DISABLECLICKS = 2;

Which function is supposed to be passed these values?
Reporter

Comment 15

6 years ago
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> Comment on attachment 735725 [details] [diff] [review]
> proposed interface
> 
> Review of attachment 735725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/public/nsISelectionController.idl
> @@ +56,5 @@
> > +   // still move the caret position and hence, alter selected content.
> > +   const short SELECTION_INPUT_DISABLECLICKDRAG = 1;
> > +   // Ignore mouse clicks. Useful on mobile devices that manipulate
> > +   // selection from front end code.
> > +   const short SELECTION_INPUT_DISABLECLICKS = 2;
> 
> Which function is supposed to be passed these values?

Sorry, see the follow up - I've added setInputSelection/getInputSelection.

Comment 16

6 years ago
Comment on attachment 735736 [details] [diff] [review]
proposed interface v.2

Review of attachment 735736 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsISelectionController.idl
@@ +84,5 @@
> +
> +   /**
> +    * getInputSelection returns the display mode for the selection.
> +    */
> +    short   getInputSelection();

I'd make this an attribute and call it inputSelectionMode.
Attachment #735736 - Flags: feedback+

Updated

6 years ago
Attachment #735725 - Flags: feedback?(ehsan)
Reporter

Updated

6 years ago
Attachment #735725 - Attachment is obsolete: true
Reporter

Updated

6 years ago
No longer blocks: metrov2planning
Reporter

Updated

6 years ago
No longer blocks: 860248
We're interested in re-viving part of this for v1 in order to get doubletap to zoom(bug 895581) to work. Right now double tapping produces the desired message but the undesired side effect of selecting text as well(bug 936754).
Reporter

Updated

5 years ago
Blocks: 957244
OS: Windows 8 Metro → Windows 8.1
Whiteboard: [selection]
Blocks: metrobacklog
Whiteboard: p=0
Mass close of bugs in obsolete product https://bugzilla.mozilla.org/show_bug.cgi?id=1350354
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.