870055 - Browser continues to scroll after swipe until finger lifted

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

On a fresh master sync'd today (5/8/2013) I see strange scrolling behavior in the B2G browser:

 1) When I swipe up or down to scroll, the window keeps scrolling until I lift my finger.
 2) Sometimes the scrolling goes into a continuous jumping cycle when I swipe near the top or bottom of the page.

An example can be seen in the following video.  (Apologies for the poor quality and orientation.)

  https://www.dropbox.com/s/xa4m78f6k4mz983/20130508_b2g_master_scroll_issue.MOV

This is on a Buri device.

Gaia Git revision:  3f766bccdd9c0961b33f2085089c1f575143bd8b

Comment 1

[Ben Francis [:benfrancis]]

This shouldn't be anything to do with your Gaia version, async pan and zoom is handled in Gecko. What gecko build do you have?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Gecko was a fresh mozilla-central.

hg changeset:  131199:b980d32c366f

But I first saw this last week, so its been around for at least that long.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I can't reproduce this on the emulator.  Not sure if that's expected or not given the weirdness of trying to swipe with a mouse.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I also cannot reproduce on an unagi.  It looks like this may be specific to the hamachi platform.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Setting back to general category for now until I can figure out why component is the cause.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I flashed my phone with the following combination of software:

  b2g-manifest: master
  b2g-gaia:     master
  gecko:        b2g18

And the unusual scroll behavior did not occur.  So it does appear to be a gecko issue.

It seems odd to me that it only appears on the buri device and not the unagi, however.

Comment 7

[Chris Lord [:cwiiis]]

I'm not sure what device the 'buri' is, but I see this on a Geeksphone Peak and a Geeksphone Keon building master of everything. Very likely an AsyncPanZoomController bug.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I just flashed my buri with the new firmware from 5/8 and this problem persists.  Since the firmware had new touch drivers there was some hope it might resolve the issue.

Also, I've done some bisecting and the problem is present as far back as 127775:842c175f344c in mozilla-central.  Unfortunately, somewhere around 127753:051cf1c1449c my b2g process stops booting properly, so I've had issues going much further back.

Comment 9

[Chris Lord [:cwiiis]]

Attachment: Fix continued movement when finger held on screen

This fixes the issue. I guess this may have been introduced to help a device with bad touch events, but the logic is faulty - this should be dealt with some other way.

Comment 10

[Chris Lord [:cwiiis]]

I'm removing the 'hamachi' keyword, this isn't specific to any platform.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Thanks Chris!  As expected, attachment 750922 [details] [diff] [review] works for my Buri as well.

If you don't mind, I'll throw the assignment over to you.

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 750922 [details] [diff] [review]
Fix continued movement when finger held on screen

Handing review over to kats.

Further comment, I think the mPos != aPos check just needs to be gotten rid of altogether and newVelocity should be calculated over time rather than on a touch-move instance. (the second part of that can be dealt with in another bug)

Comment 13

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

Comment on attachment 750922 [details] [diff] [review]
Fix continued movement when finger held on screen

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

This looks ok to me. Make sure you fix s/Asyn/Async/ in the commit message before landing.

Comment 14

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

Also CC'ing :tatiana and :drs since they added/reviewed this code in bug 824511, and might have objections.

Comment 15

[Oleg Romashin (:romaxa)]

Comment on attachment 750922 [details] [diff] [review]
Fix continued movement when finger held on screen


> void Axis::UpdateWithTouchAtDevicePoint(int32_t aPos, const TimeDuration& aTimeDelta) {
>-  if (mPos == aPos) {
>-    // Does not make sense to calculate velocity when distance is 0
>-    return;
>-  }

You should also prevent adding 0 velocity to the queue
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/Axis.cpp#124

Comment 16

[Oleg Romashin (:romaxa)]

> Further comment, I think the mPos != aPos check just needs to be gotten rid
> of altogether and newVelocity should be calculated over time rather than on
> a touch-move instance.
I was proposing same thing someday... but drs told me that we need to handle circle gesture kinetic motion... not sure why

Comment 17

[Chris Lord [:cwiiis]]

(In reply to Oleg Romashin (:romaxa) from comment #15)
> Comment on attachment 750922 [details] [diff] [review]
> Fix continued movement when finger held on screen
> 
> 
> > void Axis::UpdateWithTouchAtDevicePoint(int32_t aPos, const TimeDuration& aTimeDelta) {
> >-  if (mPos == aPos) {
> >-    // Does not make sense to calculate velocity when distance is 0
> >-    return;
> >-  }
> 
> You should also prevent adding 0 velocity to the queue
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/Axis.cpp#124

You shouldn't, otherwise the view will always move when your finger stops, even if your finger has been still for a long time.

Comment 18

[Chris Lord [:cwiiis]]

(In reply to Oleg Romashin (:romaxa) from comment #16)
> > Further comment, I think the mPos != aPos check just needs to be gotten rid
> > of altogether and newVelocity should be calculated over time rather than on
> > a touch-move instance.
> I was proposing same thing someday... but drs told me that we need to handle
> circle gesture kinetic motion... not sure why

I don't see how these things are mutually exclusive?

Comment 19

[Chris Lord [:cwiiis]]

Pushed to inbound with removal of mPos == aPos check confirmed on IRC:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2313616e4203

If we need to do something like this (which we probably do, we certainly do so in JavaPanZoomController), we need to do it in a way that doesn't break common use-cases (like wanting to stop scrolling).

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2313616e4203