1080652 - [Tarako] investigate discarding bogus averaged touch events when multiple touches occur in the keyboard

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

This bug is to investigate whether we can improve the typing experience on Tarako following Chris Peterson's suggestion:

> have you guys read Ars Technica's recent (and harsh) review [1] of the Tarako device?
>
> The reviewer reports that the keyboard has multitouch problems where pressing the "Q" and "P" keys at the same time will generate a "Y" event. Have you seen this problem?
>
>Could this be a limitation of the screen itself? I assume the "Q" and "P" touch coordinates are getting averaged into a "ghost" touch event on the "Y" key. If so, maybe the keyboard app could filter out the ghost events if they have some distinctive radius, force, or rotationAngle values.
>

Comment 1

[David Flanagan [:djf]]

Taking this for preliminary investigation at the Gaia level. I'll pass it on to someone who knows touch events in Gecko if I can't make any progress.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I don't think we should workaround anything in Gaia Keyboard -- Gecko should be responsible of absorbing this.

(Even though arguably the workaround in Gaia Keyboard is relatively easier in 2.2 compare to previous keyboards since this workaround can be contained in UserPressManager)

Comment 3

[David Flanagan [:djf]]

Tim: for this bug, I'm investigating whether anything can be done in Gaia to improve the typing experience on Tarako, with the hope that we can come up with something relatively simple that could be pushed in an OTA update.  

So I'm changing the component back to keyboard for this one. It may also make sense to file a platform bug to see if anything can be done there. Please feel free to do that, especially if you know of someone who could work on it.

Comment 4

[David Flanagan [:djf]]

Here are my findings so far:

If I tap the Q key, I get touchstart/touchend with maybe a few touchmove events between them, as expected.

If I touch the Q key and then before releasing I touch the P key, then what I see is a touchstart on Q followed by a touchmove halfway across the screen.  Because this is a hardware limitation, there is only a single touch recorded, and it acts as if it teleported to a new location.

The radiusX, radiusY, rotationAngle and force properties of the Touch objects do not change, so we can't use those to tell when this has happened. (Though perhaps there is something that Gecko could do to use those fields to send more information when this happens)

But perhaps we can use touchmove events that have a large movement to detect this case.

Comment 5

[David Flanagan [:djf]]

A few more notes about the tarako keyboard...

1) When using the browser in landscape mode, the keyboard is in landscape. And this means that multitouch on Q and P works fine. We get both keys registered and input in the order in which they are released.  In this case, it is keys like T and V that combine to produce G that are the problem.

2) Except for backspace, we generate keys on touchend. In the case of Q and P combining to produce a Y it turns out to be quite difficult to actually type a Y that way unless you can lift both fingers simultaneously. Otherwise, if one finger lifts slightly before the other, we end up with more touchmove events and the touchend ends up being over some other key.

3) It is hard to type accurately on this keyboard. Part of that is probably the small screen size compared to the Flame. Part of that is the lack of autocorrect. And part is probably an inferior and less accurate touch sensor. I find that if I slow way down, I can do okay, though I really have to pay attention to the popup bubbles because sometimes they do not match the key I think I am typing.

4) Somewhere between 1.3T and master we've changed they way multitouches are handled. On Tarako, for the cases were we do register two separate touches, we show both highlighted keys, and send them in the order of the touchend events.  On master with Flame, if the user touches Q and then P, we send the Q event as soon as the P key is touched, even thought the finger on Q has not lifted up yet.  I don't know if converting to this system would make any difference on the Tarako.

So if we modify the keyboard app to ignore suspicious touchmove events, we'll have to be sure that when the touchend event arrives, we ignore the location associated with that event and fire whatever key it is that has the circle popup highlighted.

It is a bizarre bug that pressing Q and P makes the Y key highlight. And I think we can prevent that from happening. It is worth doing because this is a high-profile bug that makes it easy to mock our low-end phones that run on this cheap hardware.

The question of how we actually make typing better on Tarako is harder. I suspect that users will simply need to learn to type slowly and crisply and fully lift one finger from a key before touching with another finger.  The only thing I can think of that might help is that when we detect a rapid move that we think is caused by a failed multitouch, we could pulse the vibrator to signal that the input was botched, and that the user should slow down and retype those keys.

