749384 - NY Times slow to respond to touch to stop a scroll/fling

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[JP Rosevear [:jpr]]

NY Times slow to respond to touch to stop a scroll/fling.  Scroll even pretty slowly, especially zoomed in, and let the page's momentum carry it forward. Touch your finger down.  Takes the page a long time to respond to the finger down and stop the fling.

Comment 1

[Chris Peterson [:cpeterson]]

scripting.com, a long text page, stops scrolling correctly, so this problem is not universal.

Comment 2

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

From a quick look it seems like the touch event handlers on nytimes.com are still taking a while, preventing us from processing the touch down event that stops the fling.

Comment 3

[Chris Peterson [:cpeterson]]

In logcat, it looks like ListenerTimeoutProcessor is not dispatching our onTouchStart event quickly enough.

Comment 4

[Wesley Johnston (:wesj)]

touchdown events will be the ones that take us the longest. We could ignore calls to preventDefault while pans are occurring. Or we could treat them normally, but just force panning to stop regardless.

Comment 5

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

(In reply to Chris Peterson (:cpeterson) from comment #3)
> In logcat, it looks like ListenerTimeoutProcessor is not dispatching our
> onTouchStart event quickly enough.

If you're looking into this, a few things to check:
(1) what is ViewConfiguration.getLongPressTimeout() set to?
(2) How long does it actually take between the listener timeout processor is queued to when it executes?
(3) If (2) is greater than (1), are there lots of other events flooding the UI queue that could be causing the delay?

Comment 6

[Chris Peterson [:cpeterson]]

> (1) what is ViewConfiguration.getLongPressTimeout() set to?

ViewConfiguration.getLongPressTimeout() is 500 ms.


> (2) How long does it actually take between the listener timeout processor is queued to 
> when it executes?

ListenerTimeoutProcessor calls stopAnimationTimer() 500 ms after being posted.

That is about how long the page seems to keep scrolling after "hitting the brakes" with touch-down. If I change EVENT_LISTENER_TIMEOUT to 1 ms, then the page stop scrolling immediately on touch-down.


> (3) If (2) is greater than (1), are there lots of other events flooding the UI queue that 
> could be causing the delay?

Where would I check for UI event flooding?

Comment 7

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

(In reply to Chris Peterson (:cpeterson) from comment #6)
> ViewConfiguration.getLongPressTimeout() is 500 ms.

This seems high. We should probably just hard-code the value to 200ms.

> ListenerTimeoutProcessor calls stopAnimationTimer() 500 ms after being
> posted.

Ok, so the nytimes touch event handling is definitely taking too long. If it's the ListenerTimeoutProcessor that triggers the stopAnimationTimer() then that means the event listeners are still not done after 500ms of running. We should probably find out why they're taking so long (and/or try to find out if they take a comparable amount of time in other browsers).

> 
> Where would I check for UI event flooding?

Not sure offhand. I'd probably start with ddms and traceview to see if they provide useful information. But it shouldn't be necessary now; it doesn't seem like UI event flooding is an issue in this case.

Comment 8

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

Ok, so upon further investigation it's not actually the nytimes touch event listener that's taking too long. It's actually kind of a specification issue.

When we touch down to stop the fling, we end up holding that touch-down in a queue until we know whether or not it has been default-prevented. And we don't find that out until the first touch-move event is processed (because a block of touch events can be default-prevented on either the down event or the first move event following it). If the user just puts their finger down to stop a fling but doesn't move their finger, then we don't know if that down has been default-prevented or not, and so end up waiting the 500ms for the timeout to expire. I can change the timeout to 200ms but it's still noticeable.

Ideally what I'd like to do is when the user puts their finger down, abort any animations regardless of whether or not that down gets default-prevented. This basically redefines "stopping in-progress flings" from being the "default action" of the down event to not being the "default action" of the down event, in order to stay in compliance with the touch events spec. I'll cook up a patch for this.

Comment 9

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

Attachment: WIP (1/2) Set touch listener timeout to 200ms

Comment 10

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

Attachment: WIP (2/2) Add ability to stop animations on touchdown regardless of preventdefault

Although I think this does the job, it's not very clean and I'd like to come up with a better solution (and test it further).

Comment 11

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

Attachment: (1/3) Set timeout to 200ms

Comment 12

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

Attachment: (2/3) Minor cleanup

Comment 13

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

Attachment: (3/3) Notify PZC of some events to remain responsive

I have a test page at http://people.mozilla.org/~kgupta/bug/749384.html which might be useful. I used it to test general panning behaviour and the specific scenarios modified by this patch:

1) Flinging and stopping the fling (the nytimes case)
2) Flinging into overscroll and stopping the fling by putting your finger down into a default-prevented area
3) After (2), moving your finger around and verifying the behaviour is sane (events go to listeners, no panning happens)
4) After (2) or (3), lifting your finger and ensuring the bounce-back happens properly

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 619995 [details] [diff] [review]
(2/3) Minor cleanup

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

