Kill fake mouse event hacks used for text selection

RESOLVED FIXED in Firefox 23

Status

()

defect
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: blassey, Assigned: Margaret)

Tracking

({mobile})

Trunk
Firefox 23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

Attachments

(1 attachment, 18 obsolete attachments)

8.85 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
bug 661388 adds support for selecting text in content to Fennec, but it does so by sending fake mouse events which seems a bit dirty. It seems to me that in order to support this in the platform, we need to support to actions, selecting at a point and modifying the current selection with x,y coords.
Posted patch selectAtPoint patch (obsolete) — Splinter Review
this patch adds the selectAtPoint function to support the initial selection that Fennec does and modifies Fennec's selection code to use the new interface.
Assignee: nobody → blassey.bugs
Attachment #541961 - Flags: feedback?(roc)
Comment on attachment 541961 [details] [diff] [review]
selectAtPoint patch

> +    void selectAtPoint(inout PRInt32 sx, inout PRInt32 sy, in PRInt32 what, out PRInt32 ex, out PRInt32 ey);

If you're going to return anything here, just return the nsIDOMRange, not the rect. In fact, JS can use the exact same code as you are to get the rnage too, so really, you don't need to return anything.
Duplicate of this bug: 666819
Posted patch patch (obsolete) — Splinter Review
This patch adds both required functions and updates the Fennec selection code to use them.
Attachment #541961 - Attachment is obsolete: true
Attachment #541991 - Flags: review?(roc)
Attachment #541961 - Flags: feedback?(roc)
Posted patch patch (obsolete) — Splinter Review
a little ws cleanup
Attachment #541991 - Attachment is obsolete: true
Attachment #541995 - Flags: review?(roc)
Attachment #541991 - Flags: review?(roc)
Could we implement the CSSOM proposed caretPositionFromPoint API (bug 654352) and use that here instead?
Posted patch patch (obsolete) — Splinter Review
this patch adds pointMove to nsISelectionController to handle modifying the selection and depends on caretPositionFromPoint (I'll put a patch in bug 654352) and WordMove for the initial selection
Attachment #541995 - Attachment is obsolete: true
Attachment #542192 - Flags: review?(roc)
Attachment #541995 - Flags: review?(roc)
Comment on attachment 542192 [details] [diff] [review]
patch

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

why do we need the PointMove method? Can't we call caretPositionFromPoint again and use the node/offset to update the selection?
It would be nice to only modify the existing selection. I haven't figured out how to use node and offset to anchor one end of the selectipn range.
We can add an API to do that if you need to.
Fwiw, Selection.modify() may be of use to you:
https://developer.mozilla.org/en/DOM/selection/modify
(or perhaps could be extended with the stuff you need)
Blocks: 669816
Duplicate of this bug: 671052
Blocks: 672316
No longer blocks: 669816
tracking-fennec: --- → ?
tracking-fennec: ? → 7+
Posted patch patch (obsolete) — Splinter Review
Attachment #542192 - Attachment is obsolete: true
Attachment #551216 - Flags: review?(mark.finkle)
Attachment #542192 - Flags: review?(roc)
all the platform bits are taken care of by bug 668228, so I'm morphing this bug to change the front end code to use the new caretPositionFromPoint API
Depends on: 668228
Summary: platform support for selecting text at a point → use caretPositionFromPoint for controlling text selection
Blocks: 668228
No longer depends on: 668228
tracking-fennec: 7+ → 8+
Whiteboard: [needs-review (mfinkle)]
Posted patch patch (obsolete) — Splinter Review
Attachment #551216 - Attachment is obsolete: true
Attachment #552659 - Flags: review?(mark.finkle)
Attachment #551216 - Flags: review?(mark.finkle)
Comment on attachment 552659 [details] [diff] [review]
patch

># HG changeset patch
># User Brad Lassey <blassey@mozilla.com>
># Date 1313160247 14400
># Node ID 05fd6b6cf9d0ab713cc6f98dd4629e822e98160f
># Parent  cbda85bd65fa72daf89c562c0c806dbdc8db2ee1
>[mq]: use_caretPos
>
>diff --git a/mobile/chrome/content/content.js b/mobile/chrome/content/content.js
>--- a/mobile/chrome/content/content.js
>+++ b/mobile/chrome/content/content.js
>@@ -1377,16 +1377,16 @@ var SelectionHandler = {
>         // page will create an empty range.
>         let selection = contentWindow.getSelection();
>         selection.removeAllRanges();
>-
>-        // Position the caret using a fake mouse click
>-        utils.sendMouseEventToWindow("mousedown", x, y, 0, 1, 0, true);
>-        utils.sendMouseEventToWindow("mouseup", x, y, 0, 1, 0, true);
>-
>+        let caretPos = 
>+          content.document.caretPositionFromPoint(json.x - scrollOffset.x,
>+                                                  json.y - scrollOffset.y);

No wrap please

>         try {
>-          let selcon = currentDocShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsISelectionDisplay).QueryInterface(Ci.nsISelectionController);
>+          let selcon = docShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsISelectionDisplay).QueryInterface(Ci.nsISelectionController);

You must use currentDocShell for this to work in iframes

>           selcon.wordMove(false, false);
>-          selcon.wordMove(true, true);
>+        selcon.wordMove(true, true);
>         } catch(e) {

Indent issue

>-        // Use fake mouse events to update the selection
>         if (json.type == "end") {
>-          // Keep the cache in "client" coordinates, but translate for the mouse event
>-          this.cache.end = { x: json.x, y: json.y };
>-          let end = { x: this.cache.end.x - scrollOffset.x, y: this.cache.end.y - scrollOffset.y };
>-          utils.sendMouseEventToWindow("mousedown", end.x, end.y, 0, 1, Ci.nsIDOMNSEvent.SHIFT_MASK, true);
>-          utils.sendMouseEventToWindow("mouseup", end.x, end.y, 0, 1, Ci.nsIDOMNSEvent.SHIFT_MASK, true);
>+          this.cache.end.x = json.x - scrollOffset.x;
>+          this.cache.end.y = json.y - scrollOffset.y;

Don't undo the code to keep the cache coords in "client" space. We need it so we can test the "tapped in selection" when ending the selection

>         } else {
>-          // Keep the cache in "client" coordinates, but translate for the mouse event
>-          this.cache.start = { x: json.x, y: json.y };
>-          let start = { x: this.cache.start.x - scrollOffset.x, y: this.cache.start.y - scrollOffset.y };
>-          let end = { x: this.cache.end.x - scrollOffset.x, y: this.cache.end.y - scrollOffset.y };
>+          this.cache.start.x = json.x - scrollOffset.x;
>+          this.cache.start.y = json.y - scrollOffset.y;

Same

>+        }
>+        try {
>+          let caretPos = 
>+            content.document.caretPositionFromPoint(json.x - scrollOffset.x,
>+                                                    json.y - scrollOffset.y);

No wrapping please

>+          let selcon = docShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsISelectionDisplay).QueryInterface(Ci.nsISelectionController);

Wrong docShell
Attachment #552659 - Flags: review?(mark.finkle) → review-
Make sure you don't regress bug 673037, for example
Whiteboard: [needs-review (mfinkle)]
Posted patch patch (obsolete) — Splinter Review
Attachment #552659 - Attachment is obsolete: true
Attachment #552665 - Flags: review?(mark.finkle)
Comment on attachment 552665 [details] [diff] [review]
patch


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

>+  getCurrentWindowAndOffset: function(x, y, offset) {

>+    return { currentWindow: elem.ownerDocument.defaultView, offset: offset};

OCD:  return { currentWindow: elem.ownerDocument.defaultView, offset: offset };


>   receiveMessage: function sh_receiveMessage(aMessage) {

>+        let ret = this.getCurrentWindowAndOffset(x, y, scrollOffset);
>+        let contentWindow = ret.currentWindow;
>+        let offset = ret.offset;

Fancy way is:

          let { contentWindow: contentWindow, offset: offset } = this.getCurrentWindowAndOffset(x, y, scrollOffset);

JS "destructuring" feature

>+        let caretPos = content.document.caretPositionFromPoint(json.x - scrollOffset.x, json.y - scrollOffset.y);

content.document -> currentWindow.document

>-          // Keep the cache in "client" coordinates, but translate for the mouse event
>           this.cache.start = { x: json.x, y: json.y };
>-          let start = { x: this.cache.start.x - scrollOffset.x, y: this.cache.start.y - scrollOffset.y };
>-          let end = { x: this.cache.end.x - scrollOffset.x, y: this.cache.end.y - scrollOffset.y };
>+        }
>+        try {
>+          let caretPos = content.document.caretPositionFromPoint(x, y);

nit: blank line above the "try"

content.document -> currentWindow.document

>+          let ret = this.getCurrentWindowAndOffset(x, y, scrollOffset);
>+          let contentWindow = ret.currentWindow;
>+          let currentDocShell = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebNavigation).QueryInterface(Ci.nsIDocShell);
>+          let selcon = currentDocShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsISelectionDisplay).QueryInterface(Ci.nsISelectionController);

We cache the currentWindow in | this.currentWindow | cause we want to keep the selection inside that window. You can use it here and skip the getCurrentWindowAndOffset call.
Attachment #552665 - Flags: review?(mark.finkle) → review-
Posted patch patch (obsolete) — Splinter Review
Attachment #552665 - Attachment is obsolete: true
Attachment #552928 - Flags: review?(mark.finkle)
Comment on attachment 552928 [details] [diff] [review]
patch


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

>   receiveMessage: function sh_receiveMessage(aMessage) {
>+        //let { contentWindow: contentWindow, offset: offset } 
>+        let ret = this.getCurrentWindowAndOffset(x, y, scrollOffset);
>+        let contentWindow = ret.contentWindow;
>+        let offset  = ret.offset;

if this doesn't work:
          let { contentWindow: contentWindow, offset: offset } = this.getCurrentWindowAndOffset(x, y, scrollOffset);

just change "ret" -> "result" and I'll be happy


>         // Hack to avoid setting focus in a textbox [Bugs 654352 & 667243]
>-        let elemUnder = elementFromPoint(json.x - scrollOffset.x, json.y - scrollOffset.y);
>+        let elemUnder = elementFromPoint(x, y);
>         if (elemUnder && elemUnder instanceof Ci.nsIDOMHTMLInputElement || elemUnder instanceof Ci.nsIDOMHTMLTextAreaElement)
>           return;

I was hoping we could remove this hack when the API landed. Since we don't use a mouse event, the <input> shouldn't get focus. If that's true, feel free to remove this hack

>         if (json.type == "end") {
>           this.cache.end = { x: json.x, y: json.y };
>         } else {
>           this.cache.start = { x: json.x, y: json.y };
>+        }

Can you put this comment above the"if" block
          // Keep the cache in "client" coordinates

Also, no { } needed when both if/else branches are one-liners

Everything looks good. Once the platform API lands, we should be good to go. We just need to hammer away on some test cases, like using in iframe, moving the handles over textboxes and <a> links

r+ with nits fixed
Attachment #552928 - Flags: review?(mark.finkle) → review+
Posted patch patch for checkin (obsolete) — Splinter Review
Attachment #552928 - Attachment is obsolete: true
Attachment #553109 - Flags: review+
Duplicate of this bug: 678957
Blocks: 677896
Attachment #553109 - Flags: approval-mozilla-aurora?
http://hg.mozilla.org/mozilla-central/rev/636d0e651d5e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Comment on attachment 553109 [details] [diff] [review]
patch for checkin

Needs more baking on trunk
Attachment #553109 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Whiteboard: [inbound]
tracking-fennec: 8+ → 9+
Posted patch backout (obsolete) — Splinter Review
Bug 681387 is likely the reason why text selection isn't highlighting the right target. If you zoom or scroll down a page, try to long tap on a word. It's unlikely you'll get the word you want.

We should backout this patch until the API is known to work better.
Attachment #558517 - Flags: review?(mbrubeck)
Comment on attachment 558517 [details] [diff] [review]
backout

>+++ b/embedding/android/GeckoAppShell.java
Unrelated changes; please remove.

Is it intentional that you did not back out the "getCurrentWindowAndOffset" part of the patch?

Otherwise this looks fine, but I'd like to double-check the final patch after the above question is answered.
Attachment #558517 - Flags: review?(mbrubeck) → review-
Posted patch backout 2 (obsolete) — Splinter Review
I left the util method, but removed the unwanted GeckpAppShell changes
Attachment #558517 - Attachment is obsolete: true
Attachment #558620 - Flags: review?(mbrubeck)
Attachment #558620 - Flags: review?(mbrubeck) → review+
Keywords: mobile
OS: Linux → All
Hardware: x86_64 → All
Bacout: http://hg.mozilla.org/integration/mozilla-inbound/rev/ac5813b1196e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
tracking-fennec: 9+ → 10+
Depends on: 681387
tracking-fennec: 10+ → -
No longer blocks: 677896
There is now DOMWindowUtils.selectAtPoint which was implemented in bug 783531.
It seems like that can be used for Fennec?
tracking-fennec: - → ?
Assignee: blassey.bugs → sjohnson
tracking-fennec: ? → +
Blocks: 800805
(added blocker per mobile triage).
Assignee: sjohnson → bnicholson
tracking-fennec: + → 20+
Blocks: 821277
Too late for 20
tracking-fennec: 20+ → ?
tracking-fennec: ? → +
I can take this from bnicholson.
Assignee: bnicholson → margaret.leibovic
Target Milestone: mozilla9 → ---
I'm updating this summary to better reflect what it's actually doing. I'm looking to update our SelectionHandler to use the APIs that metro's SelectionHandler uses, which includes caretPositionFromPoint, but also selectAtPoint.

Also, I'm moving this back to Firefox for Android, since this is only going to touch mobile code.
Component: Layout → Text Selection
Product: Core → Firefox for Android
Summary: use caretPositionFromPoint for controlling text selection → Kill fake mouse event hacks in SelectionHandler
Attachment #553109 - Attachment is obsolete: true
Attachment #558620 - Attachment is obsolete: true
Depends on: 854605
I have a some more refactoring patches, followed by a big patch that makes a new module where we can share text selection logic with metro. There are definitely still some issues with integrating this shared logic, but I want to put up my patches to start getting some feedback.
Attachment #732452 - Flags: review?(bnicholson)
The shared code expects a _updateSelectionForUI method it can call, which needs to also handle updating the cache, so I put the call to _updateCacheForSelection in there.

The metro code doesn't appear support flipping the handles, but rather pushes the selection along as one handle approaches the other. I'm not sure if this is what we'll want or not, but the logic for flipping the handles will need to change anyway, so I'm just getting rid of it for now in this patch.

This patch on its own kinda breaks text selection, so we'd need to land it as part of the big change, but I figured it would be easier to review this piece on its own.
Attachment #732459 - Flags: review?(bnicholson)
Here's a WIP to make a shared text selection module. I decided to make an abstract base class for SelectionHandler, since lots of the logic we want to share depends on knowing about the SelectionHandler object's state, rather than stateless utility calls.

Almost all the code in AbstractSelectionHandler.js is just copy/pasted out of metro's SelectionHandler.js. I decided it would probably be simplest to make this work with fennec first, then we can update metro code to use this shared module in a follow-up bug. I tried to work with the existing code as much as possible so that it would be simple to make a switch, but there are some things we may want to change (e.g. for fennec, we don't need a call to _updateSelectionUI in _adjustSelection, since our handles move in Java as the user drags them).

Text selection mostly works with this, but there seem to be some jumpiness issues when the handles approach the edge of the page or the edge of an edit box. I'm also seeing some weirdness with dragging the selection handles across different frames, and scrolling in subframes.

I also just included some of the metro Util functions at the bottom of  AbstractSelectionHandler.js to make this work, but those should probably live in a better shared Util module.
Attachment #732468 - Flags: feedback?(jmathies)
Attachment #732468 - Flags: feedback?(bnicholson)
(In reply to :Margaret Leibovic from comment #39)
> Created attachment 732468 [details] [diff] [review]
> (Part 3 WIP) Create and use AbstractSelectionHandler to share text selection
> logic
> 
> Here's a WIP to make a shared text selection module. I decided to make an
> abstract base class for SelectionHandler, since lots of the logic we want to
> share depends on knowing about the SelectionHandler object's state, rather
> than stateless utility calls.
> 
> Almost all the code in AbstractSelectionHandler.js is just copy/pasted out
> of metro's SelectionHandler.js. I decided it would probably be simplest to
> make this work with fennec first, then we can update metro code to use this
> shared module in a follow-up bug. I tried to work with the existing code as
> much as possible so that it would be simple to make a switch, but there are
> some things we may want to change (e.g. for fennec, we don't need a call to
> _updateSelectionUI in _adjustSelection, since our handles move in Java as
> the user drags them).

I was wondering about this. How do your markers stay attached to the selection in the dom? Do they roam free or are they attached in some way other than the data in _cache, which gets forwarded over?

> Text selection mostly works with this, but there seem to be some jumpiness
> issues when the handles approach the edge of the page or the edge of an edit
> box. I'm also seeing some weirdness with dragging the selection handles
> across different frames, and scrolling in subframes.

Form element scrolling works well in metro. I haven't tried scrolling in something like an iframe. I should probably file a bug on that. Generally I think selection should always be contained on mobile devices within the frame it is first created. Although you all might know of use cases where that isn't optimal.
Also I think you can do away with the overlay debug routines (_setDebugRect, _setDebugElementRect, _setDebugPoint). Those forward messages over to our ui where we draw colored divs in the selection monocle overlay for debugging purposes. I'm not sure that's something you need, although maybe you do? Not sure.
(In reply to Jim Mathies [:jimm] from comment #40)
> (In reply to :Margaret Leibovic from comment #39)
> > Created attachment 732468 [details] [diff] [review]
> > (Part 3 WIP) Create and use AbstractSelectionHandler to share text selection
> > logic
> > 
> > Here's a WIP to make a shared text selection module. I decided to make an
> > abstract base class for SelectionHandler, since lots of the logic we want to
> > share depends on knowing about the SelectionHandler object's state, rather
> > than stateless utility calls.
> > 
> > Almost all the code in AbstractSelectionHandler.js is just copy/pasted out
> > of metro's SelectionHandler.js. I decided it would probably be simplest to
> > make this work with fennec first, then we can update metro code to use this
> > shared module in a follow-up bug. I tried to work with the existing code as
> > much as possible so that it would be simple to make a switch, but there are
> > some things we may want to change (e.g. for fennec, we don't need a call to
> > _updateSelectionUI in _adjustSelection, since our handles move in Java as
> > the user drags them).
> 
> I was wondering about this. How do your markers stay attached to the
> selection in the dom? Do they roam free or are they attached in some way
> other than the data in _cache, which gets forwarded over?

While the user is dragging the handle, they roam free, and they pass coordinates over to JS for us to try to update the selection appropriately. And when the user lifts their finger, we reposition the handles based on the selection.

However, when we move the caret handle around, we do call _positionHandles (or _updateSelectionUI in these patches) as it moves, and that prevents the user from dragging it all over the place, so maybe we do want to be more restrictive for text selection movement as well.

> > Text selection mostly works with this, but there seem to be some jumpiness
> > issues when the handles approach the edge of the page or the edge of an edit
> > box. I'm also seeing some weirdness with dragging the selection handles
> > across different frames, and scrolling in subframes.
> 
> Form element scrolling works well in metro. I haven't tried scrolling in
> something like an iframe. I should probably file a bug on that. Generally I
> think selection should always be contained on mobile devices within the
> frame it is first created. Although you all might know of use cases where
> that isn't optimal.

The main issue I'm seeing with form element scrolling is that the handles aren't being hidden if the selection ends are scrolled out of view, but I noticed this issue is also present in Nightly, so I should make sure there's a separate bug filed about that (I believe we fixed this for subframe scrolling, but not for form element scrolling). However, I'm also seeing some issues with the handles not moving properly when I pan a subframe, and this is something I managed to regress. Our Java code should be taking care of positioning the handles as we pan the page, so I'll need to investigate more.

If you're looking for a page with subframe test cases, you can try this:
http://people.mozilla.com/~mleibovic/test/text_select.html
(In reply to :Margaret Leibovic from comment #42)
> 
> While the user is dragging the handle, they roam free, and they pass
> coordinates over to JS for us to try to update the selection appropriately.
> And when the user lifts their finger, we reposition the handles based on the
> selection.
> 
> However, when we move the caret handle around, we do call _positionHandles
> (or _updateSelectionUI in these patches) as it moves, and that prevents the
> user from dragging it all over the place, so maybe we do want to be more
> restrictive for text selection movement as well.

Ok, sounds similar. _updateSelectionUI used to be _positionHandles. Metro updates the marker the user isn't dragging, and lets the one the user is dragging roam free. The cache data structure _updateSelectionUI hands back to front end was also in xul/fennec, and contains a grab bag of information the front end uses to position things. In the current metro code _cache has been expanded to contain even more information. 

..and hey, what do you know, your _positionHandles does the same thing - 

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js#492

except you return a 'positions' data struct.

> > > Text selection mostly works with this, but there seem to be some jumpiness
> > > issues when the handles approach the edge of the page or the edge of an edit
> > > box. I'm also seeing some weirdness with dragging the selection handles
> > > across different frames, and scrolling in subframes.
> > 
> > Form element scrolling works well in metro. I haven't tried scrolling in
> > something like an iframe. I should probably file a bug on that. Generally I
> > think selection should always be contained on mobile devices within the
> > frame it is first created. Although you all might know of use cases where
> > that isn't optimal.
> 
> The main issue I'm seeing with form element scrolling is that the handles
> aren't being hidden if the selection ends are scrolled out of view, but I
> noticed this issue is also present in Nightly, so I should make sure there's
> a separate bug filed about that (I believe we fixed this for subframe
> scrolling, but not for form element scrolling).


If I understand the issue right, I think this behavior is expected. In metro, lets say you select a word in a 300px wide text input that has text that extends beyond the bounds of the edit view -

http://www.mathies.com/mozilla/forms.html

(second text input after the text area)

once you select the word two markers show up on either side of the word. If you then grab the right side marker and drag it to the right outside the bounds, the edit will begin to scroll. The left hand marker will remain visible and slide over to the left hand border of the edit and sit there. 

so then say you let the right hand marker go, it snaps to the right hand side of the text edit, and what you see is a small chunk of the text that was just added on right side of the text string. If you then grab the left hand marker and drag left, the edit should - 1) snap to the left hand side of the selected string, and 2) start scrolling the edit to the left. So basically you can adjust either side using the markers that side on the edges of the input control.


> some issues with the handles not moving properly when I pan a subframe, and
> this is something I managed to regress. Our Java code should be taking care
> of positioning the handles as we pan the page, so I'll need to investigate
> more.

Currently in metro panning will cancel the edit session and clear selection. This is something we want to support but it's a hard problem since the markers are up in chrome and have very little information about pan or zoom operations in content. This was why I was sorta kinda interested in moving markers down into content, so that say, scrolling a page would automatically scroll the markers too. There are some problems to overcome with this though. For example, when you zoom the page you don't want your selection marker image to zoom as well! So for now we are keeping markers in chrome, and "blowing away" selection any time the user interacts with the page.

> 
> If you're looking for a page with subframe test cases, you can try this:
> http://people.mozilla.com/~mleibovic/test/text_select.html

Thanks, I have a bunch of tests for a single subframe (iframe) but no tests for an iframe within an iframe within an iframe....  I need to add some.

Which brings up an important point, none of our selection code has tests running in automation yet, but we should get that going over the next couple weeks.
Comment on attachment 732452 [details] [diff] [review]
(Part 1) Add _targetIsEditable and _contentOffset properties

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ -526,5 @@
>        let ey = this._cache.end.y;
>  
>        // Translate coordinates to account for selections in sub-frames. We can't cache
>        // this because the top-level page may have scrolled since selection started.
> -      let offset = this._getViewOffset();

By removing this line, we're no longer calculating the offset every time we do a position, and we're instead calculating it once and caching the result. But the comment right above this says we can't cache the offset because a parent frame may have scrolled. Won't that still be an issue?
Comment on attachment 732459 [details] [diff] [review]
(Part 2) Replace _positionHandles with _updateSelectionForUI

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +95,5 @@
>        }
>        case "after-viewport-change": {
>          if (this._activeType == this.TYPE_SELECTION) {
> +          // Update the handles after the viewport changes (e.g. panning, zooming).
> +          this._updateSelectionUI(true, true, false);

Rather than passing a set of booleans to _updateSelectionUI (which is difficult to tell what they correspond to without looking at the function), I think it would be clearer to pass an array of HANDLE_TYPE_* constants (either as an actual array, or as a set of arguments handled with the spiffy new rest parameter syntax [1]). Then you could do something like "this._updateSelectionUI(this.HANDLE_TYPE_START);", which is more readable IMO.

[1] https://developer.mozilla.org/en-US/docs/JavaScript/Reference/rest_parameters

@@ +119,5 @@
> +          this._updateSelectionUI(true, false, false);
> +        } else if (handleType == this.HANDLE_TYPE_END) {
> +          this._updateSelectionUI(false, true, false);
> +        } else if (handleType == this.HANDLE_TYPE_MIDDLE) {
> +          this._updateSelectionUI(false, false, true);

Using an array-based approach would clean this up as you could simply do "this._updateSelectionUI(handleType);" without needing to check them all here.

@@ +464,5 @@
>              aY - this._contentOffset.y < rangeRect.bottom + radius.bottom);
>    },
>  
> +  /*
> +   * _updateCacheForSelection()

Just curious -- what's the benefit of copying the method name here in the comment? It seems redundant to me, but I see Metro does the same thing.

@@ +528,5 @@
> +                       top: y + this._contentOffset.y + scrollY.value,
> +                       hidden: checkHidden(x, y) });
> +    }
> +
> +    if (aUpdateEnd) {

These blocks both do the same thing; if you want, you could factor them into a nested function or even a loop (e.g., "for (let handle of [this.HANDLE_TYPE_START, this.HANDLE_TYPE_END]) { ... }").
Comment on attachment 732459 [details] [diff] [review]
(Part 2) Replace _positionHandles with _updateSelectionForUI

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

Patch looks fine other than optional style changes, though I'd recommend at least dropping the boolean parameters.
Attachment #732459 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #44)
> Comment on attachment 732452 [details] [diff] [review]
> (Part 1) Add _targetIsEditable and _contentOffset properties
> 
> Review of attachment 732452 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/SelectionHandler.js
> @@ -526,5 @@
> >        let ey = this._cache.end.y;
> >  
> >        // Translate coordinates to account for selections in sub-frames. We can't cache
> >        // this because the top-level page may have scrolled since selection started.
> > -      let offset = this._getViewOffset();
> 
> By removing this line, we're no longer calculating the offset every time we
> do a position, and we're instead calculating it once and caching the result.
> But the comment right above this says we can't cache the offset because a
> parent frame may have scrolled. Won't that still be an issue?

Hrm, I may have been too quick to change things to just match metro, when this may be a case that their code doesn't handle properly (or one that they don't need to handle for updating their UI). I will investigate this more.

(In reply to Brian Nicholson (:bnicholson) from comment #46)
> Comment on attachment 732459 [details] [diff] [review]
> (Part 2) Replace _positionHandles with _updateSelectionForUI
> 
> Review of attachment 732459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks fine other than optional style changes, though I'd recommend at
> least dropping the boolean parameters.

I used those boolean parameters because that's what the shared module expects, although we could look into changing that API instead.
Comment on attachment 732468 [details] [diff] [review]
(Part 3 WIP) Create and use AbstractSelectionHandler to share text selection logic

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

All the new AbstractSelectionHandler.jsm code copied from Metro is a lot to absorb, and I haven't looked too closely at each method yet -- but overall, the abstraction looks good.

::: mobile/android/chrome/content/SelectionHandler.js
@@ -204,5 @@
>      }
>  
> -    // Add a listener to end the selection if it's removed programatically
> -    selection.QueryInterface(Ci.nsISelectionPrivate).addSelectionListener(this);
> -

Probably not related to this block, but something that popped into mind: do we correctly handle JS selection events from web pages (such as http://www.w3schools.com/jsref/met_text_select.asp)? I'm not sure these are even working in Nightly, but it's something worth adding to your set of test cases!
Attachment #732468 - Flags: feedback?(bnicholson) → feedback+
(In reply to Brian Nicholson (:bnicholson) from comment #48)
> Comment on attachment 732468 [details] [diff] [review]
> (Part 3 WIP) Create and use AbstractSelectionHandler to share text selection
> logic
> 
> Review of attachment 732468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All the new AbstractSelectionHandler.jsm code copied from Metro is a lot to
> absorb, and I haven't looked too closely at each method yet -- but overall,
> the abstraction looks good.

Yeah, I'm still having a hard time digesting it all myself :)

> ::: mobile/android/chrome/content/SelectionHandler.js
> @@ -204,5 @@
> >      }
> >  
> > -    // Add a listener to end the selection if it's removed programatically
> > -    selection.QueryInterface(Ci.nsISelectionPrivate).addSelectionListener(this);
> > -
> 
> Probably not related to this block, but something that popped into mind: do
> we correctly handle JS selection events from web pages (such as
> http://www.w3schools.com/jsref/met_text_select.asp)? I'm not sure these are
> even working in Nightly, but it's something worth adding to your set of test
> cases!

We were supporting hiding the handles if the selection was programatically collapsed (bug 767065), but I removed this for the time being because I was running into issues when _handleSelectionPoint and it's helper methods programatically collapse the selection at times. Bug 801421 was filed about supporting updating the handles if the selection is changed programatically.
Duplicate of this bug: 767799
You were right, we can't cache the offset because it will be wrong if the parent frame scrolls. So instead of setting _contentOffest in _initTargetInfo, I decided to just make it into a getter.

(Jim, this is probably a bug in metro code, too)
Attachment #732452 - Attachment is obsolete: true
Attachment #732452 - Flags: review?(bnicholson)
Attachment #733128 - Flags: review?(bnicholson)
I addressed the points in comment 45, although I decided not to factor out the blocks to add the start/end handles to the positions array because they're different enough that it makes it annoying (mainly where they use this._cache.start vs this._cache.end).

Since the shared code I copied into AbstractSelectionHandler does expect _updateSelectionUI to take boolean arguments, I decided to just implement _updateSelectionUI in my next patch, and it calls _positionHandles appropriately.
Attachment #732459 - Attachment is obsolete: true
Attachment #733143 - Flags: review?(bnicholson)
Here's an updated version of the AbstractSelectionHandler patch. It's mostly the same as before.
Attachment #732468 - Attachment is obsolete: true
Attachment #732468 - Flags: feedback?(jmathies)
(In reply to :Margaret Leibovic from comment #51)
> Created attachment 733128 [details] [diff] [review]
> (Part 1 v2) Add _targetIsEditable and _contentOffset properties
> 
> You were right, we can't cache the offset because it will be wrong if the
> parent frame scrolls. So instead of setting _contentOffest in
> _initTargetInfo, I decided to just make it into a getter.
> 
> (Jim, this is probably a bug in metro code, too)

Metro disables selection during dom scrolling so currently it shouldn't be an issue. HI should note though that how we handle pinch/zoom and scroll is still being worked on. We want to get the async pan zoom controller attached to either <browser> or maybe scrollable frames, at which point we are going to have to sort out interaction between selection / selection markers / async scroll and zoom.
Comment on attachment 733128 [details] [diff] [review]
(Part 1 v2) Add _targetIsEditable and _contentOffset properties

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +475,5 @@
>      let radius = ElementTouchHelper.getTouchRadius();
> +    return (aX - this._contentOffset.x > rangeRect.left - radius.left &&
> +            aX - this._contentOffset.x < rangeRect.right + radius.right &&
> +            aY - this._contentOffset.y > rangeRect.top - radius.top &&
> +            aY - this._contentOffset.y < rangeRect.bottom + radius.bottom);

We should probably stick to using a method call and saving that into a local variable like we had before. If you use a getter like this, it will have to recursively calculate the bounding client rect four times in a row, which may be expensive.
Attachment #733128 - Flags: review?(bnicholson) → review-
Comment on attachment 733143 [details] [diff] [review]
(Part 2 v2) Update _positionHandles to work with an _updateSelectionUI implementation

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

LGTM

(In reply to :Margaret Leibovic from comment #52)
> Created attachment 733143 [details] [diff] [review]
> (Part 2 v2) Update _positionHandles to work with an _updateSelectionUI
> implementation
> 
> I addressed the points in comment 45, although I decided not to factor out
> the blocks to add the start/end handles to the positions array because
> they're different enough that it makes it annoying (mainly where they use
> this._cache.start vs this._cache.end).

Oops, didn't notice that difference when I made that suggestion.
Attachment #733143 - Flags: review?(bnicholson) → review+
Is it already possible to test this without manual compiling?
Status: REOPENED → ASSIGNED
(In reply to Frederik Niedernolte from comment #57)
> Is it already possible to test this without manual compiling?

Sorry, it isn't, I don't have an APK available.

I'm also sorry I haven't updated this bug recently. I found that this approach actually regressed responsiveness, so I put this on the back burner. I've been investigating making some simpler incremental patches with document.caretPositionFromPoint, but I'm also having some trouble with that because the offset it returns isn't always what we want if we pass in points outside of the rect that contains the nearby text we intend to select (to start, I just wrote a patch to use caretPositionFromPoint to move the caret, and this is where I was experiencing these problems).
New approach! Or rather, back to blassey's original approach :/

This actually works pretty well, especially in regular static text content. Unfortunately, there's definitely still some wonkiness in editable elements, mostly related to what I mentioned in my last comment. However, I feel like our current text selection in editable elements isn't great either, so there may not be much of a regression here. I played around with reusing some of the logic that's currently in _sendMouseEvents to restrict the selection bounds in text areas, and that seems promising, but I think that would be better in a separate bug.

I got rid of the selection listener in this patch partially because we're collapsing the selection ourselves now, but it also seemed to be interacting poorly with updating the selection in editable elements. I think we can file a follow-up to figure out how to address this.
Attachment #733128 - Attachment is obsolete: true
Attachment #733143 - Attachment is obsolete: true
Attachment #733144 - Attachment is obsolete: true
Attachment #739329 - Flags: review?(bnicholson)
I'm a bit more unsure about this patch. We get some unwanted offset values if we move the caret close to the edge of the input, so I think we need to do more to restrict what coordinates we pass to caretPositionFromPoint.
Attachment #739332 - Flags: feedback?(bnicholson)
I put an APK here in case anyone wants to try it out: https://dl.dropboxusercontent.com/u/3358452/text-select.apk
Comment on attachment 739329 [details] [diff] [review]
Use caretPositionFromPoint to move selection

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ -213,5 @@
>      }
>  
> -    // Add a listener to end the selection if it's removed programatically
> -    selection.QueryInterface(Ci.nsISelectionPrivate).addSelectionListener(this);
> -

So just to be sure, we still need to remove this with your new implementation?
Attachment #739329 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #62)
> Comment on attachment 739329 [details] [diff] [review]
> Use caretPositionFromPoint to move selection
>
> So just to be sure, we still need to remove this with your new
> implementation?

Oops, nevermind...just saw comment 59.
Comment on attachment 739332 [details] [diff] [review]
Use caretPositionFromPoint to move caret

Using this patch, I see really erratic behavior on text areas (e.g., http://people.mozilla.com/~bnicholson/test/form.html). But if caretPositionFromPoint() is giving us bad values, I don't know how we could improve it. Maybe we should just wait for caretPositionFromPoint() to be more reliable before trying to replace these mouse events?
(In reply to Brian Nicholson (:bnicholson) from comment #64)
> Comment on attachment 739332 [details] [diff] [review]
> Use caretPositionFromPoint to move caret
> 
> Using this patch, I see really erratic behavior on text areas (e.g.,
> http://people.mozilla.com/~bnicholson/test/form.html). But if
> caretPositionFromPoint() is giving us bad values, I don't know how we could
> improve it. Maybe we should just wait for caretPositionFromPoint() to be
> more reliable before trying to replace these mouse events?

I agree this requires further investigation. I'll file a follow-up bug for figuring out how to finish the patch for moving the caret, since I don't want it to hold up landing the first patch.

I pushed the first patch to try, and I'll file more bugs for the known issues.

https://hg.mozilla.org/integration/mozilla-inbound/rev/445e4da45a6f
Summary: Kill fake mouse event hacks in SelectionHandler → Kill fake mouse event hacks used for text selection
Depends on: 864582
Comment on attachment 739332 [details] [diff] [review]
Use caretPositionFromPoint to move caret

Moving this patch to bug 864582.
Attachment #739332 - Attachment is obsolete: true
Attachment #739332 - Flags: feedback?(bnicholson)
Depends on: 864586
Duplicate of this bug: 780301
Duplicate of this bug: 821277
Depends on: 864589
Duplicate of this bug: 783090
https://hg.mozilla.org/mozilla-central/rev/445e4da45a6f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Depends on: 960203
Depends on: 1002882
You need to log in before you can comment on or make changes to this bug.