A more radical approach that we could experiment with would be to just always send key events when we get the first touchstart event. This might work well for users like me who just type and use the backspace key. But users who touch, then move their finger to the right key and then release would be out of luck.

Comment 6

[David Flanagan [:djf]]

See also bug 1038045 for more on the underlying hardware issue.

Comment 7

[David Flanagan [:djf]]

Funny: on the tarako, you can see that two touches at the same Y position cause a move by testing on the homescreen.  Tap on the left and right sides of the screen at the same time, and you can cause a swipe from one homescreen page to the next.

Comment 8

[David Flanagan [:djf]]

If there are symptoms on the homescreen, too, then that argues for a solution in the platform rather than in the keyboard.  But we'd need something that could prevent homescreen swiping with two taps and at the same time still allow fast swipes, so this would have to be calibrated carefully.  It might be too risky to land a platform fix, and if so, then a keyboard tweak might be what we want.

Comment 9

[David Flanagan [:djf]]

I just noticed something else about the Tarako touch hardware.  If I touch the Q and M keys on the keyboard, they are recognized as two separate touches. But the two touches still affect each other somehow and get averaged a bit so that the D and N keys are highlighted, at least some of the time.

Comment 10

[Mike Habicher [:mikeh] (high bugzilla latency)]

I just tested the Tarako's Android build (once I figured out how to switch it to English!) and it exhibits exactly the same behaviour as described in comment 5.

Comment 11

[David Flanagan [:djf]]

Its going to take some tweaking, but I'm having some success with a patch that discards sudden motion events that indicate likely multitouch issues.  I've started to think of these as "multitouch jams" like the kind of jam you'd get on an old manual typewriter if you tried to type too fast.  I'm going to add vibration feedback to alert the user to the fact that we're ignoring a touch that we can't interpret.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thank you for taking the initiatives to investigate during the sleeping hours of UTC+8. Let me comment on altogether.

