Closed Bug 772280 Opened 8 years ago Closed 6 years ago

Text selection will select nearby text when just pressing whitespace

Categories

(Firefox for Android :: Text Selection, defect)

16 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26
Tracking Status
firefox14 --- unaffected
firefox15 - unaffected
firefox16 --- affected
fennec - ---

People

(Reporter: cpeterson, Assigned: gcp)

References

()

Details

Attachments

(1 file, 3 obsolete files)

STR:
1. Load a long page with lots of whitespace, such as https://en.wikipedia.org/wiki/Firefox_for_mobile
2. Tap and hold whitespace between two paragraphs, as if you were preparing to scroll while reading the page

AR:
Some nearby text will be selected, even though you are only pressing whitespace!

ER:
Text should only be selected when you are pressing text.

This bug is really annoying when reading long articles.
This bug is completely due to the "fake mouse" technique we use to create a text selection from mouse coordinates. We'd need platform API support for creating a selection range from x/y coords to make this work better.
This is sorta how text selection works, not sure what we could do differently
tracking-fennec: ? → -
I have a patch that determines the distance between the text that ends up being selected and the actual position the user clicked on screen. I think rejecting selections where the distance between those two is above some threshold will fix this.
Attached patch text-selection-debug (obsolete) — Splinter Review
I set the max distance based on some experimentation. I think this need to take into account device DPI somehow, i.e. the constant should really be a physical distance. Any hints on how to do this?
Attachment #783741 - Flags: review?(mark.finkle)
(In reply to Gian-Carlo Pascutto (:gcp) from comment #5)

> I set the max distance based on some experimentation. I think this need to
> take into account device DPI somehow, i.e. the constant should really be a
> physical distance. Any hints on how to do this?

Bug 745871 is looking into this
For what it's worth I believe this approach will fix bug 782691 as well.
Comment on attachment 783741 [details] [diff] [review]
text-selection-debug

>diff --git a/mobile/android/chrome/content/SelectionHandler.js b/mobile/android/chrome/content/SelectionHandler.js

>+const kDefaultMaxSelectionDistance = 200;

Should this be a pref? I think we did that for other "touch" metrics. I wonder what distance tapping on large chunks of text produce

>+    // Get the current display position
>+    let scrollX = {}, scrollY = {};
>+    this._contentWindow.top.QueryInterface(Ci.nsIInterfaceRequestor).
>+                            getInterface(Ci.nsIDOMWindowUtils).getScrollXY(false, scrollX, scrollY);

I think this is the fastest, non-reflow way of getting the scroll. Thankfully it only happens on start

>+    // Figure out the distance between the selection and the click
>+    let positions = this._getHandlePositions();

Note for later: We get the positions here

>+    let mindist = Math.min(dx1 + dy1, dx2 + dy2);

mindist -> distance

>     this._activeType = this.TYPE_SELECTION;
>     this._positionHandles();

This calls _getHandlePositions again. Is that expensive? Again, this is not in a loop so it probably doesn't matter. Just sanity checking.
>+  _getHandlePositions: function sh_getHandlePositions() {
>     let scrollX = {}, scrollY = {};
>     this._contentWindow.top.QueryInterface(Ci.nsIInterfaceRequestor).
>                             getInterface(Ci.nsIDOMWindowUtils).getScrollXY(false, scrollX, scrollY);

It always pains me a little to see the same code being called, just hidden by other calls (_getHandlePositions is called above and so is getScrollXY)

>+  _positionHandles: function sh_positionHandles() {

>+    if (positions) {q

Typo

f+ from me, passing to Margaret for better review. I'm not sure there are simple ways to address my nits. Maybe it's not worth it.
Attachment #783741 - Flags: review?(mark.finkle)
Attachment #783741 - Flags: review?(margaret.leibovic)
Attachment #783741 - Flags: feedback+
>I wonder what distance tapping on large chunks of text produce

Ha, that's an excellent point. I think this patch will fail to select very large words, because the handles will be far from the click point.

Distance should probably be measured to the center of the rectangle formed by both resize handles. That's an easy fix, though.
Reworked patch:
1) Avoid double calls by refactoring code.
2) Made the selection distance a pref.
3) Measure distance to point of mass of selection rather than closest handle.
Attachment #783741 - Attachment is obsolete: true
Attachment #783741 - Flags: review?(margaret.leibovic)
Attachment #784896 - Flags: review?(margaret.leibovic)
Attachment #784896 - Flags: feedback?(mark.finkle)
Comment on attachment 784896 [details] [diff] [review]
Patch 1. v2 Don't select text that's not where we click

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

This is looking good to me, but I think there are some improvements we should make before landing it.

Also, have you checked how this works in iframes and textareas? Those are often sources of edge case bugs in this part of the code. Here's a test page you might want to use:
http://people.mozilla.com/~mleibovic/test/text_select.html

::: mobile/android/app/mobile.js
@@ +437,5 @@
>  
>  // The percentage of the screen that needs to be scrolled before margins are exposed.
>  pref("browser.ui.show-margins-threshold", 20);
>  
> +pref("browser.ui.selection.distance", 250);

Add a comment about what this is.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +214,5 @@
> +    let maxSelectionDistance = Services.prefs.getIntPref("browser.ui.selection.distance");
> +    // Do not select text far away from where the user clicked
> +    if (distance > maxSelectionDistance) {
> +      // Clear out any existing active selection
> +      this._closeSelection();

Does this even work? It looks like we bail early in _closeSelection if _activeType is TYPE_NONE, which is probably is here, since we're setting it to TYPE_SELECTION down below.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#422

I think you'll need to move all this logic below where we set this._activeType.

@@ +216,5 @@
> +    if (distance > maxSelectionDistance) {
> +      // Clear out any existing active selection
> +      this._closeSelection();
> +      // Clear any existing selection from the document
> +      this._contentWindow.getSelection().removeAllRanges();

You should use the _getSelection() helper method we already have. That will also take care of correctly getting the selection if this is inside an editable element.

@@ +578,2 @@
>  
> +  _positionHandles: function sh_positionHandles(positions) {

It could be nice to add a comment here explaining that positions is an array of objects with data about handle positions, which we always get from _getHandlePositions.

@@ +578,3 @@
>  
> +  _positionHandles: function sh_positionHandles(positions) {
> +    if (positions) {

We don't expect to ever call this with a null/underfined positions argument, so you can get rid of this check.

Better yet, you could check to see if positions is null/undefined, and if it is, call this._getHandlePositions(this._getScrollPos()). This would let you avoid modifying all the existing places we call this._positionHandles() where we don't need the positions.
Attachment #784896 - Flags: review?(margaret.leibovic) → feedback+
Another tweak to make: if the touch was inside the bounding rectangle of the selection handles, always accept the selection. This way you can still select a very long text by clicking on one end of it.
1) Addressed review comments.
2) Changed logic so if the click is inside the selection, we always accept.

This seems to work correctly on that test page, even inside the scrolled textbox.
Attachment #784896 - Attachment is obsolete: true
Attachment #784896 - Flags: feedback?(mark.finkle)
Attachment #785819 - Flags: review?(margaret.leibovic)
Comment on attachment 785819 [details] [diff] [review]
Patch 1. v3 Don't select text that's not where we click

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

Almost there, but I want to make sure we clean up properly when bailing on starting the selection.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +222,5 @@
> +    let maxSelectionDistance = Services.prefs.getIntPref("browser.ui.selection.distance");
> +    // Do not select text far away from where the user clicked
> +    if (distance > maxSelectionDistance) {
> +      // Clear any existing selection from the document
> +      selection.removeAllRanges();

I think you should still call _closeSelection(), since it removes observers and event listeners that are added earlier in this startSelection function.

If you're worried about it doing unnecessary work, you could modify it to only optionally send the HideHandles message, but it looks like we need to do everything else in there.
Attachment #785819 - Flags: review?(margaret.leibovic) → review-
Reworked after some discussion on IRC that found some existing bugs in the code.
Attachment #785819 - Attachment is obsolete: true
Attachment #785840 - Flags: review?(margaret.leibovic)
Comment on attachment 785840 [details] [diff] [review]
Patch 1. v4 Don't select text that's not where we click

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

Nice, thanks.
Attachment #785840 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/612f8a4c8178
Assignee: nobody → gpascutto
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Duplicate of this bug: 826601
You need to log in before you can comment on or make changes to this bug.