827715 - AsyncPanZoomController gestures are processed unreliably (every other gesture?)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) Load nytimes.com, wait to finish
 (2) Try to fling down the page
 (3) Try to fling down the page again

You'll most likely see either (2) succeed and (3) fail, or (2) fail and (3) succeed.

It seems like the gesture detection in APZC is being messed up every other gesture.

This takes perceived browser performance to crap, and as a regression should block.

Shih-Chiang do you have cycles to take a look?

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@cjones, sure, I'll take a look at this bug.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: WIP - process all queued touch transactions after content process finishing event handler.

APZC delays gesture detection because www.nytimes.com has touch event listener. If we have more than one touch transaction in queue, APZC will only process the first touch transaction in queue. Therefore, you'll one see one gesture been triggered.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Shih-Chiang Chien [:schien] from comment #2)
> only process the first touch transaction in queue. Therefore, you'll one see
s/one see/only see

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 699737 [details] [diff] [review]
WIP - process all queued touch transactions after content process finishing event handler.

This seems right to me.  I reviewed the original patch but I don't remember this check specifically.

drs, do you recall why this is here?

Comment 5

[Doug Sherk (:drs) (inactive)]

Comment on attachment 699737 [details] [diff] [review]
WIP - process all queued touch transactions after content process finishing event handler.

The idea behind this code block was that we should require a preventDefault or no-preventDefault for each block of touch events from start->n*move->end/cancel. Removing this block will make us process all queued up touch events.

That means that if we touch the screen 3 independent times (i.e. I put my finger down, do some arbitrary motion, then take my finger off) before content manages to handle one of them, the first one to handle it will cause the following ones to be processed at the same time.

We're already not following the W3 spec for this, but here's the important part of what it has to say about this (http://www.w3.org/TR/touch-events/#list-of-touchevent-types):
> If the preventDefault method is called on this event, it should prevent any 
> default actions caused by any touch events associated with the same active 
> touch point, including mouse events or scrolling. 

My interpretation of this is that we should be associating each preventDefault/no-preventDefault to the series of touch events that it was handling (start->n*move->end/cancel). Removing this code would break that.

The reason this isn't working is because, if touch events with different mIdentifier fields are interspersed, only the first touchend will get sent.

I'd rather fix this another way like batching touch events into queues based on the mIdentifier field rather than chronological ordering, but that seems like a lot of work and personally I'm okay with fixing it this way for now as long as we come back to it (i.e. file tech debt bugs).

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: process all queued touch transactions after content process finishing event handler, v1

add FIXME comment.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 700344 [details] [diff] [review]
process all queued touch transactions after content process finishing event handler, v1

This patch doesn't fix the issue for me :(.

I suspect this is related somehow to bug 828367 and async-pan-zoom detection.

Comment 8

[Doug Sherk (:drs) (inactive)]

(In reply to Shih-Chiang Chien [:schien] from comment #6)
> Created attachment 700344 [details] [diff] [review]
> process all queued touch transactions after content process finishing event
> handler, v1
> 
> add FIXME comment.

If this patch ever sees the light of day, I'd prefer it if (a) bug(s) were filed for this instead of just adding a FIXME.

Comment 9

[Andreas Gal :gal]

schien, do you want to keep working on this?

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Andreas Gal :gal from comment #9)
> schien, do you want to keep working on this?
I handed it over to :nrc yesterday, and I'll help him on this bug since I have bandwidth now. :)

Comment 11

[Nick Cameron [:nrc]]

I find this difficult to recreate with flings, but pretty easy with pinch-zooms. I don't think that the issue is with prematurely clearing the queue though, we don't ever hit the path changed in the patch when we miss the gestures. Investigating...

Comment 12

[Nick Cameron [:nrc]]

Attachment: part 1: fix pinch to zoom behaviour

We're not entirely sure this is safe behaviour, but it certainly fixes the dropped pinch-to-zoom gestures.

We still need to fix the fling issue.

Comment 13

[Nick Cameron [:nrc]]

Patch is joint work with schien, btw. Teamwork FTW!

Comment 14

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14d1d368bcd2

Comment 15

[Nick Cameron [:nrc]]

Could someone land this to b2g18 for me please? I seem to be having hg issues with that repo :-(

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8f2ef4998b60

(Mind the [leave open], please.)

Comment 17

[JP Rosevear [:jpr]]

Kats, have you been able to reproduce the fling issue?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14d1d368bcd2

Comment 19

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

(In reply to JP Rosevear [:jpr] from comment #17)
> Kats, have you been able to reproduce the fling issue?

No, I can't reproduce it either on my device (test-drivers device running the latest available build) or on a B2G Desktop gaia build.

Comment 20

[Nick Cameron [:nrc]]

Attachment: part 2: fix flings

The problem is that when the time between samples gets 'large', the velocity gets set to 0 too quickly because the way we calculate the new velocity from the friction is incorrect. I changed the way it is calculated and that fixes the bug. I adjusted the friction global to give apx the same speed of flinging.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Chris, if this gets an r+, could you land it as well?  Nick is in GMT, and we want to land this today.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66c886b7d927
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f5278c18083b

Comment 23

[Marco Bonardo [:mak]]

backed out of inbound due to build bustage
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c886b7d927
I don't have the b2g18 repo here to do the same there... please take care of that.

Comment 24

[Marco Bonardo [:mak]]

the failure on Windows
e:/builds/moz2_slave/m-in-w32/build/gfx/layers/ipc/Axis.cpp(165) : error C2666: 'pow' : 6 overloads have similar conversions

Comment 25

[Marco Bonardo [:mak]]

ehm sorry, I posted the wrong backout changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d94202f46627

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f5278c18083b

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

gecko18 relanding

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e9eca4f2ea0f

inbound coming up when it reopens.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37eb156ec02c

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/37eb156ec02c

Comment 30

[Doug Sherk (:drs) (inactive)]

Comment on attachment 701112 [details] [diff] [review]
part 1: fix pinch to zoom behaviour

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

Clearing from my review queue, also f+ FWIW at this point.

Comment 31

[Doug Sherk (:drs) (inactive)]

Comment on attachment 701844 [details] [diff] [review]
part 2: fix flings

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

Clearing from my review queue, also f+ FWIW at this point.