670222 - Accidental text selection

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Benjamin Stover (:stechz)]

During general browsing on Fennec, I often accidentally copy text when I don't mean to.

Comment 1

[Benjamin Stover (:stechz)]

To be clear, I never really see any text selection handles or anything. I just see "text copied to clipboard" and I have no idea why.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

The text selection feature is tied to the long tap mechanism. Context menus are also tied to long tap. Do you get the same issue if you pan around a page full of links or images?

I have never seen this issue on my Nexus One or Galaxy Tab, so it is hard to debug. We could add some logging into the code to see what is causing the problem.

You say you see the "Text copied" popup, but not the handles. That could narrow down the problem.

Just to be clear, I do not want a secondary mechanism used to trigger text selection. We use the same trigger as context menus and I mean to keep it that way.

Comment 3

[Martijn Wargers (dead)]

I think 3 situations where I hit accidental text selection:
- When long tapping a link, to open it in a new tab. It is often difficult to hit links with my fat fingers on those tiny links.
- When keeping my finger on the screen, not immediately deciding to scroll down or upwards on a page.
- When trying to scroll very slowly, by moving the finger very slowly on the screen.

Comment 4

[Martijn Wargers (dead)]

Just to make more clear, I'm accidentally hitting text selection too, often when I try to open a link in a new tab or try to scroll the page.

I don't need selection all that much, only occasionally. In my opinion, it should be moved to a context menu-item that pops up when long-tapping text.

Are other mobile browsers doing the same thing, triggering text selection on long tap?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #4)

> Are other mobile browsers doing the same thing, triggering text selection on
> long tap?

Stock android browser on Gingerbread

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

The behavior I forgot to implement: Only copy to clipboard if the user taps on the selection. If the user taps anywhere else, cancel the clipboard operation.

Comment 7

[Paul [pwd]]

