775722 - Native handles lag when panning/zooming

Status: VERIFIED FIXED

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

Description

[:Margaret Leibovic]

Chrome hides the handles during panning/zooming. Seems like a reasonable solution to me.

Comment 1

[:Margaret Leibovic]

Kats, is there an easy way for me to listen for pan/zoom start/end?

Comment 2

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

Not really. It's actually probably easier to make the handles move during panning/zooming since Java gets called during the composition as the content is moving around and we already have hooks for that sort of thing. Would you consider doing that instead? If so, the place you want to reposition the handles is in gfx/LayerRenderer.java, in the Frame.drawForeground function, probably after the extra layers loop but before the scrollbars. The mFrameMetrics object accessible to that code represents the viewport being drawn, and is equivalent to the mViewportMetrics object that you used in convertLayerPointToViewPoint.

Also note that the drawForeground() method is called on the compositor thread so you may need to post something to the UI thread to do the actual moving once you've calculated where the handles should go on the screen.

If you still want to hide the handles instead then I can look into adding a hook into PanZoomController that notifies you when pan/zoom is in progress.

Comment 3

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

Attachment: Make handles move with panning/zooming (WIP)

This is roughly what I had in mind. I haven't even compiled this code though, so no idea if it works.

Comment 4

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

If you want to go the route of hiding the handles anyway, bug 775951 has some WIPs to add the necessary hooks. The first two patches on that bug should probably land regardless since they're good to have.

Comment 5

[:Margaret Leibovic]

Thanks, Kats. I assumed hiding/showing the handles would be an easier temporary solution, but moving the handles while panning/zooming would be the ideal solution, so I'll try pursuing that.

Comment 6

[:Margaret Leibovic]

Attachment: Make handles move with panning/zooming

This patch was mostly written by kats, so I'm picking someone else to review. Kats, feel free to jump in if you think there are more things to fix here.

The handles still lag a *tiny* bit, but this is way better than it was before.

Comment 7

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

Comment on attachment 644435 [details] [diff] [review]
Make handles move with panning/zooming

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

