915645 - [Buri][TOR][Browser] Semi long press on links does nothing

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect)

Description

[Joe Cheng [:jcheng] (please needinfo)]

this has to do with how a user does tapping

it seems like you have to tap pretty quickly on the link in order for the page to open.

if you long press the link, you will see "open link in new tab"

so the problem is if you tap is a bit slowly but not slow enough to see "open link in the new tab", the link gets highlighted and does nothing

this is a pretty bad user experience (if a user happen to tap slowly as habit).

our partner had reported links not working several times but never figured out what's wrong as it's not always reproducible. After I have seen her taping the phone, I have came up with my observation as what i described.

can we adjust the tapping threshold? Thanks

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

this is v1.1 btw. on partner build

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

to Dale per conversation with him

Comment 3

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/57095660

Comment 4

[Preeti Raghunath(:Preeti)]

Brad,

Why has a story been created?

I am a little concerned as it is very late for 1.1 to be landing any user stories.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

"story" is misleading. This isn't a user story, it is a bug.

Comment 6

[Preeti Raghunath(:Preeti)]

Brad,

Is this on the partner build?

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

that would be a question for the reporter

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

it is on a buri partner build. yes
i feel it's a bug

Comment 9

[Dale Harvey (:daleharvey)]

Attachment: 915645.patch

So this check will prevent anything > 300ms from being registered as a tap at all, it doesnt effect the double tap logic, as far as I can tell there is no reason for it (long press doesnt wait for TOUCH_END) and will cancel the tap.

The best theory is that the code was ported from the Java panzoomcontroller as is, this bug was also fixed in the java version seperately.

Comment 10

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

Comment on attachment 813847 [details] [diff] [review]
915645.patch

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

I don't think this is the right solution because it means that for a double-tap, the first tap has basically an unbounded timeout and the second tap is still subject to MAX_TAP_TIME. I would rather you just increased the MAX_TAP_TIME constant which should also fix this bug.

Comment 11

[Dale Harvey (:daleharvey)]

Not sure increasing MAX_TAP_TIME is a better solution as it means increasing the delay before registering clicks on a zoomable page (+unzoomable before 922896 is fixed)

Copying androids version seems reasonable here, if tap > 300ms then fire a single tap, otherwise wait for a double tap before firing single

Comment 12

[Dale Harvey (:daleharvey)]

Attachment: Allow single taps longer than MAX_TAP_TIME

Double taps will need to be 2 taps shorter than 300ms, if we have a tap that takes longer than 300ms then it fires a single tap immediately.

Comment 13

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

Comment on attachment 814352 [details] [diff] [review]
Allow single taps longer than MAX_TAP_TIME

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

So the way I understand it, the current behaviour is like so:
1. If the time between 1st touchdown and 1st touchup is:
1a. 500ms or up: long-tap and return
1b. 300ms or up: ignore and return
1c. else: process as a tap and continue to step 2
2. If the time between 2nd touchdown and 2nd touchup is:
2a. 300ms or up: ignore and return
2b. else: continue to step 3
3. If the time between 1st touchup and 2nd touchup is:
3a. 300ms or less: double-tap and return
3b. else: process as a second tap

The problem in this bug is that state 1b is hit often.

The new behaviour is like so:
1. If the time between first touchdown and touchup is:
1a. 500ms or up: long-tap and return
1c. else: process as a tap and continue to step 3
3. If the time between 1st touchup and 2nd touchup is:
3a. 300ms or less: double-tap and return
3b. else: process as a second tap

So while it eliminates the former state 1b, it also eliminates all of step 2. Which means you could do a 200ms tap followed by a 1 minute tap, and it would still get counted as two individual taps. I don't know if this behaviour is desirable or not but I would think not. I think you still want to have step 2 which checks that the second tap has a bounded length. I think the desired behaviour is actually this:

1. If the time between 1st touchdown and 1st touchup is:
1a. 500ms or up: long-tap and return
1c. else: process as a tap and continue to step 2
2. If the time between 2nd touchdown and 2nd touchup is:
2a. 500ms or up: ignore and return
2b. else: continue to step 3
3. If the time between 1st touchup and 2nd touchup is:
3a. 500ms or less: double-tap and return
3b. else: process as a second tap

And this can be achieved by simply always setting MAX_TAP_TIME to the same value as the long-press timeout (which is currently a pref defaulted to 500). Thoughts?

Comment 14

[Dale Harvey (:daleharvey)]

My worry about that is we would be increasing what already is a very large delay in sending the events to content, however with https://bugzilla.mozilla.org/show_bug.cgi?id=922896 this delay would only happen in content not optimised for mobile so it may not be as big of an issue?

Will test an ammended patch with your suggestion now

