Accidental text selection

VERIFIED FIXED in Firefox 7

Status

Fennec Graveyard
General
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: stechz, Assigned: mfinkle)

Tracking

(Depends on: 1 bug, {verified-aurora})

Trunk
Firefox 7
x86
Mac OS X
verified-aurora
Dependency tree / graph
Bug Flags:
in-litmus +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
During general browsing on Fennec, I often accidentally copy text when I don't mean to.
(Reporter)

Comment 1

6 years ago
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.
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.
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.

Updated

6 years ago
Priority: -- → P2
(Assignee)

Updated

6 years ago
Assignee: nobody → mark.finkle
tracking-fennec: --- → 7+
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?
Blocks: 661388
(In reply to comment #4)

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

Stock android browser on Gingerbread
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

6 years ago
(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?
(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
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

6 years ago
(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.
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.

Updated

6 years ago
Depends on: 674999
Ok, I filed bug 674999 for the first issue in comment 3.

Comment 13

6 years ago
(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.
Created attachment 549539 [details] [diff] [review]
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.
Attachment #549539 - Flags: review?(wjohnston)
What do you mean? In what situation doesn't range.getBoundingClientRect work?
(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.
Created attachment 549826 [details] [diff] [review]
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)
Attachment #549539 - Attachment is obsolete: true
Attachment #549826 - Flags: review?(wjohnston)
Attachment #549539 - Flags: review?(wjohnston)
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.
Attachment #549826 - Flags: review?(wjohnston) → review+
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Verified fixed, using current nightly on the LG Optimus Black.
Status: RESOLVED → VERIFIED
Flags: in-litmus?

Updated

6 years ago
Depends on: 675920

Updated

6 years ago
Depends on: 676008

Updated

6 years ago
Blocks: 674515
Is this still Aurora desired (tracking-fennec:7+) ?

Updated

6 years ago
Flags: in-litmus? → in-litmus?(martijn.martijn)
Yes
Comment on attachment 549826 [details] [diff] [review]
patch v2

mobile only and it helps stabilize the text selection feature
Attachment #549826 - Flags: approval-mozilla-aurora?

Comment 24

6 years ago
Comment on attachment 549826 [details] [diff] [review]
patch v2

Approved for releases/mozilla-aurora. Please land ASAP.
Attachment #549826 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/751b55a876f6
status-firefox7: --- → fixed
status-firefox8: --- → fixed
Target Milestone: --- → Firefox 7
Verified Fixed on Aurora
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a2) Gecko/20110805 Firefox/7.0a2 Fennec/7.0a2
Keywords: verified-aurora
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
Flags: in-litmus?(martijn.martijn) → in-litmus+
Sorry for not getting to the litmus tests, Aaron. Thanks for doing this.

Updated

6 years ago
Depends on: 695697
You need to log in before you can comment on or make changes to this bug.