774938 - Use native handles for text selection

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[:Margaret Leibovic]

Attachment: WIP

I found that setting style.top/style.left on the handles is what's causing the reflows and killing performance, even though we had hoped that wouldn't be the case because they are position:absolute; elements. If we can't find a way around these slow reflows, it seems like native handles are the way to go.

I have a WIP to start using native handles. In my first pass, I kept invisible (blue for debugging) XBL handles to act as touch event listeners to update the selection. This works well on touchmove, but runs into problems again when we have to update their position on touchend, so we'll want to get rid of this XBL altogether and send down some coordinates from Java to update the selection. As long as we're sending the same coordiantes that the touch events are already sending, I feel like this should work pretty well.

So, things this patch needs:
-Actually update the position of the native handles on touchmove (right now they only update on touchend). We should do this in TextSelectionHandle.java.
-Get rid of the XBL and touch event listeners in JS, and replace them with observers that listen for coordinates from TextSelectionHandle.java.
-Update the position of the handles when we pan (it already works when we zoom, thanks to exisiting code!)

Comment 1

[:Margaret Leibovic]

Attachment: WIP v2

Getting there. This patch basically works, but there are some rough edges to work on:
-Dragging the start handle doesn't work yet (need to do something smarter to differentiate between the handles in Java)
-Updating when panning/zooming
-Reversing the handles when the selection area flips (follow-up bug material)

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 643405 [details] [diff] [review]
WIP v2

Would it make sense to move the start and end handles (in Java) into a parent class (TextSelection) and use it to handle the Gecko messages too?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

TextSelection.java could hold the TextSelection manager and the TextSelectionHandle classes

Comment 4

[:Margaret Leibovic]

Attachment: patch

This is close enough that we may be able to land it and handle other issues in follow-up bugs. Aaron and I have been using this etherpad to start tracking issues:
https://etherpad.mozilla.org/native-handles

Few things to note:
-I'm just repositioning the handles after the viewport changes to deal with panning/zooming. This looks laggy, but it gets the job done.
-I'm reversing the handles on touch end if the selection flipped. It would be nicer to reverse them as soon as the selection flips, but this first pass was simpler.
-The handles automatically hide themselves if they shouldn't be on screen, but I still need to handle the case when you drag them too close to the edge of the screen (Aaron and I noticed that Android shrinks them if there's not enough room for them near the edge of the screen, and that looks kinda weird).

I put an APK here if anyone wants to try:
http://people.mozilla.com/~mleibovic/native-handles.apk

Comment 5

[:Margaret Leibovic]

Attachment: patch v2

This patch fixes selection in sub-frames.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 643692 [details] [diff] [review]
patch v2

Tested the APK. This is huge improvement. Excellent work.

>+        if (mTextSelection != null)
>+            mTextSelection.destroy();

Looks like you are handling this object in a way that will not break the "Don't keep activities" fix. That is good.

>diff --git a/mobile/android/base/TextSelection.java b/mobile/android/base/TextSelection.java