(In reply to David Flanagan [:djf] from comment #3)
> Tim: for this bug, I'm investigating whether anything can be done in Gaia to
> improve the typing experience on Tarako, with the hope that we can come up
> with something relatively simple that could be pushed in an OTA update.  
> 
> So I'm changing the component back to keyboard for this one. It may also
> make sense to file a platform bug to see if anything can be done there.
> Please feel free to do that, especially if you know of someone who could
> work on it.

In that case we had a different taste on whether the workaround should be. Your finding is interesting and after reading all the comment I acknowledge the value of a possible workaround in keyboard app. So I retract my opinion there.

I don't know any platform engineer who can put this in Gecko -- will try to talk to people on Monday on this.

(In reply to David Flanagan [:djf] from comment #5)
> 4) Somewhere between 1.3T and master we've changed they way multitouches are
> handled. On Tarako, for the cases were we do register two separate touches,
> we show both highlighted keys, and send them in the order of the touchend
> events.  On master with Flame, if the user touches Q and then P, we send the
> Q event as soon as the P key is touched, even thought the finger on Q has
> not lifted up yet.  I don't know if converting to this system would make any
> difference on the Tarako.

That's bug 985855, and we are planning to roll out bug 985853 etc to make combo keys usable.

(In reply to David Flanagan [:djf] from comment #11)
> Its going to take some tweaking, but I'm having some success with a patch
> that discards sudden motion events that indicate likely multitouch issues. 
> I've started to think of these as "multitouch jams" like the kind of jam
> you'd get on an old manual typewriter if you tried to type too fast.  I'm
> going to add vibration feedback to alert the user to the fact that we're
> ignoring a touch that we can't interpret.

Your proposal sounds interesting and I am looking forward to see it. I would imaging, instead of

Actual: press down Q and P get you an Y.

your patch would do

Expected: press down Q and P keeps the Q activated, and the press on P ignored (with a vibration)

?

We probably need to do more combination testing (e.g. continue to press on P then lift the finger on Q etc) to see if the fix here is robust enough.

Comment 13

[David Flanagan [:djf]]

I sent a status update about this bug by email and realized that I should post it here as well:

A platform fix would probably be better than a gaia fix if we find the level of risk acceptable. Any fix based on discarding suspicious move events is going to have problems with false alarms when the user actually does quickly swipe their finger horizontally.  So a fix that works well for the keyboard app (which has no horizontal swiping) would probably completely break a game like Fruit Ninja.

So I'm investigating a fix in gaia because 1) it seems like a quick way to investigate 2) it gives is the most flexibility to tune the fix to the app. If I can get something working, maybe we'll be able to use my technique in the platform.

I started with a keyboard-specific fix. At the end of the day yesterday, however, I realized that I can probably write a shared module to do this fairly generically for any app that wants to opt in, so I'm going to try to get that done today. (With a capturing event handler at the window level that detects and discards the bogus move events, and retroactively sends fake move events to undo bogus moves detected after the fact.)

My keyboard-specific code mostly works, and seems like an improvement over the app without the fix. It turns out, however, that when we have two touches at the same time, we actually get a series of move events and not a single one as I first reported. It is easy to detect a large, suspicious move event. But the smaller, less suspicious move event that comes before it does sometimes get processed. So with my wip patch, if I place my finger on the K key, I see the K bubble. Then if I tap the A key, sometimes that K bubble moves a little to the left and becomes a J bubble either on the press or release of the A. This is better than without the fix when a G bubble would appear.  But I'm hoping that I can get it so the J bubble never appears at all.

I'll note also that this hardware issue does manifest in other apps:

- As described in comment 7, I can pan the homescreen by tapping on the left and right edges at about the same time. 

- In homescreen, a horizontal pinch gesture will often pan to a new homescreen page.

- In gallery, I can pan between photos in the same way

- In gallery, if I do a vertical pinch to zoom in on a photo and then try to do a horizontal pinch to zoom back out, I just end up moving the photo without zooming it.

- In gallery, if I hold the phone in landscape mode, then a vertical pinch gesture will not zoom in, but a horizontal one well.

Just to be clear: there is no way we can make the pinch gestures work on this low-end hardware. My only goal here is to detect the attempt so that we can give feedback to the user and so that we can prevent a broken pinch from being turned into a pan gesture, for example.

I'm hoping to come up with a fix that is generic enough for other Gaia apps to use, but I'm keeping this as a keyboard bug because I only play to apply the fix to keyboard for this bug.

Comment 14

[David Flanagan [:djf]]

Attachment: link to 1.3t patch on github

The attached patch is Tarako-specific and was written in response to this review http://arstechnica.com/gadgets/2014/10/testing-a-35-firefox-os-phone-how-bad-could-it-be/2/ 

In particular, it addresses the embarassing problem that pressing Q and P at the same time gives you a Y. Typing on a Tarako is still going to suck pretty badly, but at least reviews won't be able to point to the Q/P bug. The bug can still be made to occur, but it now takes a very careful gentle touch, so it is unlikely to happen in practice.

Tim and/or Rudy: how does this patch look to you?

Mike: are you (or someone on your team) able to test this patch out on a Tarako and see whether it improves the UX of typing on the Tarako? Note in particular that I've tried to add a subtle vibration as feedback when we detect a multitouch error. This tells the user that their second touch was not registered and basically reminds them to slow down.

Fabrice: does this seem like something we'd want to get into an OTA update for shipping Tarako devices?  (I forget who the TAM is for Tarako, but I know you were very involved, so I'm flagging you to get this on your radar.)

Comment 15

[[:fabrice] Fabrice Desré]

I have little hope that devices already sold can be updated through OTA (there's not enough space left on device for that, so updates are limited to sd card + recovery mode).
And I don't know either if partners are still taking changes on the 1.3t branch.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8503666 [details] [review]
link to 1.3t patch on github

I am pretty sure this will work for the designed scenario but I can't be certain without reading unit tests, even though you already written the comments quite clearly. The bottom line is code w/o tests can't get an r+.

Also, what would happen if I start moving the jammed finger? For example, moving the finger on A to Q. Would the script continue to filter out bogus touches that maybe have moved to T?

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

BTW, how about testing the patch on master? I am pretty sure Tarako will continue to work if you only update the keyboard package with |APP=keyboard make install-gaia|.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Got a reply from Shawn saying it's not possible to workaround this on platform:

---------- Forwarded message ----------
From: Shawn Ku <[e-mail]>
Date: Mon, Oct 13, 2014 at 10:22 AM

Hi Tim and all:
 I don't think there is any possibility to work around in platform side after check the raw date from kernel.

The reason is:
User only click P, then Q in this scenario. However, there are some un-existed touch reported from touch driver.
Besides, SPRD only report X/Y/Multi-Touch/Syn_report to Gonk/gecko, there is no extra information (such as pressure, size etc...) for us to to judge valid or not.

If any ideas, please kindly let us know.


Hi Rachelle:
  Please help notify SPRD this issue.


// P position
/dev/input/event1: EV_ABS       ABS_MT_POSITION_X    00000016            
/dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    0000013a            
/dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
/dev/input/event1: EV_SYN       SYN_REPORT           00000000            

// Fake Touch
/dev/input/event1: EV_ABS       ABS_MT_POSITION_X    0000003b            
/dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    0000013a            
/dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
/dev/input/event1: EV_SYN       SYN_REPORT           00000000

// // Fake Touch
/dev/input/event1: EV_ABS       ABS_MT_POSITION_X    000000a7            
/dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000139            
/dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
/dev/input/event1: EV_SYN       SYN_REPORT           00000000            

// Fake Touch
/dev/input/event1: EV_ABS       ABS_MT_POSITION_X    000000ee            
/dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000139            
/dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
/dev/input/event1: EV_SYN       SYN_REPORT           00000000            

// Q position
/dev/input/event1: EV_ABS       ABS_MT_POSITION_X    00000122            
/dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000138            
/dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
/dev/input/event1: EV_SYN       SYN_REPORT           00000000

Cheers,
Shawn Ku

Comment 19

[[:fabrice] Fabrice Desré]

David, something else about the patch here: I would rather not hardcode a list of broken devices, but instead use a specific property (eg. "has_crappy_touchscreen") and use this one in your code. The drawback being that this needs a gonk update...

Comment 20

[Chris Peterson [:cpeterson]]

David: is this fix relevant for any other devices we know about, such as Dolphin? Should we uplift this fix to 2.1 or even 2.0M?

Comment 21

[David Flanagan [:djf]]

Shawn,

One piece missing from your analysis is time.  What is the time difference between the various events? My gaia-based patch tries to figure out which events are bad by looking at how quickly they happen. In gaia they typically arrive 10 to 30ms apart.  If they arrive more closely together at the platform level (and arrive in a way that is distinct from a real finger swipe) then we might be able to use that to filter at the platform level.

Comment 22

[David Flanagan [:djf]]

(In reply to Fabrice Desré [:fabrice] from comment #19)
> David, something else about the patch here: I would rather not hardcode a
> list of broken devices, but instead use a specific property (eg.
> "has_crappy_touchscreen") and use this one in your code. The drawback being
> that this needs a gonk update...

That is an easy change to make in my patch if someone can do the gonk patch.  If you don't think it is worth doing that, we could do some other setting and ask vendors to use whatever build-time settings configuration we already have to add a 'keyboard.use_multitouch_workaround'.

Comment 23

[David Flanagan [:djf]]

Fabrice: would what I describe in comment #22 be okay with you?

Comment 24

[David Flanagan [:djf]]

Tim,

Thanks for the suggestion that we test this on master. You've changed the event handling in that version, right? So testing on master will show whether the patch works more generally.

I'm going to wait to add tests until we find out whether there is actually any interest in landing this patch.

Comment 25

[David Flanagan [:djf]]

(In reply to Chris Peterson (:cpeterson) from comment #20)
> David: is this fix relevant for any other devices we know about, such as
> Dolphin? Should we uplift this fix to 2.1 or even 2.0M?

Good question. According to bug 1038045, the touchscreen issue also occurs on the ZTE Open.  I can't reproduce it on my (pre-production) Dolphin.

I suppose we should ask QA to investigate which devices this patch would be helpful on.  Note that as it stands, the patch is Tarako-specific. So what we need right now is to find out which devices other than Tarako won't allow the user to type Q and P at the same time.

Comment 26

[[:fabrice] Fabrice Desré]

(In reply to David Flanagan [:djf] from comment #23)
> Fabrice: would what I describe in comment #22 be okay with you?

Sure.

Comment 27

[viral [:viralwang]]

(In reply to David Flanagan [:djf] from comment #21)
> Shawn,
> 
> One piece missing from your analysis is time.  What is the time difference
> between the various events? My gaia-based patch tries to figure out which
> events are bad by looking at how quickly they happen. In gaia they typically
> arrive 10 to 30ms apart.  If they arrive more closely together at the
> platform level (and arrive in a way that is distinct from a real finger
> swipe) then we might be able to use that to filter at the platform level.
Unfortunately, driver side reports similar events in two different behaviours
That's why we can't distinguish quick tapping and swipe now :(

case 1) tap 'q' and 'p' separately => 9~11ms for one point reported 
8734-525899: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    0000001e            
8734-525915: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000144            
8734-525920: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
8734-525923: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
8734-536215: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    00000032            
8734-536230: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000141            
8734-536235: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
8734-536238: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
8734-545837: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    00000090            
8734-545851: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    0000013c            
8734-545856: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
8734-545859: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
8734-555808: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    000000ba            
8734-555820: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    0000013b            
8734-555825: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
8734-555829: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
8734-565748: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    000000cc            
8734-565763: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    0000013a            
8734-565768: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
8734-565772: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
8734-575653: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    000000fa            
8734-575667: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000138            
8734-575672: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
8734-575675: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
8734-585631: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    0000011d            
8734-585647: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000136            
8734-585652: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
8734-585655: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
8734-595533: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    00000122            
8734-595548: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000135            
8734-595553: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
8734-595556: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
8734-605577: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    00000128            
8734-605591: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000135            
8734-605596: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
8734-605599: /dev/input/event1: EV_SYN       SYN_REPORT           00000000

case 2) swipe from left to right => still 9~11ms for one point reported 
9090-867390: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    0000000b            
9090-867404: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    0000014a            
9090-867409: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
9090-867412: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
9090-877629: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    00000022            
9090-877644: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000149            
9090-877648: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
9090-877651: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
9090-887294: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    0000003a            
9090-887308: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    0000014b            
9090-887313: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
9090-887317: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
9090-897256: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    0000005c            
9090-897270: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    0000014f            
9090-897275: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
9090-897278: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
9090-907242: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    00000085            
9090-907256: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000153            
9090-907260: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
9090-907264: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
9090-917194: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    000000a7            
9090-917210: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000156            
9090-917214: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
9090-917218: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
9090-927133: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    000000be            
9090-927148: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000155            
9090-927153: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
9090-927156: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
9090-937081: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    000000c6            
9090-937097: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000153            
9090-937102: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
9090-937106: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
9090-947107: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    000000df            
9090-947122: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    00000150            
9090-947127: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
9090-947130: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
9090-956951: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    000000f9            
9090-956967: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    0000014f            
9090-956971: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
9090-956974: /dev/input/event1: EV_SYN       SYN_REPORT           00000000            
9090-966852: /dev/input/event1: EV_ABS       ABS_MT_POSITION_X    0000010d            
9090-966878: /dev/input/event1: EV_ABS       ABS_MT_POSITION_Y    0000014b            
9090-966883: /dev/input/event1: EV_SYN       SYN_MT_REPORT        00000000            
9090-966886: /dev/input/event1: EV_SYN       SYN_REPORT           00000000

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to David Flanagan [:djf] from comment #24)
> Thanks for the suggestion that we test this on master. You've changed the
> event handling in that version, right? So testing on master will show
> whether the patch works more generally.

Probably. The event handling on master is not that different. If you could show the same script works on other apps that is probably more evident.

> I'm going to wait to add tests until we find out whether there is actually
> any interest in landing this patch.

Sure. I am getting confusing messages too -- there were e-mail saying that "we should not have devices come with screen like this in the future" and there were other e-mails saying otherwise. Shawn could probably comment.

Comment 29

[shawn ku [:sku]]

(In reply to David Flanagan [:djf] from comment #21)
> Shawn,
> 
> One piece missing from your analysis is time.  What is the time difference
> between the various events? My gaia-based patch tries to figure out which
> events are bad by looking at how quickly they happen. In gaia they typically
> arrive 10 to 30ms apart.  If they arrive more closely together at the
> platform level (and arrive in a way that is distinct from a real finger
> swipe) then we might be able to use that to filter at the platform level.

Hi David:
 Please help check comment 27 to see if viral answer the question you raised?
BTW, you can use below command to get extra event type, name and time stamp while looking the kernel log.

// command
adb shell getevent -t -l

Comment 30

[David Flanagan [:djf]]

Shawn: yes, I think Viral answered my question. We don't get any more detail from the kernel timestamps than we do at the gaia level, so I don't see any advantage to trying to address this issue at the kernel or gecko levels.

Comment 31

[Omega Feng [:Omega] [:馮於懋]]

Comment on attachment 8503666 [details] [review]
link to 1.3t patch on github

The hardware limitation still exists, but I think the UX is improved by this patch.

Comment 32

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

I checked Flatfish, Flame, Buri and Open_C (all of the devices available to me) and was unable to reproduce these keyboard touch event issues.  Removing the qawanted tag as we cannot check any more devices.  Please feel free to readd the tag if you feel that this was not enough.

Comment 33

[David Flanagan [:djf]]

Marcia: can you find out who could get this fix into the low-end device you showed me?

Comment 34

[Jan Jongboom [:janjongboom] (inactive)]

These issues happen on GeeksPhone Keon, all SC6821 based phones, GoFox F15.

We have a fix work in progress in Telenor, but it's targeted at gaia side, not driver. We verified the fix against real dataset and it shows a good improvement.

Brian has the numbers.

Comment 35

[David Flanagan [:djf]]

Jan: the patch attached to this bug is also a Gaia fix, not a driver fix.  I don't know if it is at all similar to what you're working on at Telenor

Comment 36

[Jan Jongboom [:janjongboom] (inactive)]

Yeah, I know, we'll run the test set against your patch and against the patch Brian wrote and see which one performs best.

Comment 37

[GitHub Autolander Bot]

Attachment: [gaia] brianweet:bug1080652 > mozilla-b2g:master

Comment 38

[Brian Weeteling [:brianweet]]

Created a fix specifically for the keyboard. We check for touch events with specific characteristics (fast movement). When these characteristics are met we initiate a click on the keys at the touchstart and the touchend coordinates. If we would block the movement or cancel the touchend event we lose a character. With our fix the touchend event might not give us the exact coordinates because of averaging but it is likely that the fix provides us with a key close to the actual touchend location.

We did some tests against a dataset we collected from real users typing on the keyboard. The users did not get any feedback about typing errors and we disabled autocorrect. 

The error percentage for two users typing on a keon 
Test set               | base   | djf    | our fix | djf+ac | our fix+ac
romy/2015-03-24 18:56  | 14.59% | 13.20% | 11.09%  | 12.17% | 8.12%
sergi/2015-03-18 16:00 | 10.04% | 9.91%  | 9.29%   | 8.05%  | 7.43%

ac = autocorrect enabled

Comment 39

[David Flanagan [:djf]]

(In reply to Brian Weeteling [:brianweet] from comment #38)
> Created a fix specifically for the keyboard. We check for touch events with
> specific characteristics (fast movement). When these characteristics are met
> we initiate a click on the keys at the touchstart and the touchend
> coordinates. 

That's much simpler than what I did, and it looks like you got better results. I think this means that if the user touches Q and then touches P, you will immediately input a Q.  Then, if they release Q and then release P you'll input a P.  If I understand correctly, though, if they release the P first and then release the Q, they'll get a second Q input. I think that is different than what the keyboard currently does, but in practice, it may well be closer to what the user wants.

What happens if the user swipes quickly across the screen? I imagine that results in unexpected input, right? Your patch does not take direction into account, so it treats any rapid move as a bogus input instead of only checking for horizontal movements (when the phone is in portrait orientation). I think it would be an easy change to exceedSpeedLimit to only check for horizontal movement. And it would allow you to get rid of the Math.sqrt() call, too.

Comment 40

[Brian Weeteling [:brianweet]]

(In reply to David Flanagan [:djf] from comment #39)
> (In reply to Brian Weeteling [:brianweet] from comment #38)
> > Created a fix specifically for the keyboard. We check for touch events with
> > specific characteristics (fast movement). When these characteristics are met
> > we initiate a click on the keys at the touchstart and the touchend
> > coordinates. 
> 
> That's much simpler than what I did, and it looks like you got better
> results. I think this means that if the user touches Q and then touches P,
> you will immediately input a Q.  Then, if they release Q and then release P
> you'll input a P.  If I understand correctly, though, if they release the P
> first and then release the Q, they'll get a second Q input. I think that is
> different than what the keyboard currently does, but in practice, it may
> well be closer to what the user wants.
> 
> What happens if the user swipes quickly across the screen? I imagine that
> results in unexpected input, right? Your patch does not take direction into
> account, so it treats any rapid move as a bogus input instead of only
> checking for horizontal movements (when the phone is in portrait
> orientation). I think it would be an easy change to exceedSpeedLimit to only
> check for horizontal movement. And it would allow you to get rid of the
> Math.sqrt() call, too.

What we do right now is we submit the key at the touchstart position and the key at the touchend position once we notice a touchjam.
Say we touch the Q key then touch the P key, lift finger from Q key and finally lift finger from P key. If this process occurs 'fast' enough the user will see QP as input. (The input might also be e.g. QW if the touch didn't move to the P key yet because of the averaging effect.) 

Swiping will result in unexpected input indeed, but at I can't think of a way to differentiate between the two.  Regarding direction, the horizontal bands are quite big on the phones I have over here (keon and GoFox F15). I think it covers about 2 complete keyboard rows. Therefore movement in the vertical direction could also be a jammed multitouch.

Comment 41

[David Flanagan [:djf]]

I'm turning this over to you, Brian. I'm glad someone is working actively on it!

Comment 42

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8586114 [details] [review]
[gaia] brianweet:bug1080652 > mozilla-b2g:master

This patch is very nicely wired against the current modules. Great job.

It make more sense to do built-time customization for low-end devices than runtime detections, but that might be too much for this bug, so we can (unfortunately) stick with reading mozSettings for now.

One thing I dislike is the exposure of press object to target handlers, but I don't know what's the better approach here. If the difference of behavior really need to be crafted at target handlers, this is indeed the only way. 

However, this patch is conflict with bug 1145072, which asks *TargetHandler#commit method to return a promise to ensure the right event order. I kind of think putting the handling of the jammed events into the same queue will defeat our ability to detect it, so with that bug we would need some other approaches to fix this one.

Is it possible to absorb the jammed events into UserPressManager since that's our first line of defense, and it already absorb touch v.s. mouse events? Like, do something different in UserPressManager directly when the user reach the speed limit -- rather than sending startTarget|target|startTime|endTime in UserPress object and let the downstream to decide what the target is ...

This approach would however introduce a risk of overloading UserPressManager... you can decide where or not we need a LowEndDeviceUserPressManager inheriting UserPressManager or simply wrapping the different handling in if .. else..

(please create a new pull request for the new patch and leave this one as reference.)

Thank you for the patch!

Comment 43

[GitHub Autolander Bot]

Attachment: [gaia] brianweet:bug1080652-v2 > mozilla-b2g:master

Comment 44

[Brian Weeteling [:brianweet]]

Thanks for the feedback Tim!

The new patch absorbs jammed events in the UserPressManager. When the user reaches the speed limit and has moved his/her finger to a different key, the UserPressManager will move the current UserPress back to the start position and execute the onpressend function. Subsequently, the UserPressManager will create a new UserPress for the target at the end of the movement. 
Had to move the current press back to the start position because we need to commit the presses in the right order. (the touchstart matters, I guess that was the whole idea behind bug 1145072)

Comment 45

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8590303 [details] [review]
[gaia] brianweet:bug1080652-v2 > mozilla-b2g:master

Great work! I don't want to slow you down so I am setting r+ right now. There are just some readability improvement needed. The test part is tricky -- I don't think the test should pass but they passed -- you might want to double check the messages in console, maybe the promise chain happen to recover it.

Comment 46

[Jan Jongboom [:janjongboom] (inactive)]

Try is green. https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=004f12bc26017ee877101afe9d9fe6397f676717

Comment 47

[Jan Jongboom [:janjongboom] (inactive)]

Do we want to uplift this to 1.3t?

Comment 48

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/a0b9d4359cb266c827322c941f131f2855152256

Comment 49

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #47)
> Do we want to uplift this to 1.3t?

1.3t is freezed a long time ago... you would also have to re-write the patch to uplift I think.

Comment 50

[Jan Jongboom [:janjongboom] (inactive)]

But there's still new devices with 1.3 coming out. But yeah we need to rewrite anyway as we're probably taking this in our own 1.4 branch.