946661 - Taps > 300ms are ignored if APZC content disables zooming

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Comment 1

[Dale Harvey (:daleharvey)]

To clarify this bug, https://bugzilla.mozilla.org/show_bug.cgi?id=922896 regressed https://bugzilla.mozilla.org/show_bug.cgi?id=915645, any press over 300ms is ignored as it triggers a tapconfirmed directly which is ignored on zoomable content

Comment 2

[Dale Harvey (:daleharvey)]

Attachment: Dont send tapconfirmed event when tapup was handled

This is half the fix that was in https://bugzilla.mozilla.org/show_bug.cgi?id=942929, split out into its own patch and avoiding extra state variables.

Now we only run double tap timeout if the controller has told us it ignored the tapup, so we wont get tapconfirmed events following a tapup that has been handled which means the controller can handle tapconfirmed unconditionally.

Comment 3

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

Attachment: Tests!

This adds tests to ensure we don't regress this again. The MediumPress test fails on current m-c and passes with your patch. Please try to add similar tests for additional changes to this code as it tends to be quite brittle.

Comment 4

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

Also the tests require the patch from bug 947931 to work.

Comment 5

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

Comment on attachment 8344578 [details] [diff] [review]
Dont send tapconfirmed event when tapup was handled

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

This seems reasonable to me. Please also add a comment in APZC::OnSingleTapUp next to the "&& !mAllowZoom" condition that says if mAllowZoom is true we wait for a call to OnSingleTapConfirmed before dispatching to content.

Comment 6

[Dale Harvey (:daleharvey)]

Comment on attachment 8344649 [details] [diff] [review]
Tests!

Thanks a lot, I had made a start on the tests however I was doing https://bugzilla.mozilla.org/show_bug.cgi?id=942929 at the same time and attempting to test both, also the test framework is fairly new to me but much easier to understand seeing tests that go along with the patch.

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: Dont send tapconfirmed event when tapup was handled

Added the comment and rebased on top of the patch you created for the tests

Comment 8

[Dale Harvey (:daleharvey)]

Pushed the 3 patches to try: https://tbpl.mozilla.org/?tree=Try&rev=e0905361a062

Comment 9

[Dale Harvey (:daleharvey)]

https://hg.mozilla.org/integration/b2g-inbound/rev/85d76f521dfe
https://hg.mozilla.org/integration/b2g-inbound/rev/a1cabe789fec

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85d76f521dfe
https://hg.mozilla.org/mozilla-central/rev/a1cabe789fec

Comment 11

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

Based on comment 0 this is a regression in Gecko 27. I don't think we need to uplift to the beta branch but we should uplift to aurora and get it in B2G 1.3 at least.

Comment 12

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

Comment on attachment 8344649 [details] [diff] [review]
Tests!

(This request is for both patches)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 922896
User impact if declined: doing a tap where your finger is down for 300-500ms is not treated as a tap.
Testing completed (on m-c, etc.): on m-c, in gtest
Risk to taking this patch (and alternatives if risky): pretty low risk. it's in code that is used on B2G and metro but nowhere else.
String or IDL/UUID changes made by this patch: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f429fc42cbc3
https://hg.mozilla.org/releases/mozilla-aurora/rev/232017120b3f