Confine cursor positioning thumb control to edit box

VERIFIED FIXED in Firefox 19

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: aaronmt, Assigned: bnicholson)

Tracking

Trunk
Firefox 20
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18+ affected, firefox19+ fixed, firefox20 verified, firefox21 verified, fennec18+)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
Assignee: nobody → sriram
tracking-fennec: ? → 18+
Duplicate of this bug: 718393
Sriram, what is the status here?
Flags: needinfo?(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).
Flags: needinfo?(sriram)
Duplicate of this bug: 816241
(Assignee)

Comment 5

6 years ago
Created attachment 688977 [details] [diff] [review]
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.
Assignee: sriram → bnicholson
Status: NEW → ASSIGNED
Attachment #688977 - Flags: review?(wjohnston)
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?
(Assignee)

Comment 7

6 years ago
(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
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!
(Assignee)

Comment 9

6 years ago
Created attachment 689026 [details] [diff] [review]
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.
Attachment #688977 - Attachment is obsolete: true
Attachment #688977 - Flags: review?(wjohnston)
Attachment #689026 - Flags: review?(wjohnston)
(Assignee)

Comment 10

6 years ago
I've updated the linked build with the latest patch (http://dl.dropbox.com/u/35559547/fennec-drag-cursor.apk).
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?
Attachment #689026 - Flags: review?(wjohnston) → review-
Need a new patch from brian
Flags: needinfo?(bnicholson)
(Assignee)

Comment 13

6 years ago
Created attachment 692094 [details] [diff] [review]
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.
Attachment #689026 - Attachment is obsolete: true
Attachment #692094 - Flags: review?(wjohnston)
Flags: needinfo?(bnicholson)
(Assignee)

Comment 14

6 years ago
Created attachment 692100 [details] [diff] [review]
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.
Attachment #692100 - Flags: review?(wjohnston)
(Assignee)

Comment 15

6 years ago
Created attachment 692102 [details] [diff] [review]
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.
Attachment #692102 - Flags: review?(wjohnston)
(Assignee)

Comment 16

6 years ago
Updated build with these patches, along with the fix in bug 818715: http://dl.dropbox.com/u/35559547/fennec-drag-cursor.apk
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?
Attachment #692094 - Flags: review?(wjohnston) → review-
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.
Attachment #692100 - Flags: review?(wjohnston) → review+
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.
Attachment #692102 - Flags: review?(wjohnston) → review+
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.
Attachment #692094 - Flags: review- → review+
(Assignee)

Updated

6 years ago
status-firefox18: --- → affected
status-firefox19: --- → affected
https://hg.mozilla.org/mozilla-central/rev/ffe29f6052f2
https://hg.mozilla.org/mozilla-central/rev/9af5a57f905a
https://hg.mozilla.org/mozilla-central/rev/7f3b0fc0e464
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
(Assignee)

Updated

6 years ago
Depends on: 823321
(Assignee)

Updated

6 years ago
Depends on: 823325
(Assignee)

Comment 23

6 years ago
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.
tracking-firefox18: --- → ?
(Assignee)

Comment 24

6 years ago
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
Attachment #692094 - Flags: approval-mozilla-beta?
Attachment #692094 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 25

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

See above
Attachment #692100 - Flags: approval-mozilla-beta?
Attachment #692100 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 26

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

See above
Attachment #692102 - Flags: approval-mozilla-beta?
Attachment #692102 - Flags: approval-mozilla-aurora?
Comment on attachment 692094 [details] [diff] [review]
Part 1: Cleanup for TextSelection/TextSelectionHandle

low risk fix for a firefox18 regression.Approving on aurora
Attachment #692094 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #692100 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #692102 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 28

6 years ago
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.
Status: RESOLVED → VERIFIED
status-firefox20: --- → affected
status-firefox21: --- → verified
(Reporter)

Updated

6 years ago
status-firefox20: affected → verified
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.
Attachment #692094 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

6 years ago
Attachment #692100 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

6 years ago
Attachment #692102 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

6 years ago
tracking-firefox18: ? → +

Updated

6 years ago
tracking-firefox19: --- → +
You need to log in before you can comment on or make changes to this bug.