Closed Bug 811198 Opened 12 years ago Closed 12 years ago

Sticky like touch events occasionally seen when using CrazyCopter app

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED INVALID

People

(Reporter: jsmith, Assigned: mwu)

References

Details

Attachments

(1 file)

Generate clicks for single taps
2.96 KB, patch
Details | Diff | Splinter Review 
Build:

Device - Unagi

Hashes

  <project name="gaia" path="gaia" remote="b2g" revision="92b7a950de3ec1cdecd90fb9d2cd7f930bb1f227"/>
  <project name="releases-mozilla-aurora" path="gecko" remote="mozilla" revision="11e6372e034f7f19ad0b39749b48425a462120af"/>

Steps:

1. Install the app here - https://marketplace.mozilla.org/app/crazycopter/
2. Launch the app and play the game for a few seconds

Expected:

As you play the game, the touch event responsiveness should reflect how long you hold down on the phone.

Actual:

Occasionally in the game, you'll notice the touch events being "sticky," meaning clicking on the screen, taking your finger away, will appear to be still showing that the touch event is active. In this particular game, that will result in some frustrating, unintended deaths.
blocking-basecamp: --- → ?
Chris, do you know who can help with touch events?
blocking-basecamp: ? → +
chris ^^
Has the possibility of a bug in the game been ruled out?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Has the possibility of a bug in the game been ruled out?

We have confirmed that this issue in this bug only happens on FF OS, not on FF Android. The only other idea I could think of to 100% rule it out would be to switch the UA on this app to make sure there's no weird UA sniffing issues here.
We're also seeing similar issues in other games. Though of course they could be sharing some sort of library in theory.
mwu, i think this is a possible dup of 774458.  Over to you for debugging.  <3 doug
Assignee: nobody → mwu
This bug will be used to cover touch event correctness in sync scroll content. (bug 774458 for async scroll content) Setting target milestone accordingly.
Target Milestone: --- → B2G C2 (20nov-10dec)
Priority: -- → P1
Hardware: ARM → All
Depends on: 817806
I think this is pretty close to what we need, but there's two messy aspects that I'd like feedback on.

1. The single tap timer is currently hard coded in http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/GestureEventListener.cpp#23 . We just use the same value but that seems like a bad idea. Should I do something about this?
2. TOUCH_START_TOLERANCE is hard coded in http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#27 . We need to get that value and the DPI in the child process. Is calling TabChild::GetDPI efficient enough or should we attempt to cache? Or is it already cached somewhere? Should I also hardcode TOUCH_START_TOLERANCE in TabChild?
Attachment #687960 - Flags: feedback?(jones.chris.g)
Comment on attachment 687960 [details] [diff] [review]
Generate clicks for single taps

I think you mean "<" on the time check.

How is this going to work with long-press and double-click?
Attachment #687960 - Flags: feedback?(jones.chris.g)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> Comment on attachment 687960 [details] [diff] [review]
> Generate clicks for single taps
> 
> I think you mean "<" on the time check.
> 

It looks right to me. The check tells us to bail if the tap has taken too long.

> How is this going to work with long-press and double-click?

Long presses just disappear for now, and double taps turn into a series of single taps. It looks like sending a contextmenu event on long process would be a good idea to match Fennec. Double taps are normally never seen by pages in browsers, but we can do something special for them if we allow click latency to be increased.
Several gaia apps depend on long-press for context menu, simplest being homescreen to change wallpaper

I'm afraid this approach may not suffice unless those events are Just Working, or can be made to easily.  We may need to stick the GestureDetector in the pipeline here.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #12)
> I'm afraid this approach may not suffice unless those events are Just
> Working, or can be made to easily.  We may need to stick the GestureDetector
> in the pipeline here.