(In reply to comment #6)
> The behavior I forgot to implement: Only copy to clipboard if the user taps
> on the selection. If the user taps anywhere else, cancel the clipboard
> operation.

In that case, wouldn't it be more intuitive to have a popup where a user can click copy?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #7)
> (In reply to comment #6)
> > The behavior I forgot to implement: Only copy to clipboard if the user taps
> > on the selection. If the user taps anywhere else, cancel the clipboard
> > operation.
> 
> In that case, wouldn't it be more intuitive to have a popup where a user can
> click copy?

Different bug. Let's fix the current bug before adding bells and whistles

Comment 9

[Thomas Arend [:tarend]]

I would like to avoid any kind of popups. Also: *IF* you expect users to select -> tap on the selection to actually copy, I would expect to see issues when you select only a word or small portion of text, as it may be difficult to tap on the selection without moving the handles or tapping the line above or below, especially on small screens.

Comment 10

[Paul [pwd]]

(In reply to comment #8)
Different bug? It's an alternative solution to fix this problem. No matter how you look at it, the proposed method isn't very intuitive. Whereas, the 'bells and whistles' approach is the standardised approach across the two biggest operating systems with an overwhelming user-share across the planet. It makes little sense to cut our nose off to spite our face and go against what the user knows. It's basically akin to saying on desktop: "We're going to make copy SHIFT+C rather than Ctrl/Cmd+C just because..." Convention remains a compelling argument.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

So file a new bug. I do not want to take such a fix in this bug. I want to be able to land the fix for _this_ bug in Fx7, if possible - so simple is better.

Comment 12

[Martijn Wargers (dead)]

Ok, I filed bug 674999 for the first issue in comment 3.

Comment 13

[Paul [pwd]]

(In reply to comment #11)
> So file a new bug. I do not want to take such a fix in this bug. I want to
> be able to land the fix for _this_ bug in Fx7, if possible - so simple is
> better.
That was Bug 668201 which was in fact filed prior to this.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch tests to see if the user has tapped on the selection before copying to the clipboard. Because the selection range may be gone by the time we need it, I cache the rect of the selection in the "cache" object.

extractFromRange could be nicer/cleaner, but it gets the job done with as little hackiness as possible. I wish range.getBoundingClientRect actually worked.

Comment 15

[Martijn Wargers (dead)]

What do you mean? In what situation doesn't range.getBoundingClientRect work?

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #15)
> What do you mean? In what situation doesn't range.getBoundingClientRect work?

While debugging the patch, I was using range.getBoundingClientRect() to set this.cache.rect, but the value was always empty when I needed to use it.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch v2

This patch is the same as the last patch except:
* I use range.getClientBoundingRect() where possible
* the range bounding rect needs to be offset for iframes
* the tap point we use to test the tap-in-selection needs to be offset with the iframe offset, not just the scrollOffset

To fix a few other bugs as well:
* the iframe offset needs to be inited to the scrollOffset (bug 675454)
* we test that the selection is limited to the contentWindow of the initial selection (bug 675459)
* we test that the initial tap point is in the initial selection range or we cancel (bug 674515)

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 549826 [details] [diff] [review]
patch v2

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

Lots of debugging cruft, but overall looks good.

I'm still seeing a few problems selecting in scrolled iframes that are inside scrolled pages, and just noticed that selecting in rtl pages is also broken (grrr). But those are very different from this bug. I'll file the second, and have to look at the first harder to figure out exactly whats up.

::: mobile/chrome/content/common-ui.js
@@ +1278,3 @@
>      this._start.addEventListener("TapUp", this, true);
>  
> +//    this._end.addEventListener("TapDown", this, true);

Some reason we're keeping these around?

@@ +1284,5 @@
>      messageManager.addMessageListener("Browser:SelectionCopied", this);
>  
>      this.popupState.target.messageManager.sendAsyncMessage("Browser:SelectionStart", { x: this.popupState.x, y: this.popupState.y });
>  
> +    //BrowserUI.pushPopup(this, [this._start, this._end]);

Same

@@ +1310,5 @@
> +                let json = {
> +                  x: pos.x,
> +                  y: pos.y
> +                };
> +

I thought I was the only one allowed to use crazy indents?

@@ +1350,5 @@
> +          this.deltaY = (aEvent.clientY - this.target.top);
> +          window.addEventListener("TapMove", this, true);
> +        } else {
> +          this.hide(aEvent);
> +        }

Different bug probably, but this will make it easier to allow panning + selection. In fact, it might be better to only fire this on clicks, not TapDown.

@@ +1373,5 @@
>            };
>            this.popupState.target.messageManager.sendAsyncMessage("Browser:SelectionMove", json);
>          }
>          break;
> +      case "mousedown":

Is there a mousedown listener here? I can't find it. Also, there's some debugging below this.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

I removed all the commented code and debuggin
I un-indented the JSON block
I removed the unused "mousemove"

pushed:
http://hg.mozilla.org/mozilla-central/rev/254760547835

Comment 20

[Martijn Wargers (dead)]

Verified fixed, using current nightly on the LG Optimus Black.

Comment 21

[Aaron Train [:aaronmt]]

Is this still Aurora desired (tracking-fennec:7+) ?

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Yes

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 549826 [details] [diff] [review]
patch v2

mobile only and it helps stabilize the text selection feature

Comment 24

[LegNeato]

Comment on attachment 549826 [details] [diff] [review]
patch v2

Approved for releases/mozilla-aurora. Please land ASAP.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/751b55a876f6

Comment 26

[Aaron Train [:aaronmt]]

Verified Fixed on Aurora
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a2) Gecko/20110805 Firefox/7.0a2 Fennec/7.0a2

Comment 27

[Aaron Train [:aaronmt]]

Litmus Test Case (Aurora 7.0) https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=25817
Litmus Test Case (Nightly 8.0) https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=25818

Comment 28

[Martijn Wargers (dead)]

Sorry for not getting to the litmus tests, Aaron. Thanks for doing this.