::: mobile/android/base/GeckoApp.java
@@ +2032,5 @@
>          }
>      }
>  
> +    public void repositionTextSelectionOnPanZoom(ImmutableViewportMetrics metrics) {
> +        mTextSelection.repositionOnPanZoom(metrics);

not a fan of this simple pass through (I know we have it all over the place already, but let's not perpetuate it)

::: mobile/android/base/TextSelectionHandle.java
@@ +112,3 @@
>  
> +        // Keep track of the viewport and point for repositioning on pan/zoom
> +        mGeckoViewport = GeckoApp.mAppContext.getLayerClient().getGeckoViewportMetrics();

why not just grab a new viewport on pan/zoom?

::: mobile/android/base/gfx/LayerController.java
@@ -312,3 @@
>          PointF origin = viewportMetrics.getOrigin();
>          float zoom = viewportMetrics.zoomFactor;
> -        ViewportMetrics geckoViewport = mLayerClient.getGeckoViewportMetrics();

why change this signature?

::: mobile/android/base/gfx/LayerRenderer.java
@@ +593,5 @@
>                  }
>              }
>  
> +            if (!mPageContext.fuzzyEquals(mLastPageContext)) {
> +                GeckoApp.mAppContext.repositionTextSelectionOnPanZoom(mFrameMetrics);

so, rather than call this pass through, you would call:
GeckoApp.mAppContext.getTextSelection().repositionOnPanZoom(mFrameMetrics)

Comment 8

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

Since the comments apply to the parts of the patch that I wrote...

(In reply to Brad Lassey [:blassey] from comment #7)
> > +        // Keep track of the viewport and point for repositioning on pan/zoom
> > +        mGeckoViewport = GeckoApp.mAppContext.getLayerClient().getGeckoViewportMetrics();
> 
> why not just grab a new viewport on pan/zoom?
> 

Because it changes. The position data coming in from gecko is relative to the viewport that gecko has at the time. After pan/zoom, the getGeckoViewportMetrics() returns a different viewport. Actually even this implementation is not great because the getGeckoViewportMetrics() could have changed before the TextSelection:PositionHandles message even gets here (which is what causes the lag described in comment #6). To fix that we would actually need to send gecko's current viewport information directly from browser.js in the TextSelection:PositionHandles message and use that instead of getGeckoViewportMetrics().

This arises from the fact that getGeckoViewportMetrics is meant to be used only for java -> JS messages, and is implicitly synchronized on the UI thread. Here we are using it for JS -> java messages, and so it can end up out-of-sync.

> ::: mobile/android/base/gfx/LayerController.java
> why change this signature?
> 

To be able to pass in the stored viewport; using the latest value of getGeckoViewportMetrics() would be wrong as described above.

> ::: mobile/android/base/gfx/LayerRenderer.java
> > +            if (!mPageContext.fuzzyEquals(mLastPageContext)) {
> > +                GeckoApp.mAppContext.repositionTextSelectionOnPanZoom(mFrameMetrics);
> 
> so, rather than call this pass through, you would call:
> GeckoApp.mAppContext.getTextSelection().repositionOnPanZoom(mFrameMetrics)

While that is less code in the short run, it's worse in the long term because it is less modular - it exposes the entire text selection class as public. See http://en.wikipedia.org/wiki/Law_of_Demeter#In_object-oriented_programming (or chapter 5 of "The Pragmatic Programmer", available on Safari books online) for why.

Comment 9

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

(In reply to Kartikaya Gupta (:kats) from comment #8)
> Since the comments apply to the parts of the patch that I wrote...
> 
> (In reply to Brad Lassey [:blassey] from comment #7)
> > > +        // Keep track of the viewport and point for repositioning on pan/zoom
> > > +        mGeckoViewport = GeckoApp.mAppContext.getLayerClient().getGeckoViewportMetrics();
> > 
> > why not just grab a new viewport on pan/zoom?
> > 
> 
> Because it changes. The position data coming in from gecko is relative to
> the viewport that gecko has at the time. After pan/zoom, the
> getGeckoViewportMetrics() returns a different viewport. 
Solid reasoning, need a comment in the code explaining that
> Actually even this
> implementation is not great because the getGeckoViewportMetrics() could have
> changed before the TextSelection:PositionHandles message even gets here
> (which is what causes the lag described in comment #6). To fix that we would
> actually need to send gecko's current viewport information directly from
> browser.js in the TextSelection:PositionHandles message and use that instead
> of getGeckoViewportMetrics().

Any reason not to do this?

 
> > ::: mobile/android/base/gfx/LayerRenderer.java
> > > +            if (!mPageContext.fuzzyEquals(mLastPageContext)) {
> > > +                GeckoApp.mAppContext.repositionTextSelectionOnPanZoom(mFrameMetrics);
> > 
> > so, rather than call this pass through, you would call:
> > GeckoApp.mAppContext.getTextSelection().repositionOnPanZoom(mFrameMetrics)
> 
> While that is less code in the short run, it's worse in the long term
> because it is less modular - it exposes the entire text selection class as
> public. See
> http://en.wikipedia.org/wiki/Law_of_Demeter#In_object-oriented_programming
> (or chapter 5 of "The Pragmatic Programmer", available on Safari books
> online) for why.

Less code is not the motivation here. The "right" way is to have an interface with this method and return a reference to that (either implemented by GeckoApp or TextSelection).

Comment 10

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

(In reply to Brad Lassey [:blassey] from comment #9)
> 
> Any reason not to do this?
> 

Nope, I just didn't think of it until I wrote that comment. And looking at the code, it should be even easier, since browser.js already has the data in CSS pixel coordinates. It just needs to send them relative to the page top-left rather than the current scroll position, and then in Java we can convert that to be in device pixels. I'll update the patch with this change.

> Less code is not the motivation here.

What is the motivation then?

> The "right" way is to have an
> interface with this method and return a reference to that (either
> implemented by GeckoApp or TextSelection).

Sure. Alternatively we could add the handles as a layer directly into the LayerRenderer. We discussed that solution on IRC on Friday and I would have done it that way if Layer were a clean interface rather than an abstract class. I might just extract an interface out of it and do it that way if it turns out to be cleaner.

Comment 11

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

Attachment: Make handles move with panning/zooming (v2)

Updated to:
- make TextSelection a Layer that can be added directly to LayerRenderer
- make the TextSelection:PositionHandles event provide page-relative coordinates
- remove the LayerController.convertLayerPointToViewPoint method which is no longer needed (and in fact should never be used because of the discussion in comment 8).

Comment 12

[:Margaret Leibovic]

Comment on attachment 645091 [details] [diff] [review]
Make handles move with panning/zooming (v2)

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

::: mobile/android/chrome/content/browser.js
@@ +1849,5 @@
>      // 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();
> +    let scrollX = this._view.top.scrollX;
> +    let scrollY = this._view.top.scrollY;

We found that scrollX/scrollY cause reflows, so it would be better to use nsIDOMWindowUtils's getScrollXY here. See bug 773864 (we don't use that code anymore, but the idea is still valid).

Comment 13

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

Attachment: Make handles move with panning/zooming (v3)

Updated to use getScrollXY

Comment 14

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

Comment on attachment 645284 [details] [diff] [review]
Make handles move with panning/zooming (v3)

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

::: mobile/android/base/TextSelectionHandle.java
@@ +112,5 @@
> +        ImmutableViewportMetrics metrics = layerController.getViewportMetrics();
> +        repositionWithView(metrics.viewportRectLeft, metrics.viewportRectTop, metrics.zoomFactor);
> +    }
> +
> +    void repositionWithView(float x, float y, float zoom) {

naming nit, "repositionWithView" makes me thing there should be a view argument, but there isn't

Comment 15

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

Landed with that function renamed to repositionWithViewport which might be a little less confusing, at least the arguments describe a viewport.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0e4574d8d0

Comment 16

[:Margaret Leibovic]

If you don't think this is too risky, this would be pretty cool to uplift for when we ship text selection for the first time in 15.

Comment 17

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

Yeah I'll request uplift once it's in m-c.

Comment 18

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/2b0e4574d8d0

Comment 19

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

Comment on attachment 645284 [details] [diff] [review]
Make handles move with panning/zooming (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 774938 (native text selection handles)
User impact if declined: the native handles lag during pan/zoom operations
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): mobile only. small risk of the handles glitching while moving them or panning, but it shouldn't break anything else.
String or UUID changes made by this patch: none

Comment 20

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/c7d8a9e01250
https://hg.mozilla.org/releases/mozilla-beta/rev/7401bb6fa4e5

The beta patch didn't apply cleanly because of missing bug 776547, but it was just a change in the context of the patch so applying it manually was trivial.

Comment 21

[Adrian Tamas (:AdrianT)]

The handles still lag a bit but there is a really big improvement on the behavior. Marking as verified fix.
Tested on:
Firefox Mobile 15.0b3/Nightly 17.0a1 2012-08-02/Aurora 16.0a2 2012-08-02
HTC Desire(Android 2.2.2)/ HTC Desire (Android 2.3.3)