718463 - Tap during animated zoom leaves the page blurry

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Martijn Wargers (dead)]

Steps to reproduce:
- Triple tap on some part of the page

Expected result:
- The page should zoom in as like with a double tap

Actual result:
- After the third tap, the zoom action stops, and the page is stuck in a blurry state.

Comment 1

[Johnathan Nightingale [:johnath]]

This needs testing post-maple landing to see if it's still an issue

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

So the issue is, we get a ActionDown in state Fling, and I we kill off the animation in mid run. Somewhere we also don't bother to update our viewportmetrics I think, which makes us blurry... 

This adds an interruptable property to the animation to specify whether or not it can be stopped. It also adds checks for flings and animations in onSingleTapConfirmed and onDoubleTap to prevent accidental clicks while we're in mid animation.

kats, you or pcwalton may have a better idea for how to handle this? I'd ask for just feedback, but if you like this approach, feel free to just review it.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Helpful stuff

Just putting this here to store it somewhere. This slows down our animations to make it easier to play with them.

Comment 4

[Wesley Johnston (:wesj)]

OK, I think we're going to freeze the zoom when this happens, but we still need to not be blurry. Renaming this bug so that I don't confuse myself again.

Comment 5

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

Comment on attachment 601352 [details] [diff] [review]
Patch

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

Minusing this patch to get it out of my queue, then.

Comment 6

[Martijn Wargers (dead)]

I'll retest after Maple lands.

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I did a pinch zoom and before ending the zoom, I panned and still see blurriness. 

This is with tinderbox build: http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-central-android/1331738369/

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Triple tapping does stop the animated zoom on the third tap still with the latest nightly  ( 3/20/2012) w/ Galaxy Nexus.

Pinch zoom and pan before ending with Galaxy Nexus w/ latest nightly 3/20/2012 does not seem to cause blurriness any more.

Will test with some other devices.

Comment 9

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Tested with HD Desire and Samsung Galaxy S II.  Same result as Comment 8 occurs.  tapping a third time during an animated zoom will cause the zoom to stop and show blurriness still in the nightly 3/20/2012.

Comment 10

[JP Rosevear [:jpr]]

I cannot replicate the pan/zoom or triple click issue on Galaxy S or Galaxy Nexus.

Comment 11

[Chris Lord [:cwiiis]]

I can duplicate this by tapping during a double-tap zoom out on the HTC Flyer. If I pan, the page adjusts correctly, but if I just tap to interrupt the animation, I'm left with a blurry page.

Comment 12

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

Attachment: Patch

This one is a pretty straightforward fix

Comment 13

[Chris Lord [:cwiiis]]

Comment on attachment 614063 [details] [diff] [review]
Patch

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

Leaving as r? pending explanation of the below.

::: mobile/android/base/ui/PanZoomController.java
@@ +273,5 @@
> +            // disabled by getRedrawHint(), so force a redraw in case
> +            // this touchstart is just a tap that doesn't end up triggering
> +            // a redraw
> +            mController.setForceRedraw();
> +            mController.notifyLayerClientOfGeometryChange();

Why don't we do this for ANIMATED_ZOOM? Surely this isn't going to fix the issue?

Comment 14

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

Turns out we're pretty much never in the ANIMATED_ZOOM state, since the animateZoomTo(Rect) code calls bounce(viewport) to do the actual animation. If you prefer I can try to fix that so that we actually remain in the ANIMATED_ZOOM state when we're doing an animated zoom... but that sounds like crazy talk! :p

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 614063 [details] [diff] [review]
Patch

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

Possibly nice to add a comment under the ANIMATED_ZOOM case (and even nicer to either remove that state entirely, or make it do as it says), but r+ in that case :)

::: mobile/android/base/ui/PanZoomController.java
@@ +273,5 @@
> +            // disabled by getRedrawHint(), so force a redraw in case
> +            // this touchstart is just a tap that doesn't end up triggering
> +            // a redraw
> +            mController.setForceRedraw();
> +            mController.notifyLayerClientOfGeometryChange();

Why don't we do this for ANIMATED_ZOOM? Surely this isn't going to fix the issue?

Comment 16

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

Attachment: Patch v2

I decided it was worth the effort to fix properly. This makes sure we remain in the ANIMATED_ZOOM state during the animation. I verified that our handling for all incoming touch events during this state is sane. The only 'odd' case is if you double-tap and leave your finger down after the second tap (and then possibly drag around before lifting). In that case we do the zoom-to-rect as expected, and ignore the drag.

Comment 17

[Chris Lord [:cwiiis]]

Comment on attachment 614117 [details] [diff] [review]
Patch v2

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

Looks good to me, though I wouldn't be surprised at something subtly breaking... (not a comment on the change of course, so much as this being a delicate bit of code)

r+ with the comment sorted.

::: mobile/android/base/ui/PanZoomController.java
@@ +488,3 @@
>          // set the animation target *after* setting state BOUNCE, so that
>          // the getRedrawHint() is returning false and we don't clobber the display
>          // port we set as a result of this animation target call.

This comment needs amending.

Comment 18

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

Good catch, fixed it on landing. And yeah, I agree this code is pretty delicate. *crosses fingers*

https://hg.mozilla.org/integration/mozilla-inbound/rev/66e7ce7034c7

I need to buy you multiple $BEVERAGEs for dealing with all my bugspam and reviews so quickly :)

Comment 19

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #18)
> Good catch, fixed it on landing. And yeah, I agree this code is pretty
> delicate. *crosses fingers*
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/66e7ce7034c7
> 
> I need to buy you multiple $BEVERAGEs for dealing with all my bugspam and
> reviews so quickly :)

Well, I could say the same for you doing all this work so quickly, but I never turn down a $BEVERAGE :)

Comment 20

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/66e7ce7034c7

Comment 21

[Cristian Nicolae (:xti)]

This issue is not reproducible anymore on the latest Nightly by performing the str from comment #0. 
Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-23)
Device: Galaxy Nexus
OS: Android 4.0.2