1030221 - overscroll gets easily stuck in Line app

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Steps to Reproduce:

1) Start a conversation in the Line app
2) Send a message (easiest to reproduce if you are the last person to send a message in the conversation).
3) Scroll up in conversation.
4) Scroll down to bottom such that overscroll is triggered.

Expected:

Overscroll animation runs as normal.

Actual:

Maybe 80% of the time I get stuck in the overscroll animation.

This could be related to bug 1027309, but that bug requires two fingers while this bug can be triggered with a single finger.

Flame
v121-2 base image
v2.0 pvt update channel
Build ID: 20140625000201
Gaia Rev: de77f794

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

You may need to actually send a message and then try the scroll immediately after.  Perhaps its getting confused about the scroll position or something when the new message is added to the list.

Comment 2

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

I was able to reproduce this on a Flame and it looks like the problem is that when a fling animation is cancelled, overscroll is not cleared. This can happen if, for example, you fling and while you're in the overscroll state (still in the fling animation, not in the snapback part yet), you get a ScrollTo call and cancel the animation. Patching coming shortly.

Comment 4

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

Attachment: Part 1 - Move Sample functions into class definitions

Seems kinda silly/inconsistent to have the Sample function implementations so far away. This one is a non-functional change, I just moved the code and adjusted indentation to match.

Comment 5

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

Attachment: Part 2 (fix) - Clear overscroll when cancelling fling

I tried adding a gtest for this but overscroll is actually preffed off by default in gfxPrefs, and only preffed on in B2G. I don't know if there's a way to fiddle with prefs in the middle of a test but I couldn't find an easy way to do it. I'll file a follow-up bug for this.

Comment 6

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

Filed bug 1031024 for the gtests thing.

Comment 7

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8446808 [details] [diff] [review]
Part 1 - Move Sample functions into class definitions

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

I'm kind of lukewarm to this since it sucks to have really long functions inside class declarations, but since they don't do much else, I think it's ok in this case.

Comment 8

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8446810 [details] [diff] [review]
Part 2 (fix) - Clear overscroll when cancelling fling

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +563,5 @@
> +    // state. We deal with this by explicitly clearing the overscroll when
> +    // cancelling a fling.
> +    mApzc.mX.ClearOverscroll();
> +    mApzc.mY.ClearOverscroll();
> +  }

What do you think of making FlingAnimation inherit from OverscrollSnapBackAnimation instead?

Comment 9

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

(In reply to Doug Sherk (:drs) from comment #8)
> 
> What do you think of making FlingAnimation inherit from
> OverscrollSnapBackAnimation instead?

Sounds like a good idea, although I'd want a different name for it.

Comment 10

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

Attachment: Part 2 (fix) - Clear overscroll when cancelling fling (v2)

Now with an OverscrollableAnimation base class as discussed on IRC.

Comment 11

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8446821 [details] [diff] [review]
Part 2 (fix) - Clear overscroll when cancelling fling (v2)

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

Thanks, this is a lot cleaner.

Comment 12

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

Thanks for the quick turnaround :)

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/98771cfac871
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/2b7a95f55ad0

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98771cfac871
https://hg.mozilla.org/mozilla-central/rev/2b7a95f55ad0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/54208d905243
https://hg.mozilla.org/releases/mozilla-aurora/rev/1e4a2e728730

Comment 15

[Lance（leave from Mozilla）]

This issue has been verified successfully on Flame 2.0, 2.1.
See attachment: 1424.MP4
Reproducing rate: 0/5

Step:
1.Launch Market.
2.Search Line app to install.
3.Launch Line, log in account.
4.Select a friend to chat.
5.Send a message (easiest to reproduce if you are the last person to send a message in the conversation).
6.Scroll up in conversation.
7.Scroll down to bottom such that overscroll is triggered.

Actual result:
Overscroll animation runs as normal.

Flame 2.0 version:
Gaia-Rev        856863962362030174bae4e03d59c3ebbc182473
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-2g32_v2_0/rev/e40fe21e37f1
Build-ID        20141208000206
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141208.035628
FW-Date         Mon Dec  8 03:56:38 EST 2014
Bootloader      L1TC00011880

Flame 2.1 version:
Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141205.035305
FW-Date         Fri Dec  5 03:53:16 EST 2014
Bootloader      L1TC00011880

Comment 16

[Lance（leave from Mozilla）]

Attachment: 1424.MP4