Last Comment Bug 765079 - Support text selection in inputs/textareas
: Support text selection in inputs/textareas
Status: VERIFIED FIXED
[parity-xul], [parity-stock], [parity...
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 17
Assigned To: Sriram Ramasubramanian [:sriram]
:
Mentors:
: 765555 766258 777684 (view as bug list)
Depends on: 780301 780906 781182
Blocks: text-selection
  Show dependency treegraph
 
Reported: 2012-06-14 16:19 PDT by :Margaret Leibovic
Modified: 2013-04-23 01:02 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified
17+


Attachments
WIP (10.40 KB, patch)
2012-08-02 12:45 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: feedback+
Details | Diff | Review
Patch (9.94 KB, patch)
2012-08-02 16:33 PDT, Sriram Ramasubramanian [:sriram]
margaret.leibovic: review+
sriram.mozilla: feedback+
Details | Diff | Review

Description :Margaret Leibovic 2012-06-14 16:19:45 PDT
The patch in bug 695173 adds support for text selection in static text areas, but we should also support text selection in inputs/textareas. I'm guessing that patch should get us most of the way there to solving this problem.
Comment 1 Aaron Train [:aaronmt] 2012-06-18 11:20:54 PDT
*** Bug 765555 has been marked as a duplicate of this bug. ***
Comment 2 Aaron Train [:aaronmt] 2012-06-19 18:47:01 PDT
*** Bug 766258 has been marked as a duplicate of this bug. ***
Comment 3 Sriram Ramasubramanian [:sriram] 2012-08-02 12:45:59 PDT
Created attachment 648445 [details] [diff] [review]
WIP

This does the basic job of showing handles, moving handles, selecting text, copying to clipboard. I am not sure about our javascript conventions but I've tried to match it to my best.

We might need to add "cut, paste" to the context menu -- only if its' an input element -- which is not there currently.

Also, we add to context menu on Clipboard.init(). Should we be removing them (or using them with / moving them to SelectionHandler?)
Comment 4 :Margaret Leibovic 2012-08-02 13:30:19 PDT
Comment on attachment 648445 [details] [diff] [review]
WIP

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

I'm happy with how simple this turned out to be. Nice work :) I just have one piece of feedback...

::: mobile/android/chrome/content/browser.js
@@ +1644,5 @@
>      }
>  
> +    if (aElement instanceof Ci.nsIDOMNSEditableElement) {
> +      // Input element
> +      this._view = aElement;

I don't like overloading _view to be either an editable element or a window. I think that we should make a different field to keep track of the case where we have an editable element, and in that case do something different to get the selection/selectionController. This will avoid the places where you need to call this._view.ownerDocument.defaultView in a bunch of places below, and it will make it easier to avoid confusion in the future.
Comment 5 Sriram Ramasubramanian [:sriram] 2012-08-02 14:10:25 PDT
(In reply to Margaret Leibovic [:margaret] from comment #4)
> Comment on attachment 648445 [details] [diff] [review]
> WIP
> 
> Review of attachment 648445 [details] [diff] [review]:
> -----------------------------------------------------------------
>  
> I don't like overloading _view to be either an editable element or a window.
> I think that we should make a different field to keep track of the case
> where we have an editable element, and in that case do something different
> to get the selection/selectionController. This will avoid the places where
> you need to call this._view.ownerDocument.defaultView in a bunch of places
> below, and it will make it easier to avoid confusion in the future.

I am not sure how to approach it. I felt _view is the one that's under consideration for the text-selection. Like the element where "text selection" needs to be done. It can either be an entire window or the input field. Where _view feels right to me.

But if we want to hold the window in _view, I would suggest, let _window holds the window in both cases and _view hold window / input element. Is that fine?
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-02 14:40:45 PDT
Comment on attachment 648445 [details] [diff] [review]
WIP


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


>-    // Get the element's view
>-    this._view = aElement.ownerDocument.defaultView;
>+    if (aElement instanceof Ci.nsIDOMNSEditableElement) {
>+      // Input element
>+      this._view = aElement;
>+    } else {
>+      // Get the element's view
>+      this._view = aElement.ownerDocument.defaultView;
>+      this._isRTL = (this._view.getComputedStyle(aElement, "").direction == "rtl");
>+    }

I am OK with using _target instead of _view and using _view as the window only.

That said, I think _isRTL must always be set, using the _view.

f+ from me. Use Margaret for the final review once you switch the _target/_view stuff.
Comment 7 :Margaret Leibovic 2012-08-02 15:10:17 PDT
(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> (In reply to Margaret Leibovic [:margaret] from comment #4)
> > Comment on attachment 648445 [details] [diff] [review]
> > WIP
> > 
> > Review of attachment 648445 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >  
> > I don't like overloading _view to be either an editable element or a window.
> > I think that we should make a different field to keep track of the case
> > where we have an editable element, and in that case do something different
> > to get the selection/selectionController. This will avoid the places where
> > you need to call this._view.ownerDocument.defaultView in a bunch of places
> > below, and it will make it easier to avoid confusion in the future.
> 
> I am not sure how to approach it. I felt _view is the one that's under
> consideration for the text-selection. Like the element where "text
> selection" needs to be done. It can either be an entire window or the input
> field. Where _view feels right to me.
>
> But if we want to hold the window in _view, I would suggest, let _window
> holds the window in both cases and _view hold window / input element. Is
> that fine?

I think _view is better than _window as a variable name because it corresponds to ownerDocument.defaultView, whereas _window could be confused with the browser window or top-level document window (element.ownerDocument is not necessarily the top-level document). This also minimizes changes to the existing code, since we don't need to be doing anything with _view in this patch.

Keeping track of the selection element in a _target variable like mfinkle suggested sounds nice to me, and then we can do a check to see if that element is a Ci.nsIDOMNSEditableElement. Keep in mind you'll also want to store a weak reference to that like to do for _view to avoid leaks.

Also note that like mfinkle said, we should always be setting _isRTL (I missed that when I was looking at this patch the first time), and we should use _view for that.
Comment 8 Sriram Ramasubramanian [:sriram] 2012-08-02 16:33:46 PDT
Created attachment 648552 [details] [diff] [review]
Patch

This patch has required changes. f+ carried forward.
Comment 9 :Margaret Leibovic 2012-08-02 17:14:28 PDT
Comment on attachment 648552 [details] [diff] [review]
Patch

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

When I was testing this, I found weird things happened when you dragged the selection handle outside of the input area. I imagine this is caused by us still using the editable _target to get the selection, even when the selection moves outside of the input. I think what we need here is more logic for keeping the selection inside the selection area, but we can do this in a follow-up, since it may be tricky.

Other than that, I just had a few nits:

::: mobile/android/chrome/content/browser.js
@@ +1378,5 @@
>          element = element.parentNode;
>        }
>  
>        // only send the contextmenu event to content if we are planning to show a context menu (i.e. not on every long tap)
> +      if (!(this.textContext.matches(element)) && this.menuitems) {

Nit: You can ditch the extra parens and just do !this.textContext.matches(element).

And could you also add a comment about why we never want to show a context menu on text? It's not your code, but the fact that "textContext" matches inputs and textareas is confusing (I would have thought it matched text nodes or something like that).

@@ +1900,5 @@
>      let offset = this._getViewOffset();
>      let scrollX = {}, scrollY = {};
> +    let win = this._view;
> +    win.top.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).getScrollXY(false, scrollX, scrollY);
> +

You don't need to make this change.
Comment 10 Sriram Ramasubramanian [:sriram] 2012-08-03 12:39:28 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/4863aa949575
Comment 11 Ed Morley [:emorley] 2012-08-04 11:18:53 PDT
https://hg.mozilla.org/mozilla-central/rev/4863aa949575
Comment 12 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-07 08:22:30 PDT
This broke the ability to "Add as search engine" on arbitrary text fields, since now long-pressing on a text field doesn't bring up a context menu any more. Will file a new bug to block this.
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-07 10:51:31 PDT
(In reply to Kartikaya Gupta (:kats) from comment #12)
> This broke the ability to "Add as search engine" on arbitrary text fields,
> since now long-pressing on a text field doesn't bring up a context menu any
> more. Will file a new bug to block this.

Actually, on Gingerbread a long tap always shows the contextmenu. Two of the menu items are "Select All" and "Select Word". Tapping into the editbox shows the caret handle.
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-09 10:40:41 PDT
*** Bug 777684 has been marked as a duplicate of this bug. ***
Comment 15 Cristian Nicolae (:xti) 2012-09-25 06:42:07 PDT
Text selection is supported on the latest Nightly build. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-25)
Device: Galaxy Note
OS: Android 4.0.4

Note You need to log in before you can comment on or make changes to this bug.