Closed Bug 695173 (text-selection) Opened 13 years ago Closed 12 years ago

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

Categories

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

ARM
Android
defect

Tracking

(firefox15 verified, firefox16 verified)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- verified
firefox16 --- verified

People

(Reporter: elan, Assigned: Margaret)

References

Details

Attachments

(2 files, 7 obsolete files)

Text selection and clipboard
Priority: -- → P1
Priority: P1 → P3
OS: Mac OS X → Android
Hardware: x86 → ARM
Whiteboard: [QA+]
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.
Summary: Selection → [meta] Unable to select text in web content (static text areas or textboxes)
tracking-fennec: --- → 11+
Blocks: 724339
No longer blocks: 724339
Depends on: 724339
likely minus this for blocking 1.0.
blocking-fennec1.0: --- → ?
un nom meta bug
blocking-fennec1.0: ? → ---
Blocks: 566225
Tracking 15?
tracking-fennec: 11+ → ?
Whiteboard: [QA+]
Assignee: nobody → sriram
Attached patch WIP (obsolete) — Splinter Review
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.
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
Sriram and I are trading bugs :)
Assignee: sriram → margaret.leibovic
Attached patch WIP v2 (obsolete) — Splinter Review
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).
Attachment #624116 - Attachment is obsolete: true
Attachment #630006 - Flags: feedback?(mark.finkle)
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
Attachment #630006 - Flags: feedback?(mark.finkle) → feedback+
Attached patch mostly working patch (obsolete) — Splinter Review
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
Attachment #630006 - Attachment is obsolete: true
Attachment #631230 - Flags: review?(mark.finkle)
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}]
Depends on: 763772
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #631230 - Attachment is obsolete: true
Attachment #631230 - Flags: review?(mark.finkle)
Attachment #632075 - Flags: review?(mark.finkle)
Attached patch patch v2.1 (obsolete) — Splinter Review
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).
Attachment #632075 - Attachment is obsolete: true
Attachment #632075 - Flags: review?(mark.finkle)
Attachment #632080 - Flags: review?(mark.finkle)
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.
Attached patch patch v2.2 (obsolete) — Splinter Review
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
Attachment #632080 - Attachment is obsolete: true
Attachment #632080 - Flags: review?(mark.finkle)
Attachment #632464 - Flags: review?(mark.finkle)
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.
(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.
Attached patch patch v2.3 (obsolete) — Splinter Review
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.
Attachment #632464 - Attachment is obsolete: true
Attachment #632464 - Flags: review?(mark.finkle)
Attachment #633218 - Flags: review?(mbrubeck)
Depends on: 765057
Depends on: 765072
Depends on: 765076
Depends on: 765079
This morphed away from a meta bug. The bugs that depend on this one are all follow-ups that depend on this patch.
Summary: [meta] Unable to select text in web content (static text areas or textboxes) → Basic support for text selection in web content (static text areas)
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?
Attachment #633218 - Flags: review?(mbrubeck) → review-
Depends on: 765367
Attached patch patch v3Splinter Review
(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.
Attachment #633218 - Attachment is obsolete: true
Attachment #633663 - Flags: review?(mbrubeck)
(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?
Depends on: 765372
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...
Attachment #633663 - Flags: review?(mbrubeck) → review+
Depends on: 765390
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3bbd67b9528
Target Milestone: --- → Firefox 16
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.
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
https://hg.mozilla.org/mozilla-central/rev/e2578b5c2968
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 766102
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
Flags: in-moztrap?(fennec)
Depends on: 766556
Depends on: 766566
Depends on: 766588
Depends on: 677896
Depends on: 766651
Depends on: 766789
Depends on: 767070
Depends on: 767065
Depends on: 767626
Depends on: 767799
Depends on: 767800
Depends on: 767600
Depends on: 768665
Depends on: 768666
Depends on: 768667
Depends on: 769038
Depends on: 769049
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
Attachment #637747 - Flags: approval-mozilla-aurora?
Alias: text-selection
Verified Fixed on trunk (m-c), 06/28.
Status: RESOLVED → VERIFIED
Depends on: 770371
Depends on: 771197
Depends on: 772140
Depends on: 772280
Attachment #637747 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 772483
Depends on: 772422
Depends on: 772656
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.
Component: General → Text Selection
Depends on: 773718
Depends on: 773730
No longer depends on: 773730
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 ...
(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.
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!
Depends on: 667993
No longer depends on: 763772
Depends on: 779930
Depends on: 782691
Depends on: 828254
tracking-fennec: ? → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.