Shouldn't be too bad to add long press support. Double tap support would be annoying enough to consider borrowing GestureDetector though.
If this change needs to have a followup on the Homescreen, does it means other apps will be affected as well? If so can we know what has to change exactly?
(In reply to Vivien Nicolas (:vingtetun) from comment #14)
> If this change needs to have a followup on the Homescreen, does it means
> other apps will be affected as well? If so can we know what has to change
> exactly?

This will also break panning as it is currently implemented in BrowserElementScrolling.js. It seems like text selection will be also affected.
Also a lot of feedback in apps use the :active css class. This patch will regress that.
(In reply to Vivien Nicolas (:vingtetun) from comment #15)
> (In reply to Vivien Nicolas (:vingtetun) from comment #14)
> > If this change needs to have a followup on the Homescreen, does it means
> > other apps will be affected as well? If so can we know what has to change
> > exactly?
> 
> This will also break panning as it is currently implemented in
> BrowserElementScrolling.js. It seems like text selection will be also
> affected.
> Also a lot of feedback in apps use the :active css class. This patch will
> regress that.

I have a WIP to fix BrowserElementScrolling.js. I'll look into the text selection and :active css selectors.

I realize it's shitty to fix gaia to support this. We might want to consider implementing this behavior in the webapps wrapper where the right behavior really counts, but we'll probably want to have gaia depend on normal event behavior eventually. I don't like the normal mix of touch/mouse events on the web, but it's basically what everyone supports.
According to Vivien this issue and the related one (bug 817806) could break a lot of things. Should we block on those or should we wait for a next release ? It looks pretty risky !
blocking-basecamp: + → ?
Flags: needinfo?(jones.chris.g)
Flags: needinfo?(gal)
Flags: needinfo?(21)
I will note from watching Rob Hawkes's weekly reports + Lisa's app reviews that this issue is definitely evident with gaming-based apps. 

Without a fix for this, it will definitely impact the gaming experience on FF OS.
Whiteboard: [games]
I'm looking into less risky solutions now, but we basically break a lot of webapps if we don't fix this.
There seems to be something else weird going on on this app - it's using mouse events instead of touch events, but the js appears to support touch events.
So..

This app is actually just checking the useragent string to determine whether it should use touch events. Nothing in our useragent matches anything it knows so it just looks at Firefox, thinks it's on desktop, and uses mouse events.
The app has a bug in its mouse event handling which makes it get stuck when you drag.
User agent issue with the app - not a gecko bug. Closing as not valid.
Status: NEW → RESOLVED
blocking-basecamp: ? → ---
Closed: 12 years ago
Flags: needinfo?(jones.chris.g)
Flags: needinfo?(gal)
Flags: needinfo?(21)
Priority: P1 → --
Resolution: --- → INVALID
Whiteboard: [games]
Target Milestone: B2G C2 (20nov-10dec) → ---
Can we change the UA for this game?
(In reply to Andreas Gal :gal from comment #24)
> Can we change the UA for this game?

I filed a bug for that actually, but I believe Lawrence disagreed with doing a UA override since this game isn't popular enough to warrant an override. It so happens to be a marketplace app as well (along with many of the other games), so the best course of action was to outreach to the developer through the marketplace reviewers.
(In reply to Jason Smith [:jsmith] from comment #25)
> (In reply to Andreas Gal :gal from comment #24)
> > Can we change the UA for this game?
> 
> I filed a bug for that actually, but I believe Lawrence disagreed with doing
> a UA override since this game isn't popular enough to warrant an override.
> It so happens to be a marketplace app as well (along with many of the other
> games), so the best course of action was to outreach to the developer
> through the marketplace reviewers.

I missed this back in December.  Was the issue still reproducible?  (When I gave the game a quick try now it didn't appear to respond to touch events at all and the copter just flew into the ground)
(In reply to Andrew Williamson [:eviljeff] from comment #26)
> (In reply to Jason Smith [:jsmith] from comment #25)
> > (In reply to Andreas Gal :gal from comment #24)
> > > Can we change the UA for this game?
> > 
> > I filed a bug for that actually, but I believe Lawrence disagreed with doing
> > a UA override since this game isn't popular enough to warrant an override.
> > It so happens to be a marketplace app as well (along with many of the other
> > games), so the best course of action was to outreach to the developer
> > through the marketplace reviewers.
> 
> I missed this back in December.  Was the issue still reproducible?  (When I
> gave the game a quick try now it didn't appear to respond to touch events at
> all and the copter just flew into the ground)

Originally it was. The touch event issue however might be the same user agent problem.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: