If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox 14

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: jpr, Assigned: kats)

Tracking

Trunk
Firefox 15
Points:
---

Firefox Tracking Flags

(firefox14 verified, firefox15 verified, blocking-fennec1.0 +)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
scripting.com, a long text page, stops scrolling correctly, so this problem is not universal.
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.
Depends on: 743736
In logcat, it looks like ListenerTimeoutProcessor is not dispatching our onTouchStart event quickly enough.
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.
(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?
blocking-fennec1.0: --- → ?
> (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?
(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.
Assignee: nobody → wjohnston
blocking-fennec1.0: ? → +
Assignee: wjohnston → bugmail.mozilla
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.
Created attachment 619699 [details] [diff] [review]
WIP (1/2) Set touch listener timeout to 200ms
Created attachment 619701 [details] [diff] [review]
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).
Created attachment 619994 [details] [diff] [review]
(1/3) Set timeout to 200ms
Attachment #619699 - Attachment is obsolete: true
Attachment #619994 - Flags: review?(wjohnston)
Created attachment 619995 [details] [diff] [review]
(2/3) Minor cleanup
Attachment #619995 - Flags: review?(wjohnston)
Created attachment 619996 [details] [diff] [review]
(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
Attachment #619701 - Attachment is obsolete: true
Attachment #619996 - Flags: review?(wjohnston)
Attachment #619994 - Flags: review?(wjohnston) → review+
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 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.
Attachment #619995 - Flags: review?(wjohnston) → review+
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?
Attachment #619996 - Flags: review?(wjohnston) → review+
(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?
Yep. Shoulda remembered that!
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
status-firefox14: --- → affected
status-firefox15: --- → fixed
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/f3f7531f6fa3
https://hg.mozilla.org/mozilla-central/rev/2bc37f6fcb44
https://hg.mozilla.org/mozilla-central/rev/2894e42bc18b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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
Attachment #619994 - Flags: approval-mozilla-aurora?
Comment on attachment 619995 [details] [diff] [review]
(2/3) Minor cleanup

[Approval Request Comment]
Same as above
Attachment #619995 - Flags: approval-mozilla-aurora?
Comment on attachment 619996 [details] [diff] [review]
(3/3) Notify PZC of some events to remain responsive

[Approval Request Comment]
Same as above
Attachment #619996 - Flags: approval-mozilla-aurora?
Attachment #619994 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #619995 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #619996 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
status-firefox14: affected → fixed
Verified fix on:
Nightly 15.0a1 (2012-05-21)/ Aurora 14.0a2 (2012-05-21)
Device: HTC Desire
OS: Android 2.2.2
Status: RESOLVED → VERIFIED
Verified fixed on:
Aurora 14.0a2 (2012-05-24)
Beta 14.0b3
Samsung Galaxy SII (2.3.4)
status-firefox14: fixed → verified
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.