Closed
Bug 915645
Opened 11 years ago
Closed 11 years ago
[Buri][TOR][Browser] Semi long press on links does nothing
Categories
(Firefox OS Graveyard :: Gaia::Browser, defect)
Tracking
(blocking-b2g:leo+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed)
RESOLVED
FIXED
blocking-b2g | leo+ |
People
(Reporter: jcheng, Assigned: daleharvey)
Details
(Keywords: qablocker, Whiteboard: [sprintready][systemsfe])
Attachments
(1 file, 3 obsolete files)
3.47 KB,
patch
|
daleharvey
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•11 years ago
|
blocking-b2g: --- → leo?
Flags: needinfo?(bfrancis)
Reporter | ||
Comment 1•11 years ago
|
||
this is v1.1 btw. on partner build
Reporter | ||
Comment 2•11 years ago
|
||
to Dale per conversation with him
Assignee: nobody → dale
Flags: needinfo?(bfrancis)
Updated•11 years ago
|
blocking-b2g: leo? → leo+
Comment 3•11 years ago
|
||
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/57095660
Updated•11 years ago
|
Whiteboard: [sprintready]
Comment 4•11 years ago
|
||
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.
Flags: needinfo?(blassey.bugs)
Comment 5•11 years ago
|
||
"story" is misleading. This isn't a user story, it is a bug.
Flags: needinfo?(blassey.bugs)
Comment 7•11 years ago
|
||
that would be a question for the reporter
Flags: needinfo?(blassey.bugs) → needinfo?(jcheng)
Reporter | ||
Comment 8•11 years ago
|
||
it is on a buri partner build. yes i feel it's a bug
Flags: needinfo?(jcheng)
Assignee | ||
Comment 9•11 years ago
|
||
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.
Attachment #813847 -
Flags: review?(bugmail.mozilla)
Comment 10•11 years ago
|
||
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.
Attachment #813847 -
Flags: review?(bugmail.mozilla) → review-
Assignee | ||
Comment 11•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 12•11 years ago
|
||
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.
Attachment #813847 -
Attachment is obsolete: true
Attachment #814352 -
Flags: review?(bugmail.mozilla)
Flags: needinfo?(bugmail.mozilla)
Comment 13•11 years ago
|
||
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?
Assignee | ||
Comment 14•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 16•11 years ago
|
||
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?
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 17•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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.
Flags: needinfo?(bugmail.mozilla)
Comment 20•11 years ago
|
||
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.
Attachment #814352 -
Flags: review?(bugmail.mozilla) → review-
Updated•11 years ago
|
Whiteboard: [sprintready] → [sprintready][systemsfe]
Assignee | ||
Comment 21•11 years ago
|
||
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.
Attachment #814352 -
Attachment is obsolete: true
Attachment #818032 -
Flags: review?(bugmail.mozilla)
Comment 22•11 years ago
|
||
Comment on attachment 818032 [details] [diff] [review] 915645.patch Review of attachment 818032 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks!
Attachment #818032 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 23•11 years ago
|
||
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•11 years ago
|
||
(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.
Assignee | ||
Comment 25•11 years ago
|
||
Carrying r+, had |mTapStartTime - mLastTapEndTime| the wrong way round, pushed to try https://tbpl.mozilla.org/?tree=Try&rev=8a53759f8fab
Attachment #818032 -
Attachment is obsolete: true
Attachment #823324 -
Flags: review+
Assignee | ||
Comment 26•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/98725e0b432c
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/98725e0b432c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 28•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4d7a24c243de https://hg.mozilla.org/releases/mozilla-b2g18/rev/31fa87bfba88
status-b2g18:
--- → fixed
status-b2g-v1.1hd:
--- → affected
status-b2g-v1.2:
--- → fixed
status-firefox26:
--- → wontfix
status-firefox27:
--- → wontfix
status-firefox28:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•