Description: Please check the attached video. When scroll screen and stop at one point, then lift up finger, sometimes page will start moving. This is really annoying. Steps to Reproduce: 1. Scroll on the screen and stop at one point. 2. Lift up finger 3. Stopped page will start moving again. Expected Results: When finger stops at one point and lifts up, page will stay still. Actual Results: Page moves after lift up finger. Environmental Variables: Version: 1.2 Device: Buri Build Date: 12/12/2013 RIL Type (Mozilla RIL or Commericial RIL): Commercial RIL

Some discussion with Markus and Botond indicated that this might be caused because we still have velocities in the velocity queue on touch-up, and so that velocity might get turned into a fling. We should do something better with killing velocities in the velocity queue after time has elapsed, or weighting the more recent velocities higher (assuming we also generate a velocity from the touchend coordinates).

I was trying to reproduce this on my Hamachi and Peak devices running master, since the Gordon from the UX team says this is still an issue, but I'm not able to. It's quite possible that the issue described in comment 2 is causing this problem on some devices that dispatch touch events less frequently. It might even happen on these devices with a different base image or something, not sure. One way to see if that's the problem is reduce the apz.max_velocity_queue_size pref to 1 or 2 (currently it defaults to 5) and see if it still happens. If it goes away then that's likely the problem. Gordon, do you mind trying that and seeing if it fixes the problem for you?

This also happens very frequently on Flame devices.

I don't have a Flame device so I can't test there, unfortunately. Can you please change the pref and see if it still happens? Otherwise this bug is basically unactionable for me.

I can repro this on Flame, so I'll dig into it.

Some refactoring. There should be no functional change as a result of this. The only fishy part is the sticky axis-lock code in TrackTouch which now gets executed on duplicate events. However in that case running through that code a second time should be a no-op because it will do exactly the same thing as the first time through.

This seems to work well on Flame, but the pref value may need to be adjusted for other devices. I'll test on Helix and Hamachi as well before requesting review.

Comment on attachment 8428873 [details] [diff] [review] Part 2 - Drop velocities that are old Review of attachment 8428873 [details] [diff] [review]: ----------------------------------------------------------------- This value works fine on both Hamachi and Flame. Helix build is still going but I assume it'll be fine there too.

Comment on attachment 8428872 [details] [diff] [review] Part 1 - Remove APZC::mLastEventTime, pass timestamps to Axis Review of attachment 8428872 [details] [diff] [review]: ----------------------------------------------------------------- This looks like a step backwards in terms of using strongly-typed units (TimeStamp and TimeDuration) to work with time. However, since I can't find a way to construct a TimeStamp from a uint32_t (I asked about that on ask.m.o btw [1]), I guess we have to live with that. In lieu of using TimeStamps, let's at least document that Axis::mPosTime and the aTimestamp argument to Axis::UpdateWith...() are in milliseconds (either with a comment, or by suffixing the variable with "Ms"). (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7) > The only fishy part is the sticky axis-lock code in TrackTouch which now > gets executed on duplicate events. However in that case running through that > code a second time should be a no-op because it will do exactly the same > thing as the first time through. We're also calling CallDispatchScroll() for duplicate events, which, while also a no-op, seems like unnecessary work. Perhaps we should just guard calling that with 'prevTouchPoint != touchPoint'? [1] https://ask.mozilla.org/question/685/converting-a-uint32_t-timestamp-to-a-mozillatimestamp/

Comment on attachment 8428873 [details] [diff] [review] Part 2 - Drop velocities that are old Review of attachment 8428873 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/src/AsyncPanZoomController.cpp @@ +235,5 @@ > * "apz.velocity_bias" > * How much to adjust the displayport in the direction of scrolling. This value > * is multiplied by the velocity and added to the displayport offset. > * > + * "apz.velocity_relevance_time" In gfxPrefs the pref name has an "_ms" suffix. ::: gfx/layers/apz/src/Axis.h @@ +57,5 @@ > /** > * Notify this Axis that a touch has ended gracefully. This may perform > * recalculations of the axis velocity. > */ > + void EndTouch(uint32_t aTimestamp); Please document that aTimestamp is in milliseconds. @@ +193,5 @@ > int32_t mStartPos; > float mVelocity; > bool mAxisLocked; // Whether movement on this axis is locked. > AsyncPanZoomController* mAsyncPanZoomController; > + nsTArray<std::pair<uint32_t, float> > mVelocityQueue; Please say that these are (timestamp-in-milliseconds, velocity) pairs.

Stephany, is this something UX would want uplifted to 1.4?

Yes: If you have a fix and we can have it for 1.4, UX would love that. Thanks, Milan!

Addressed comments and landed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a624b81b68f6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e5651c1ad7e

Been around in past releases. Given where we are with 1.4 not blocking on it

https://hg.mozilla.org/mozilla-central/rev/a624b81b68f6 https://hg.mozilla.org/mozilla-central/rev/5e5651c1ad7e

Hi Preeti, Is there a challenge in adding it to 1.4? The issue happens in past and that release too, so at least 1.4 would be worth it. I would like to get this user facing annoyance away if we can.

Milan Would you be able to prioritize this issue for 1.4?

Milan Please help with risk analysis here.

Other than the fact that it is late in the release cycle, this is a fairly low risk patch.

Backlog for now.