795045 - Confine cursor positioning thumb control to edit box

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Bug 652168 added the ability to invoke a cursor position thumb control to edit boxes.

Currently when one makes a gesture drag of the handle outside the edit box, the keyboard dismisses, the selection is lost and the handle is dismissed. Instead, akin to Chrome/Stock/iPhone Safari, the handle should move left and right despite a user attempting to drag the handle outside the confines of the edit box area.

For text-area's with multiple rows (which handle movement is currently broken), the handle should jump to the end of the nearest character directly below it.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Sriram, what is the status here?

Comment 3

[Sriram Ramasubramanian [:sriram]]

I am overloaded with private browsing and need to study the code for this to work on it. I didn't do much (other than copy paste) for cursor at java side. It would be able to take it next week or the week after (thanksgiving).

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Lock handle to caret position

This patch locks the handle position to the position of the caret. Unlike text selection, the handle doesn't move freely; its position is only change after Gecko has clicked, set the cursor, and given us the position of the cursor.

One of the big problems I had making this change is getting the correct coordinates for the click. Before, newX and newY were relative to the handle, and the handle always moved with the finger. This worked fine since we could get the click coordinates by using the current touch position with the handle offset. But since the handle now sticks to the caret position, relative coordinates become useless since the offset relative to the top left of the handle (mTouchStartX/Y) is constantly changing. The best I could come up with was using absolute coordinates to set the handle positions, which requires offsetting the coordinates with the LayerView position.

Clicking an input field usually causes the keyboard to appear, resulting in a window resize; this made the cursor appear and quickly disappear whenever an unfocused textbox was clicked. This happens because we call hideThumb() in Window:Resize, so I just removed this call.

Comment 6

[Wesley Johnston (:wesj)]

How is the performance doing this?

I'd kinda like, while you're fixing this, if we could switch to using DOMWindowUtils.SelectAtPoint. Performance there should be better, as well as it fixing any number of other bugs that might happen because we're clicking like mad here. You may have to do some work there to make it support a "nothing" selection type though: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h#353. That's probably a completely different bug though.

In the mean time, you mind throwing a build up?

Comment 7

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #6)
> How is the performance doing this?
> 
> I'd kinda like, while you're fixing this, if we could switch to using
> DOMWindowUtils.SelectAtPoint. Performance there should be better, as well as
> it fixing any number of other bugs that might happen because we're clicking
> like mad here. You may have to do some work there to make it support a
> "nothing" selection type though:
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h#353.
> That's probably a completely different bug though.

Performance isn't great, especially with bug 818715; if we could position the cursor without having to send a click for every move, that would be excellent.

> In the mean time, you mind throwing a build up?

Sure: http://dl.dropbox.com/u/35559547/fennec-drag-cursor.apk

Comment 8

[Wesley Johnston (:wesj)]

Talked to finkle. We'll move the DOMWindowUtils stuff to a different bug. Maybe 818715 if you want to repurpose it (I would guess its related to Gecko being flooded with crazy mouse events). I'll review this as it!

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Lock handle to caret position, v2

In the previous patch, the handle still disappeared if moved to the edges of an input field or text area where the text overscrolled its container. This patch clamps the click to the smallest dimension of the text and editor rects. It still needs work since the editor should scroll when the handle is at the edge, but this is at least an improvement.

Comment 10

[Brian Nicholson (:bnicholson)]

I've updated the linked build with the latest patch (http://dl.dropbox.com/u/35559547/fennec-drag-cursor.apk).

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 689026 [details] [diff] [review]
Lock handle to caret position, v2

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

This patch might be fine. Just had a few questions.

::: mobile/android/base/TextSelectionHandle.java
@@ +72,5 @@
>                  mTouchStartX = Math.round(event.getX());
>                  mTouchStartY = Math.round(event.getY());
> +
> +                int[] rect = new int[2];
> +                LayerView layerView = GeckoApp.mAppContext.getLayerView();

This is an ImageView so ((GeckoApp)getContext()) should work. There are two other calls to get the layerView in here as well. Maybe we can consolidate them (and make ram happy).

::: mobile/android/chrome/content/browser.js
@@ +1920,5 @@
> +      range.selectNodeContents(this._target.QueryInterface(Ci.nsIDOMNSEditableElement).editor.rootElement);
> +      let textRect = range.getBoundingClientRect();
> +
> +      // Get rect of editor
> +      let editorRect = this._cwu.sendQueryContentEvent(this._cwu.QUERY_EDITOR_RECT, 0, 0, 0, 0);