>+public class TextSelection implements GeckoEventListener {
>+    private static final String LOGTAG = "TextSelection";

Out of convention, we typically add "Gecko" to the logtag so it's easier to filter on all gecko output


>+    public void handleMessage(String event, JSONObject message) {
>+                Log.i(LOGTAG, "ShowHandles");

>+                Log.i(LOGTAG, "HideHandles");

>+                Log.i(LOGTAG, "PositionHandles: " + startLeft + ", " + startTop + "; " + endLeft + ", " + endTop);

Feel free to drop these logs before checkin, just so we don't pile them up.

>diff --git a/mobile/android/base/TextSelectionHandle.java b/mobile/android/base/TextSelectionHandle.java

>+public class TextSelectionHandle extends ImageView implements View.OnTouchListener {
>+    private static final String LOGTAG = "TextSelectionHandle";

Same

>+    private boolean mIsStartHandle;

You might think I am crazy, but I don't like this boolean (used anywhere in the patch). Can we just use a string? and call the variable "mHandleType" ? Or an enum in Java and a convert it from a string in XML and JSON? I prefer the enum, but that could be too much work for now. If you think the string change is too much for now, we can file a followup bug and just focus on the feature itself.

>+    public TextSelectionHandle(Context context, AttributeSet attrs) {

>+        TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.TextSelectionHandle);
>+        mIsStartHandle = a.getBoolean(R.styleable.TextSelectionHandle_isStartHandle, false);

          mHandleType = a.getString(R.styleable.TextSelectionHandle_handleType);
          if (mHandleType == null)
              mHandleType = "start";

since getString can return null

>+                try {
>+                    args.put("isStartHandle", mIsStartHandle);

"isStartHandle" -> "handleType"

>diff --git a/mobile/android/base/gfx/LayerController.java b/mobile/android/base/gfx/LayerController.java

>+    public PointF convertLayerPointToViewPoint(PointF layerPoint) {
>+        if (mLayerClient == null) {
>+            return null;
>+        }
>+
>+        ImmutableViewportMetrics viewportMetrics = mViewportMetrics;
>+        PointF origin = viewportMetrics.getOrigin();
>+        float zoom = viewportMetrics.zoomFactor;
>+        ViewportMetrics geckoViewport = mLayerClient.getGeckoViewportMetrics();
>+        PointF geckoOrigin = geckoViewport.getOrigin();
>+        float geckoZoom = geckoViewport.getZoomFactor();
>+
>+        PointF viewPoint = new PointF(
>+                ((layerPoint.x + (geckoOrigin.x / geckoZoom)) * zoom - origin.x),
>+                ((layerPoint.y + (geckoOrigin.y / geckoZoom)) * zoom - origin.y));
>+
>+        return viewPoint;
>+    }

Let's get Kats to review this part

>diff --git a/mobile/android/base/resources/layout-xlarge/gecko_app.xml b/mobile/android/base/resources/layout-xlarge/gecko_app.xml
>diff --git a/mobile/android/base/resources/layout/gecko_app.xml b/mobile/android/base/resources/layout/gecko_app.xml

I think we need to add this to web_app.xml too
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/web_app.xml

>diff --git a/mobile/android/base/resources/layout/text_selection_handles.xml.in b/mobile/android/base/resources/layout/text_selection_handles.xml.in

>+   <org.mozilla.gecko.TextSelectionHandle android:id="@+id/start_handle"
>+                                          gecko:isStartHandle="true"/>

>+   <org.mozilla.gecko.TextSelectionHandle android:id="@+id/end_handle"
>+                                          gecko:isStartHandle="false"/>

handleType

>diff --git a/mobile/android/base/resources/values/attrs.xml b/mobile/android/base/resources/values/attrs.xml

>+    <declare-styleable name="TextSelectionHandle">
>+         <attr name="isStartHandle" format="boolean"/>
>+    </declare-styleable>

handleType, string

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>+    Services.obs.addObserver(this, "TextSelection:MoveSelection", false);
>+    Services.obs.addObserver(this, "TextSelection:TouchEnd", false);

Naming OCD kicking in. You use "XxxHandles" for anything handle specific, so let's assume that anything else is "selection" specific. Therefore, "Selection" is redundant. Long story short: 
TextSelection:MoveSelection -> TextSelection:Move
TextSelection:TouchEnd ->TextSelection:End

r+ with the nits. think about the isStartHandle -> string or enum change

You might want to fix up any of the JS errors listed in the etherpad before landing or very quickly after landing.

Comment 7

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #6)

> You might think I am crazy, but I don't like this boolean (used anywhere in
> the patch). Can we just use a string? and call the variable "mHandleType" ?
> Or an enum in Java and a convert it from a string in XML and JSON? I prefer
> the enum, but that could be too much work for now. If you think the string
> change is too much for now, we can file a followup bug and just focus on the
> feature itself.

Yeah, I thought about this before but punted on it. Using "mHandleType" will be better for when we make a handle to position the cursor.

> >+    Services.obs.addObserver(this, "TextSelection:MoveSelection", false);
> >+    Services.obs.addObserver(this, "TextSelection:TouchEnd", false);
> 
> Naming OCD kicking in. You use "XxxHandles" for anything handle specific, so
> let's assume that anything else is "selection" specific. Therefore,
> "Selection" is redundant. Long story short: 
> TextSelection:MoveSelection -> TextSelection:Move
> TextSelection:TouchEnd ->TextSelection:End

I'm okay on "TextSelection:Move", but "TextSelection:End" is misleading, since it isn't the end of the selection, it's just the end of the touch. Maybe I can call it "TextSelection:Position", since the handler is updating the cache/handle position.

> You might want to fix up any of the JS errors listed in the etherpad before
> landing or very quickly after landing.

Yeah, I'm a little worried about some of those, since Aaron ran into one case where Gecko totally did. I'll try to add some better error handling.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 643692 [details] [diff] [review]
patch v2

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

The point conversion code looks fine to me. Also noted a bunch of nits that would be nice to fix up before landing to make less work for cpeterson and his proguard patches.

::: mobile/android/base/TextSelection.java
@@ +8,5 @@
> +import android.view.View;
> +
> +import org.json.JSONObject;
> +
> +public class TextSelection implements GeckoEventListener {

You can leave this class package-scope instead of public. That way proguard will be able to optimize it more, and it's also better for modularity.

@@ +12,5 @@
> +public class TextSelection implements GeckoEventListener {
> +    private static final String LOGTAG = "TextSelection";
> +
> +    private TextSelectionHandle mStartHandle;
> +    private TextSelectionHandle mEndHandle;

These can be final

@@ +14,5 @@
> +
> +    private TextSelectionHandle mStartHandle;
> +    private TextSelectionHandle mEndHandle;
> +
> +    public TextSelection(TextSelectionHandle startHandle, TextSelectionHandle endHandle) {

This can be package-scope.

@@ +23,5 @@
> +        GeckoAppShell.registerGeckoEventListener("TextSelection:HideHandles", this);
> +        GeckoAppShell.registerGeckoEventListener("TextSelection:PositionHandles", this);
> +    }
> +
> +    public void destroy() {

This can be package-scope

::: mobile/android/base/TextSelectionHandle.java
@@ +21,5 @@
> +    private static final String LOGTAG = "TextSelectionHandle";
> +
> +    private boolean mIsStartHandle;
> +    private int mWidth;
> +    private int mHeight;

You can make mWidth and mHeight final

@@ +25,5 @@
> +    private int mHeight;
> +    private int mLeft;
> +    private int mTop;
> +    private int mTouchDeltaX;
> +    private int mTouchDeltaY;

These variables are confusingly named; mTouchDeltaX and mTouchDeltaY are really more like mTouchStartX and mTouchStartY since they store a screen coordinate rather than a delta. Also the arguments to move() should be newX and newY.

@@ +27,5 @@
> +    private int mTop;
> +    private int mTouchDeltaX;
> +    private int mTouchDeltaY;
> +
> +    public TextSelectionHandle(Context context, AttributeSet attrs) {

Constructor can be package-scope

@@ +67,5 @@
> +        }
> +        return true;
> +    }
> +
> +    public void move(int deltaX, int deltaY) {

This function should be private

@@ +72,5 @@
> +        mLeft = mLeft + deltaX - mTouchDeltaX;
> +        mTop = mTop + deltaY - mTouchDeltaY;
> +
> +        // Send x coordinate on the right side of the start handle, left side of the end handle.
> +        PointF geckoPoint = new PointF(new Point(mLeft + (mIsStartHandle ? mWidth : 0), mTop));

Can you just use the new PointF(x, y) constructor here, rather than creating a Point object which isn't really needed? (If so, also take out the Point import statement)

@@ +73,5 @@
> +        mTop = mTop + deltaY - mTouchDeltaY;
> +
> +        // Send x coordinate on the right side of the start handle, left side of the end handle.
> +        PointF geckoPoint = new PointF(new Point(mLeft + (mIsStartHandle ? mWidth : 0), mTop));
> +        geckoPoint = GeckoApp.mAppContext.getLayerController().convertViewPointToLayerPoint(geckoPoint);

getLayerController() can return null here because of the "don't keep activities" business. Add a null check and just abort if it is.

@@ +91,5 @@
> +        params.topMargin = mTop;
> +        setLayoutParams(params);
> +    }
> +
> +    public void positionFromGecko(int left, int top) {

This function can be package-scope

@@ +92,5 @@
> +        setLayoutParams(params);
> +    }
> +
> +    public void positionFromGecko(int left, int top) {
> +        PointF geckoPoint = new PointF(new Point(left, top));

use new PointF(x,y)

@@ +93,5 @@
> +    }
> +
> +    public void positionFromGecko(int left, int top) {
> +        PointF geckoPoint = new PointF(new Point(left, top));
> +        geckoPoint = GeckoApp.mAppContext.getLayerController().convertLayerPointToViewPoint(geckoPoint);

getLayerController() can return null

::: mobile/android/base/gfx/LayerController.java
@@ +319,5 @@
> +        float geckoZoom = geckoViewport.getZoomFactor();
> +
> +        PointF viewPoint = new PointF(
> +                ((layerPoint.x + (geckoOrigin.x / geckoZoom)) * zoom - origin.x),
> +                ((layerPoint.y + (geckoOrigin.y / geckoZoom)) * zoom - origin.y));

This looks good to me

Comment 9

[:Margaret Leibovic]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/626c04a08a18

Let's start filing/fixing follow-up bugs, and get this uplifted ASAP!

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/626c04a08a18

Comment 11

[:Margaret Leibovic]

Comment on attachment 643692 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this vastly improves the performance of the new text selection feature
User impact if declined: text selection can be really slow on some pages (bug 772140)
Testing completed (on m-c, etc.): landed on m-c 7/20
Risk to taking this patch (and alternatives if risky): there are known dependencies, most of which have patches, but there could be more correctness regressions from the XBL text selection handles, since this has had less test coverage (however, minor regressions would be worth it for the perf win we get here)
String or UUID changes made by this patch: n/a

Comment 13

[:Margaret Leibovic]

I duped bug 772140 to this bug, and that was tracking firefox 15/16. Sorry for the flag churn!

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 643692 [details] [diff] [review]
patch v2

Thanks for keeping the bugs up to date.  Approving for branches, please uplift today if possible we get this out to users in the next beta and have time for feedback.

Comment 15

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb23493f8d3c
https://hg.mozilla.org/releases/mozilla-beta/rev/d250d98d4a9a