Comment 15

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

Just to update the bug: Dale and I discussed this on IRC and increasing the MAX_TAP_TIME is problematic because of what he said in comment 14. Although this situation is similar to the dblclick mouse event the solution there (send a click event followed by a dblclick event) cannot really be used here because you might double-tap on a link to zoom in and you don't want to end up following the link on the first tap.

Dale will look into what other mobile browsers do to handle this scenario so that maybe we can do the same thing.

Comment 16

[Dale Harvey (:daleharvey)]

As best as I can read the code, the implementation of chrome on android will fire taps of any unbounded length

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java&q=getDoubleTapTimeout&sq=package:chromium&type=cs&l=485

this doesnt correspond to events inside the content though, using http://patrickhlauke.github.io/touch/tests/event-listener.html I can see that click events arent getting fired in content when you tap on a button for ~>600ms, however when tapping on a link, and length pressed thats lower than the long press triggers navigation, that suggests the taps are unbounded, but are always interrupted by a long press, even if the long press doesnt trigger a context menu

I think that suggests that we should seperate out DOUBLE_TAP_TIMEOUT from the TAP_TIMEOUT, where a double tap requires <300ms tap, <300ms between, <300ms tap, where as single taps will still be fired as long as they are below ui.click_hold_context_menus.delay

Sounds right?

Comment 17

[Dale Harvey (:daleharvey)]

Looking at this the original patch does as we want it to, when we are waiting on a tap we run a timer for longtaps (http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/GestureEventListener.cpp#91) if we hit that then we cancel the WAITING_X_TAP state (http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/GestureEventListener.cpp#348) 

So taps are bounded < click_hold_pref and double taps need to be < 300ms

Comment 18

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

(In reply to Dale Harvey (:daleharvey) from comment #16)
> I think that suggests that we should seperate out DOUBLE_TAP_TIMEOUT from
> the TAP_TIMEOUT, where a double tap requires <300ms tap, <300ms between,
> <300ms tap, where as single taps will still be fired as long as they are
> below ui.click_hold_context_menus.delay
> 
> Sounds right?

I can live with this, sure.

(In reply to Dale Harvey (:daleharvey) from comment #17)
> Looking at this the original patch does as we want it to,

Ok, I'll take a look at the first patch again tomorrow. Leaving needinfo as a reminder to myself.

Comment 19

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

The first patch is close but still not quite there. For example, one scenario that fails is if you do a 400ms tap followed immediately by a 100ms tap. The 400ms tap gets treated as a regular tap, but the code branch it takes puts mState into GESTURE_WAITING_DOUBLETAP, and so the 100ms tap after will in fact kick off a double-tap event. This is not in line with your "<300ms tap, <300ms between, <300ms tap" behaviour in comment 16.

Comment 20

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

Comment on attachment 814352 [details] [diff] [review]
Allow single taps longer than MAX_TAP_TIME

Minusing this second patch to clear it from my queue.

Comment 21

[Dale Harvey (:daleharvey)]

Attachment: 915645.patch

Good catch. Sorry for the delay, on PTO but wanted to get this patch cleared, havent been able to test properly due to issues getting device online, but will make sure to test properly if the code looks ok.

Comment 22

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

Comment on attachment 818032 [details] [diff] [review]
915645.patch

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

This looks good, thanks!

Comment 23

[Dale Harvey (:daleharvey)]

Cheers, on testing this I am seeing event.mTime == 0, which causes 

   printf_stderr("GestureEventListener ignoring double tap %d %d %d", mLastTapEndTime, mTapStartTime, event.mTime);

   I/Gecko   (  728): GestureEventListener ignoring double tap 1805497 1805214 0

gonna investigate more, might just be the wrong way to print it out, it seems like the problem its causing would happen regardless of this patch, but will investigate whats causing it before landing

Comment 24

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

(In reply to Dale Harvey (:daleharvey) PTO - 28th Oct from comment #23)
>    printf_stderr("GestureEventListener ignoring double tap %d %d %d",
> mLastTapEndTime, mTapStartTime, event.mTime);

mLastTapEndTime and mTapStartTime are uint64_t, so you need to use %lld to print them. Otherwise printf will misinterpret your data and print basically garbage.

Comment 25

[Dale Harvey (:daleharvey)]

Attachment: 915645.patch

Carrying r+, had |mTapStartTime - mLastTapEndTime| the wrong way round, pushed to try 

https://tbpl.mozilla.org/?tree=Try&rev=8a53759f8fab

Comment 26

[Dale Harvey (:daleharvey)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98725e0b432c

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/98725e0b432c

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4d7a24c243de
https://hg.mozilla.org/releases/mozilla-b2g18/rev/31fa87bfba88

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/31fa87bfba88