851521 - Issues with errant selection when panning (ignoreNativeFrameTextSelection support)

Status: RESOLVED INCOMPLETE

(Firefox for Metro Graveyard :: Input, defect)

Description

[Jim Mathies [:jimm]]

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.

Comment 1

[Jim Mathies [:jimm]]

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

[(no longer active)]

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

Comment 3

[Jim Mathies [:jimm]]

(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.

Comment 4

[Jim Mathies [:jimm]]

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.

Comment 5

[Brian R. Bondy [:bbondy]]

Based on Comment 4 moving to v2 triage

Comment 6

[Asa Dotzler [:asa]]

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?

Comment 7

[Jim Mathies [:jimm]]

(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.

Comment 8

[Jim Mathies [:jimm]]

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

Comment 9

[Jim Mathies [:jimm]]

Marco, we need to bring this out and add it to the next iteration.

Comment 10

[Jim Mathies [:jimm]]

(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.

Comment 11

[Jim Mathies [:jimm]]

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

Comment 12

[Jim Mathies [:jimm]]

Attachment: proposed interface

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.

Comment 13

[Jim Mathies [:jimm]]

Attachment: proposed interface v.2

Updated with new getter and setter methods for input mode.

Comment 14

[(no longer active)]

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?

Comment 15

[Jim Mathies [:jimm]]

(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

[(no longer active)]

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.

Comment 17

[Allison Naaktgeboren :ally]

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).

Comment 18

[Firefox Bug Husbandry Bot]

Mass close of bugs in obsolete product https://bugzilla.mozilla.org/show_bug.cgi?id=1350354