695173 - (text-selection) Basic support for text selection in web content (static text areas)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect, P3)

Description

[Erin Lancaster [:elan]]

Text selection and clipboard

Comment 1

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

More clarification: We can't select text in textboxes or static web content. We could track webcontent drag handles, textbox context menu and textbox drag handles all as separate bugs. So this is a meta bug.

Comment 3

[Tony Chung [:tchung]]

likely minus this for blocking 1.0.

Comment 4

[Tony Chung [:tchung]]

un nom meta bug

Comment 6

[Aaron Train [:aaronmt]]

Tracking 15?

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

This patch allows text selection and copying to clipboard.
Few things to fix:
1. Scrolling and zooming doesnt position the handles properly. The initial handle positioning should take care of scrolling and zooming <-- I need some help here.
2. The other event listeners (like url-change, tab-change) should be take care of -- currently commented out.
3. Code should be cleaned up to be moz-central ready.

Probable things:
1. Newer handles.
2. No resizing of handles on zooming.

Comment 10

[Aaron Train [:aaronmt]]

Just going to note here of an assortment of bugs and edge cases to consider with this reimplementation (see all the dependancy bugs for XUL) https://bugzilla.mozilla.org/showdependencytree.cgi?id=661388&hide_resolved=1

Comment 11

[:Margaret Leibovic]

Sriram and I are trading bugs :)

Comment 12

[:Margaret Leibovic]

Attachment: WIP v2

I really changed things around in this patch, but I think it's on its way to being much better than before. In particular, I found that we could simplify things since we don't need to deal with e10s anymore.

Problems left to deal with:
-The selection seems to get messed up sometimes when moving the start handle. I was trying to debug this, and it seems like something is going wrong with the mouse events that we're sending, since the handle positions are in the correct place.
-Touch events can be a little flakey when the handles are small.
-The handles disappear when changing tabs or trying to select text in a sub-document.
-We need to figure out how to make these handle elements anonymous if we want to keep them in the content document.

Awesome things:
-Works well with scrolling/zooming.
-Handles now flip positions when the end handle moves above the start handle (side-effect of my finishMoveSelection function).

Comment 13

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

Comment on attachment 630006 [details] [diff] [review]
WIP v2