Sorry for the delay here. I think I needed the longer timeout in the original code because we only had one set of timers. Just wanted to think this through and make sure it was ok, and then I somehow ran off on vacation.

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 619995 [details] [diff] [review]
(2/3) Minor cleanup

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

Sorry for the delay here. I think I needed the longer timeout in the original code because we only had one set of timers. Just wanted to think this through and make sure it was ok, and then I somehow ran off on vacation.

Comment 16

[Wesley Johnston (:wesj)]

Comment on attachment 619996 [details] [diff] [review]
(3/3) Notify PZC of some events to remain responsive

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

Just a question below. If that's good, then i'm ok with this.

::: mobile/android/base/gfx/TouchEventHandler.java
@@ +183,5 @@
>              mEventQueue.add(MotionEvent.obtain(event));
>          } else if (mDispatchEvents) {
>              dispatchEvent(event);
> +        } else if (touchFinished(event)) {
> +            mPanZoomController.preventedTouchFinished();

A bit worried this will end up calling bounce when we have multiple fingers on the screen? Maybe we need a check for the processingBalance in touchFinished?

@@ +270,5 @@
>              // default-prevented.
>              if (allowDefaultAction) {
>                  dispatchEvent(event);
> +            } else if (touchFinished(event)) {
> +                mPanZoomController.preventedTouchFinished();

Same here?

Comment 17

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

(In reply to Wesley Johnston (:wesj) from comment #16)
> A bit worried this will end up calling bounce when we have multiple fingers
> on the screen? Maybe we need a check for the processingBalance in
> touchFinished?
> 

touchFinished checks for ACTION_UP, which should only get fired when the last finger is lifted. If there are multiple fingers on the screen and one of those is lifted we should get an ACTION_POINTER_UP event, so we shouldn't be sending the preventedTouchFinished at that point. Does that answer your question?

Comment 18

[Wesley Johnston (:wesj)]

Yep. Shoulda remembered that!

Comment 19

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

heh yeah :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f7531f6fa3
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc37f6fcb44
https://hg.mozilla.org/integration/mozilla-inbound/rev/2894e42bc18b

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f3f7531f6fa3
https://hg.mozilla.org/mozilla-central/rev/2bc37f6fcb44
https://hg.mozilla.org/mozilla-central/rev/2894e42bc18b

Comment 21

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

Comment on attachment 619994 [details] [diff] [review]
(1/3) Set timeout to 200ms

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: attempting to stop a fling by putting a finger down on a page with touch event listeners is very laggy
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): mobile-only
String or UUID changes made by this patch: none

Comment 22

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

Comment on attachment 619995 [details] [diff] [review]
(2/3) Minor cleanup

[Approval Request Comment]
Same as above

Comment 23

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

Comment on attachment 619996 [details] [diff] [review]
(3/3) Notify PZC of some events to remain responsive

[Approval Request Comment]
Same as above

Comment 24

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ae8038733dce
https://hg.mozilla.org/releases/mozilla-aurora/rev/c3339c6f1ab8
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc28e94bedd6

Comment 25

[Adrian Tamas (:AdrianT)]

Verified fix on:
Nightly 15.0a1 (2012-05-21)/ Aurora 14.0a2 (2012-05-21)
Device: HTC Desire
OS: Android 2.2.2

Comment 26

[Catalin Suciu [:csuciu]]

Verified fixed on:
Aurora 14.0a2 (2012-05-24)
Beta 14.0b3
Samsung Galaxy SII (2.3.4)