I think we'd be better to use Geometry.jsm's rect class here (its already lazily imported into Fennec):

https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Geometry.jsm/Rect#intersect%28%29

You'll have to do a little work to build the rects, but its cleaner. This rect should actually be fairly constant right? Maybe we can skip this extra work...

@@ +1934,5 @@
> +      let right = (textRight < editorRight) ? textRight : editorRight;
> +
> +      // Clamp vertically
> +      if (aY < top + 1) {
> +        aY = top + 1;

Why the +1 here and not for the bottom piece? If there's a good reason, a comment explaining it is fine. Same below.

@@ +1950,3 @@
>      // Send mouse event 1px too high to prevent selection from entering the line below where it should be
> +    this._cwu.sendMouseEventToWindow("mousedown", aX - 1, aY - 1, 0, 0, useShift ? Ci.nsIDOMNSEvent.SHIFT_MASK : 0, true);
> +    this._cwu.sendMouseEventToWindow("mouseup", aX - 1, aY - 1, 0, 0, useShift ? Ci.nsIDOMNSEvent.SHIFT_MASK : 0, true);

Why are we shifting x now too?

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Need a new patch from brian

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Cleanup for TextSelection/TextSelectionHandle

(In reply to Wesley Johnston (:wesj) from comment #11)
> ::: mobile/android/base/TextSelectionHandle.java
> @@ +72,5 @@
> >                  mTouchStartX = Math.round(event.getX());
> >                  mTouchStartY = Math.round(event.getY());
> > +
> > +                int[] rect = new int[2];
> > +                LayerView layerView = GeckoApp.mAppContext.getLayerView();
> 
> This is an ImageView so ((GeckoApp)getContext()) should work. There are two
> other calls to get the layerView in here as well. Maybe we can consolidate
> them (and make ram happy).

I hate making assumptions like this that aren't guaranteed in the API, but we do this in a number of other places, so I guess doing it here won't hurt. And it's not like GeckoApp.mAppContext is any better.

I've also tried to clean up TextSelection.java since it has the same problems.

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Don't round coordinates until they're used

(In reply to Wesley Johnston (:wesj) from comment #11)
> @@ +1934,5 @@
> > +      let right = (textRight < editorRight) ? textRight : editorRight;
> > +
> > +      // Clamp vertically
> > +      if (aY < top + 1) {
> > +        aY = top + 1;
> 
> Why the +1 here and not for the bottom piece? If there's a good reason, a
> comment explaining it is fine. Same below.
> 
> @@ +1950,3 @@
> >      // Send mouse event 1px too high to prevent selection from entering the line below where it should be
> > +    this._cwu.sendMouseEventToWindow("mousedown", aX - 1, aY - 1, 0, 0, useShift ? Ci.nsIDOMNSEvent.SHIFT_MASK : 0, true);
> > +    this._cwu.sendMouseEventToWindow("mouseup", aX - 1, aY - 1, 0, 0, useShift ? Ci.nsIDOMNSEvent.SHIFT_MASK : 0, true);
> 
> Why are we shifting x now too?

Yeah, I wasn't exactly sure why I needed these changes, but things weren't working properly without them. We round floats many times, which may have been causing compounding precision loss. In general, I think it's best to round as late as possible, so I made a patch to carry floats as long as we can.

Also, I changed Math.round() to simply truncate via casting to int. I'm thinking of it like this: the screen is a grid of 1x1 pixels, where the first pixel's range is 0-0.999~, the second pixel is 1.0-1.999~, etc. If we round up, that means that any values from 0.5-0.999~ actually moves it to the next pixel, which breaks the 1:1 mapping from the screen dimensions to Gecko's dimensions.

Comment 15

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Lock handle to caret position and add scrolling support

(In reply to Wesley Johnston (:wesj) from comment #11)
> ::: mobile/android/chrome/content/browser.js
> @@ +1920,5 @@
> > +      range.selectNodeContents(this._target.QueryInterface(Ci.nsIDOMNSEditableElement).editor.rootElement);
> > +      let textRect = range.getBoundingClientRect();
> > +
> > +      // Get rect of editor
> > +      let editorRect = this._cwu.sendQueryContentEvent(this._cwu.QUERY_EDITOR_RECT, 0, 0, 0, 0);
> 
> I think we'd be better to use Geometry.jsm's rect class here (its already
> lazily imported into Fennec):
> 
> https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Geometry.
> jsm/Rect#intersect%28%29
> 
> You'll have to do a little work to build the rects, but its cleaner. This
> rect should actually be fairly constant right? Maybe we can skip this extra
> work...

Nice, thanks. I went with restrictTo() under the assumption that it's slightly more efficient since it reuses the given Rect. I tried caching the editor's rect, but it appears to change whenever the zoom level changes...and we can't cache the text's rect either since that changes when the field is scrolled.

Comment 16

[Brian Nicholson (:bnicholson)]

Updated build with these patches, along with the fix in bug 818715: http://dl.dropbox.com/u/35559547/fennec-drag-cursor.apk

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 692094 [details] [diff] [review]
Part 1: Cleanup for TextSelection/TextSelectionHandle

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

I don't love putting everything on the ui thread, or passing this mActivity around either.

::: mobile/android/base/TextSelection.java
@@ +69,5 @@
>          }
>      }
>  
> +    public void handleMessage(final String event, final JSONObject message) {
> +        mActivity.runOnUiThread(new Runnable() {

This can just be handle.post()

@@ +82,5 @@
>  
> +                        mViewLeft = 0.0f;
> +                        mViewTop = 0.0f;
> +                        mViewZoom = 0.0f;
> +                        LayerView layerView = mActivity.getLayerView();

Maybe I'm being pedantic. I don't like passing around these Activities. Looking at this briefly, seems like we could add a getLayerView message to the handles. We've got null checks after each one already so, we're fine and the handle can just do something like:

if (!(handle.getContext() instanceof GeckoApp)) return null;

What do you think?

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 692100 [details] [diff] [review]
Part 2: Don't round coordinates until they're used

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

I love this one.

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 692102 [details] [diff] [review]
Part 3: Lock handle to caret position and add scrolling support

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

This looks good to me.

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 692094 [details] [diff] [review]
Part 1: Cleanup for TextSelection/TextSelectionHandle

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

Talked this through on irc.

Comment 21

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffe29f6052f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9af5a57f905a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3b0fc0e464

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ffe29f6052f2
https://hg.mozilla.org/mozilla-central/rev/9af5a57f905a
https://hg.mozilla.org/mozilla-central/rev/7f3b0fc0e464

Comment 23

[Brian Nicholson (:bnicholson)]

This was originally supposed to make 18, but missed the deadline. It's been on Nightly for several weeks without any dependencies created (other than the ones I filed), so I'd consider it low risk. Without this patch, it can be very difficult to position the cursor handle inside of a textbox.

Comment 24

[Brian Nicholson (:bnicholson)]

Comment on attachment 692094 [details] [diff] [review]
Part 1: Cleanup for TextSelection/TextSelectionHandle

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 652168
User impact if declined: cursor handles are difficult to use in input fields
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk; has baked for several weeks with no added regressions
String or UUID changes made by this patch: none

Comment 25

[Brian Nicholson (:bnicholson)]

Comment on attachment 692100 [details] [diff] [review]
Part 2: Don't round coordinates until they're used

See above

Comment 26

[Brian Nicholson (:bnicholson)]

Comment on attachment 692102 [details] [diff] [review]
Part 3: Lock handle to caret position and add scrolling support

See above

Comment 27

[bhavana bajaj [:bajaj]]

Comment on attachment 692094 [details] [diff] [review]
Part 1: Cleanup for TextSelection/TextSelectionHandle

low risk fix for a firefox18 regression.Approving on aurora

Comment 28

[Aaron Train [:aaronmt]]

Fixed a while ago, verifying confine to inputs (e.g, data:text/html,<input/>) on trunk (21) (mozilla-central) via Samsung Galaxy Note II (Android 4.1.2), and HTC Nexus One (Android 2.3.4)

>This was originally supposed to make 18, but missed the deadline. It's been on Nightly for several weeks without any >dependencies created (other than the ones I filed), so I'd consider it low risk. Without this patch, it can be very difficult to >position the cursor handle inside of a textbox.

Agreed.

Comment 29

[Alex Keybl [:akeybl]]

Comment on attachment 692094 [details] [diff] [review]
Part 1: Cleanup for TextSelection/TextSelectionHandle

Approving for Beta as well, in preparation for the possibility of inclusion in a FF18.0.1. Please land Monday to make it into FF18 beta 2.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2599378e0df3
https://hg.mozilla.org/releases/mozilla-beta/rev/0f62d48fd77a
https://hg.mozilla.org/releases/mozilla-beta/rev/1797a13d3d2b