>+var SelectionHandler = {

>+    Services.obs.addObserver(this, "Gesture:LongPress", false);
>+    Services.obs.removeObserver(this, "Gesture:LongPress", false);

I wonder if we'll run into any conflicts with the contextmenu code

>+  startSelection: function sh_startSelection(aX, aY) {

>+    } catch(e) {
>+      // If we couldn't select the word at the given point, bail
>+      Cu.reportError(e);

I guess reporting something here makes sense. You might want to use: dump("Unable to select text: " + e);

>+  finishMoveSelection: function sh_finishMoveSelection() {
>+    dump("finishMoveSelection--------------------");
>+    // Cache the selected text since the selection might be gone by the time we get the "end" message
>+    let selection = this.view.getSelection()
>+    this.selectedText = selection.toString().trim();
>+
>+    // Update the rect we use to test when finishing the clipboard operation
>+    let range = selection.getRangeAt(0);
>+    this._updateCacheFromRange(range);
>+
>+    // Update the offset in case any of the windows scrolled during touch movement
>+    this._updateCacheOffset();
>+  },
>+
>+  endSelection: function sh_endSelection(aX, aY) {

>+        this.view.getSelection().removeAllRanges();
>+    } catch(e) {
>+      Cu.reportError(e);

This might be a reason for someone to file a bug we don't care about. Let's not report anything here.

>+      clipboard.copyString(this.selectedText);
>+      NativeWindow.toast.show("Text copied", "short");

Put a string in browser.properties

>+  get _start() {
>+    delete this._start;
>+
>+    let doc = this.view.document;
>+    if (doc.getElementById("selectionhandle-start") == null) {
>+      let selectionHandleStart = doc.createElement("DIV");
>+      selectionHandleStart.id = "selectionhandle-start";
>+      doc.body.appendChild(selectionHandleStart);

I could be wrong here, but this function/property seems to be memoizing (deleting the original getter and assigning this._start to a DOM node at the end of the function). This would work ok for a single web page, but what about navigating to a new web site? Don't we need to create a new DIV in that new webpage? Also, should we clean up our reference to this DOM node on pagehide so we don't leak it? or does the DOM get cleaned up on it's own?

I know this will likely change a bunch for anonymous handles too.

>+      selectionHandleStart.customDragger = {
>+        isDraggable: function isDraggable(target, content) { return { x: true, y: false }; },
>+        dragStart: function dragStart(cx, cy, target, scroller) {},
>+        dragStop: function dragStop(dx, dy, scroller) { return false; },
>+        dragMove: function dragMove(dx, dy, scroller) { return false; }
>+      };

I think this "customDragger" is cruft from e10s

>+  get _end() {

same changes here


>+  handleEvent: function sh_handleEvent(aEvent) {
>+    aEvent.preventDefault();

Should we do this to touchstart, touchmove and touchend? or just touchstart?


>diff --git a/mobile/android/themes/core/browser.css b/mobile/android/themes/core/browser.css

> /* content scrollbars */
> .scroller {
>-  opacity: 0;
>+  opacity: 0.7;

Is this needed?

f+ overall this looks good

Comment 14

[:Margaret Leibovic]

Attachment: mostly working patch

This really isn't perfect, but maybe it's close enough to land and then work on follow-up bugs. The only thing I would maybe want us to figure out before landing is the binding stuff.

I was having a tough time getting the XBL to work with style.MozBinding. At first I got a content security warning when I had the binding in bindings.xml, so I tried putting it in its own file in a contentaccessible package, but then I still couldn't get it to bind (I'm concerned my jar.mn foo may have been wrong).

I'm also still getting weird problems sometimes with the mouse events in moveSelection causing too much text to get selected, but I wasn't able to figure out what's going on there.

Lastly, I was testing on a page with subframes [1], and there are still some things to work out with getting the coordinates correct in all cases, but that feels like it would be okay in a follow-up.

[1] http://people.mozilla.com/~mleibovic/test/text_select_iframe.html

Comment 15

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

I tried out the patch but things were not working for me. On initial long tap (on text) I get a selected word, but as we try to show the handles, I get errors:

E/GeckoConsole(15882): [JavaScript Error: "TypeError: this._start is null" {file: "chrome://browser/content/browser.js" line: 1634}]
E/GeckoConsole(15882): [JavaScript Error: "TypeError: this._start is null" {file: "chrome://browser/content/browser.js" line: 1664}]
E/GeckoConsole(15882): [JavaScript Error: "TypeError: this._start is null" {file: "chrome://browser/content/browser.js" line: 1664}]
E/GeckoConsole(15882): [JavaScript Error: "TypeError: this._start is null" {file: "chrome://browser/content/browser.js" line: 1664}]

Comment 17

[:Margaret Leibovic]

Attachment: patch v2

Let me know if you still run into problems testing. Which webpage were you using? Maybe it was a build problem getting the XBL/CSS to update, since the error was that it couldn't find the bound element.

Also, I found this to be pretty slow with a debug build, but much faster with an opt build, in case that affects your feelings when testing.

Comment 18

[:Margaret Leibovic]

Attachment: patch v2.1

I updated the x/y coordinates being sent in moveSelection to account for the handle padding, and this seems to be working really well for me right now on a basic text page (knock on wood).

Comment 19

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

Comment on attachment 632080 [details] [diff] [review]
patch v2.1

I tried this patch in a local build again. Still failing to find the bound elements. I tried a build on a nexus s and a galaxy nexus.

Comment 20

[:Margaret Leibovic]

Attachment: patch v2.2

It seems that the binding doesn't get attached properly on some pages, but I can't figure out why. I updated the patch to log the error and bail when that happens, so text selection just won't work on some pages, but nothing else bad will happen. I feel like we need to call in the XBL experts here, because I'm out of ideas.

I also changed the patch to only store weak references to DOM elements, so that we don't cause leaks, since I was seeing some errors about this.

Here's an opt build to test:
https://dl.dropbox.com/u/3358452/text-select.apk

A page I know for sure works is my super simple plain text testcase:
http://people.mozilla.com/~mleibovic/test/lorem_ipsum.html

Comment 21

[:Margaret Leibovic]

Neil, I'm wondering if you can offer some XBL help here. We're trying to use XBL to attach anonymous "handle" elements to the same document as a selection, and running into two main issues:

1) For my first attempt, I made a style rule to attach the binding to every body element. This appears to work fine, but then I discovered that for some pages my document.getAnonymousElementByAttribute calls aren't finding my bound elements. I couldn't find a pattern to figure out what about the pages causes this to happen. Do you have any ideas?

2) Ideally, we want to dynamically attach these bindings only when there is an active text selection. I tried doing this with body.style.MozBinding (commented out in my patch), but I couldn't get it to work. I tried calling document.loadBindingDocument to make sure the binding document was loaded (MDN says this works synchronously), but the binding still didn't seem to attach.

Comment 22

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #21)

> 1) For my first attempt, I made a style rule to attach the binding to every
> body element. This appears to work fine, but then I discovered that for some
> pages my document.getAnonymousElementByAttribute calls aren't finding my
> bound elements. I couldn't find a pattern to figure out what about the pages
> causes this to happen. Do you have any ideas?

I've found this problem happens when a stylesheet (even an empty one) is included in the page. I made a testcase here:
http://people.mozilla.com/~mleibovic/test/bad.html

This makes me suspect that loading a stylesheet is causing something wonky to happen and get rid of the binding attached to the body.

Comment 23

[:Margaret Leibovic]

Attachment: patch v2.3

The problem was that we needed to include a <children/> node in the binding content, since otherwise the content won't be added if the body already has children. The fact that this worked at all in some cases without this change is confusing, but it looks to be working on all pages now.

Comment 24

[:Margaret Leibovic]

This morphed away from a meta bug. The bugs that depend on this one are all follow-ups that depend on this patch.

Comment 25

[Matt Brubeck (:mbrubeck)]

Comment on attachment 633218 [details] [diff] [review]
patch v2.3

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

Looks nice!   Just a handful of things I'd like to see fixed before landing.

::: mobile/android/chrome/content/browser.js
@@ +1432,5 @@
> +    // Only try to select text for certain kinds of elements
> +    if (!(element instanceof Ci.nsIDOMHTMLParagraphElement ||
> +          element instanceof Ci.nsIDOMHTMLDivElement ||
> +          element instanceof Ci.nsIDOMHTMLLIElement ||
> +          element instanceof Ci.nsIDOMHTMLPreElement ||

I've never liked this whitelist approach since I don't think it'll ever catch everything.  Right now you can't select bold text, for example.  The correct thing would be to check if the gesture results in a context menu, and if not then do selection instead.

But since this code already exists in XUL Fennec I won't block the review on it -- we can fix it in a follow-up bug if you want.  For now though let's at least add a bunch more elements to the whitelist: BodyElement, FontElement, FormElement, LabelElement, LegendElement, ModElement, QuoteElement, TableCaptionElem, and whatever it takes to match generic inline elements like <span>, <b>, <i>, <em>, <stronge>, <abbr>, <code>, <samp>, <tt>, <cite>, <sup>, <small>, <ruby>, etc.

@@ +1683,5 @@
> +    switch (aEvent.type) {
> +      case "touchstart":
> +        aEvent.preventDefault();
> +        aEvent.target.addEventListener("touchmove", this, false);
> +        break;

On touchstart you should note the initial touch coordinates and touch ID, so later on touchmove you can move the handles according to the delta from the initial coordinates (instead of snapping the center of the handle to the touch location).

@@ +1693,5 @@
> +        this.finishMoveSelection(isStartHandle);
> +        break;
> +
> +      case "touchmove":
> +        let touch = aEvent.changedTouches[0];

Instead of changedTouches[0], use touches.identifiedTouch(id), where "id" is the ID that you saved from the touchstart.

::: mobile/android/themes/core/content.css
@@ +346,5 @@
> +
> +/*XXX Add the binding dynamically using body.style.MozBinding */
> +body {
> +  -moz-binding: url("chrome://browser-content/content/content.xml#selection-handles");
> +}

Attaching to the body element messes up the absolute positioning if the body has position:relative.  I'm not sure if attaching to the html element would be any safer, or if you just need to correct for the body position in JavaScript.

@@ +349,5 @@
> +  -moz-binding: url("chrome://browser-content/content/content.xml#selection-handles");
> +}
> +
> +.selection-handle-start,
> +.selection-handle-end {

As you mentioned on IRC, there could be a problem if these styles conflict with content.  It's already pretty unlikely, but maybe we should use even more out-there class names (like "moz-fennec-selection-handle-start"?).  Or we could use XUL elements for the handles and a XUL namespace selector, if this doesn't impact performanc.

Or if we were really paranoid then I think we could give the handles their own XBL binding with its own binding stylesheet.

@@ +370,5 @@
> +}
> +
> +.selection-handle-start {
> +  background-image: url("chrome://browser/skin/images/handle-start.png");
> +}

What happens with these images in RTL content?  Do they need to be swapped then, or do they do the right thing already?

Comment 26

[:Margaret Leibovic]

Attachment: patch v3

(In reply to Matt Brubeck (:mbrubeck) from comment #25)

> ::: mobile/android/chrome/content/browser.js
> @@ +1432,5 @@
> > +    // Only try to select text for certain kinds of elements
> > +    if (!(element instanceof Ci.nsIDOMHTMLParagraphElement ||
> > +          element instanceof Ci.nsIDOMHTMLDivElement ||
> > +          element instanceof Ci.nsIDOMHTMLLIElement ||
> > +          element instanceof Ci.nsIDOMHTMLPreElement ||
> 
> I've never liked this whitelist approach since I don't think it'll ever
> catch everything.  Right now you can't select bold text, for example.  The
> correct thing would be to check if the gesture results in a context menu,
> and if not then do selection instead.

In this version of the patch I moved the startSelection call to where we handle context menus. It feels like we could do some clean-up to make this less spaghetti-ish, but it results in the desired behavior. 

> But since this code already exists in XUL Fennec I won't block the review on
> it -- we can fix it in a follow-up bug if you want.  For now though let's at
> least add a bunch more elements to the whitelist: BodyElement, FontElement,
> FormElement, LabelElement, LegendElement, ModElement, QuoteElement,
> TableCaptionElem, and whatever it takes to match generic inline elements
> like <span>, <b>, <i>, <em>, <stronge>, <abbr>, <code>, <samp>, <tt>,
> <cite>, <sup>, <small>, <ruby>, etc.

Looking at this makes me feel like a whitelist is a bad idea.

> ::: mobile/android/themes/core/content.css
> @@ +346,5 @@
> > +
> > +/*XXX Add the binding dynamically using body.style.MozBinding */
> > +body {
> > +  -moz-binding: url("chrome://browser-content/content/content.xml#selection-handles");
> > +}
> 
> Attaching to the body element messes up the absolute positioning if the body
> has position:relative.  I'm not sure if attaching to the html element would
> be any safer, or if you just need to correct for the body position in
> JavaScript.

I'm now attaching the binding to the root element, so we shouldn't need to worry about this.

> @@ +349,5 @@
> > +  -moz-binding: url("chrome://browser-content/content/content.xml#selection-handles");
> > +}
> > +
> > +.selection-handle-start,
> > +.selection-handle-end {
> 
> As you mentioned on IRC, there could be a problem if these styles conflict
> with content.  It's already pretty unlikely, but maybe we should use even
> more out-there class names (like "moz-fennec-selection-handle-start"?).  Or
> we could use XUL elements for the handles and a XUL namespace selector, if
> this doesn't impact performanc.
> 
> Or if we were really paranoid then I think we could give the handles their
> own XBL binding with its own binding stylesheet.

For now I just created a longer class name as suggested. I got rid of the separate class names for the start/end handles, since I was planning to do that for bug 765057 anyway.

> @@ +370,5 @@
> > +}
> > +
> > +.selection-handle-start {
> > +  background-image: url("chrome://browser/skin/images/handle-start.png");
> > +}
> 
> What happens with these images in RTL content?  Do they need to be swapped
> then, or do they do the right thing already?

Good catch. I made a test page with dir="rtl" on the body [1], and I found that there are some problems, but I'm thinking the patch for bug 765057 will take care of it. Bug 724339 was already filed to track this issue, so I think we can make this follow-up material.

[1] http://people.mozilla.com/~mleibovic/test/lorem_ipsum_rtl.html

Other things to note:

-I was seeing an error caused by the call to getListenerInfoFor in _hasMouseListener, caused by the fact that we were passing in a null element from isElementClickable. I found that we were passing in the html element to isElementClickable, and then realized that the bound handle elements were what were getting found in the elementFromPoint calls in moveSelection. This is a similar issue to what I found in bug 765072, and I'm not sure of an easy solution to that, so I commented out the element check in moveSelection for now, and I filed bug 765367 about the issue.

-I found that calling endSelection() after navigating the page still tried to copy text, so I added a check to make sure we're in the same view when tying to copy the selected text.

Comment 27

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #26)

> -I found that calling endSelection() after navigating the page still tried
> to copy text, so I added a check to make sure we're in the same view when
> tying to copy the selected text.

Er, this didn't actually fix the problem because navigating doesn't change the document. I suppose I could add a listener to clear selection on navigation. Follow up?

Comment 28

[Matt Brubeck (:mbrubeck)]

Comment on attachment 633663 [details] [diff] [review]
patch v3

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

Nice!

Random thought, not needed for this bug but maybe a followup:  If we had an event listener object for each handle, then you could actually drag both handles at once using two fingers...

Comment 29

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3bbd67b9528

Comment 30

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e4861bc9f86f - other than the orange in mochitests 1-3 and 6-8, robocop, crashtests and reftests, everything when swimmingly with that push.

Comment 32

[:Margaret Leibovic]

I found that the problem was with putting content.xml in a new contentaccessible browser-content package. Since we don't actually need that for this version of the patch, I got rid of it. I verified that this is green on try:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2578b5c2968

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e2578b5c2968

Comment 34

[:Margaret Leibovic]

The changes to content.css and chrome/jar.mn didn't make it into the patch that I pushed to inbound due to me messing up conflict resolution (and not looking closely enough at the patch that I pushed). Fortunately, those changes *did* make it into the try push [1], so I'm just pushing the difference between what landed on try and inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/940f42b448bc

[1] https://hg.mozilla.org/try/rev/169a7ccf256f

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/940f42b448bc

Comment 36

[:Margaret Leibovic]

Attachment: Rollup patch for text selection

This is a rollup patch based on aurora of all the follow-up bugs that have landed. They're all listed in the commit message.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: users can't select/copy text on webpages
Testing completed (on m-c, etc.): 2 of these patches are still on inbound, but the rest have been on m-c for up to a week
Risk to taking this patch (and alternatives if risky): the main risk is that text selection itself could be buggy, this is a pretty self-contained patch, and the major changes have been tested on m-c
String or UUID changes made by this patch: added one string for "Share" item in context menu

Comment 37

[Aaron Train [:aaronmt]]

Verified Fixed on trunk (m-c), 06/28.

Comment 38

[:Margaret Leibovic]

Uplifted a roll-up patch to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/2fb0a358eaf6

I'll flip the appropriate flags in the dependent bugs, and it would be great to get some QA help to verify those bugs on aurora as well.

Comment 39

[Aaron Train [:aaronmt]]

Verified Fixed on Aurora (15.0a2)

Testcases Added: 

+ https://moztrap.mozilla.org/manage/case/785/
+ https://moztrap.mozilla.org/manage/case/786/
+ https://moztrap.mozilla.org/manage/case/787/

Comment 40

[J]

I'm sorry, I'm confused: will we see it in v15? In http://www.mozilla.org/en-US/mobile/14.0.1/releasenotes/ it's the only bug where it's said to be resolved in v16 but it isn't what I'm understanding from the above ...

Comment 41

[Aaron Train [:aaronmt]]

(In reply to J from comment #40)
> I'm sorry, I'm confused: will we see it in v15? In
> http://www.mozilla.org/en-US/mobile/14.0.1/releasenotes/ it's the only bug
> where it's said to be resolved in v16 but it isn't what I'm understanding
> from the above ...

Yes, this is targeting 15. It will be available likely end of week on Firefox Beta on Google Play.

Comment 42

[J]

Great! Then (specially if it also applies to Text fields' text) I'll be able to make it my default browser! Thanks a lot, guys! Firefox is the best browser, now on my cell phone!