:Margaret Leibovic Reporter
	Description • 12 years ago

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.

	Sriram Ramasubramanian [:sriram] Assignee
	Comment 3 • 12 years ago

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?)

	:Margaret Leibovic Reporter
	Comment 4 • 12 years ago

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.

	Sriram Ramasubramanian [:sriram] Assignee
	Comment 5 • 12 years ago

(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?

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 6 • 12 years ago

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.

	:Margaret Leibovic Reporter
	Comment 7 • 12 years ago

(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.

	Sriram Ramasubramanian [:sriram] Assignee
	Comment 8 • 12 years ago

This patch has required changes. f+ carried forward.

	:Margaret Leibovic Reporter
	Comment 9 • 12 years ago

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.

	Sriram Ramasubramanian [:sriram] Assignee
	Comment 10 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/4863aa949575

	Ed Morley [:emorley]
	Comment 11 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/4863aa949575

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 12 years ago

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.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 13 • 12 years ago

(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.

	Cristian Nicolae (:xti)
	Comment 15 • 12 years ago

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