Closed Bug 946661 Opened 6 years ago Closed 6 years ago

Taps > 300ms are ignored if APZC content disables zooming

Categories

(Core :: DOM: Events, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed

People

(Reporter: daleharvey, Assigned: daleharvey)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

No description provided.
Assignee: nobody → dale
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
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.
Attachment #8344578 - Flags: review?(bugmail.mozilla)
Attached patch Tests!Splinter Review
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.
Attachment #8344649 - Flags: review?(dale)
Also the tests require the patch from bug 947931 to work.
Depends on: 947931
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.
Attachment #8344578 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8344649 - Flags: review?(dale) → review+
Added the comment and rebased on top of the patch you created for the tests
Attachment #8344578 - Attachment is obsolete: true
Attachment #8344874 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/85d76f521dfe
https://hg.mozilla.org/mozilla-central/rev/a1cabe789fec
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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.
blocking-b2g: --- → 1.3?
Keywords: regression
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
Attachment #8344649 - Flags: approval-mozilla-aurora?
Attachment #8344649 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8344874 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.