806540 - Keyboard doesn't allow multi-touch

Status: VERIFIED FIXED

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

Description

[Joe Drew (not getting mail)]

If I accidentally tap two keys at the same time, only the last one to be tapped takes effect. If I'm doing a multitouch action, eg typing ' and t in very quick succession with two different fingers, *both* should take effect according to when I let go of the key. This would make typing much easier, because you would not have to completely let go of the screen between each character.

Comment 1

[Ben Francis [:benfrancis]]

This is a nice to have, but not a v1 blocker.

Comment 2

[Josh Carpenter [:jcarpenter]]

Renoming. Keyboard performance is unacceptably poor. Anything we can do to improve should be a top priority.

Comment 3

[David Flanagan [:djf]]

Josh,

Could you be more specific about "keyboard performance is unacceptably poor"? Without specific bugs filed for specific performance issues, nothing is likely to change.

Are you talking about the time it takes for the keyboard to appear? The time it takes between tapping on a key and seeing the character appear in the input field? The time to see word suggestions?

Comment 4

[Peter La]

Hi David,

At Josh's request, I'm adding links to the relavant keyboard bugs:

814758 [Gaia::Keyboard][perf][uxtrust] Bringing up the keyboard is slow
814766 [Gaia::Keyboard][perf][uxtrust] Delay between pressing a keyboard key and when character gets added to screen
814778 [Gaia::Keyboard][perf][uxtrust] Keyboard tooltips render slowly and sometimes don't render at all

The first two have video attachments that show the problem and compare with our reference performance platform (Otoro running ICS 4.0.4).

I will be attaching videos to the 3rd bug today.

Comment 5

[Gordon Brander :gordonb]

As a two-thumb typer, this bug trips me up frequently when using the phone.

Comment 6

[Chris Lee [:clee]]

I support Josh here.  Overall keyboard usability is key to enable users to input anything.  The keyboard today has some issues and multi-touch is one of the primary ones.  

I wouldn't consider this a feature, and classify this as 'not implemented to spec' or 'buggy' based on the initial implementation.  

Josh, can you link to the specific pages of the keyboard spec so there's no confusion on what is expected from UX?  Thanks.

Comment 7

[Fabien Cazenave [:kaze]]

FWIW I also agree with Josh here. I’ve been using the Otoro as my only phone for a few months now, and the poor keyboard responsiveness is the #1 annoyance in my daily use.

Comment 8

[David Scravaglieri [:scravag]]

Chris, Not a V1 feature but if you want to block on this, do.

Comment 9

[Peter Dolanjski [:pdol]]

We will be blocking on some of the highest priority UX items.  This is near the top of the list since it spans across all applications and experiences on the device.

Comment 10

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 11

[:Margaret Leibovic]

Attachment: WIP

Changing the keyboard to use touch events turned out to be a bit trickier than I expected, but this seems to be working pretty well so far.

A few things to note:
-preventDefault() isn't currently working in the touch event handlers (bug 819102), so I'm just handling the subsequent mouse events myself
-I kept support for mouse events so that people can still test the keyboard on desktop (in the future it would be nice if things like desktop b2g just turned mouse events into touch events at a platform level)
-I got rid of our mouseover event handler and put its logic in the mousemove handler, since that better parallels touchmove
-Instead of creating and dispatching a new event in redirectMouseOver, I found that we could just handle that redirection directly in movePress, which was simpler
-I had to change some logic for showing the alternate keys menu in render.js because if we remove the key from the document, touch events will keep firing on it, but our listener won't catch them because the key isn't a child of the keyboard element anymore

Things I still need to look into:
-I didn't do anything to handle updating the isPressing flag for the touch events state
-I haven't been able to figure out how to get the onScroll listener to fire, but I still need to update that to handle the touch events case

Comment 12

[David Flanagan [:djf]]

Comment on attachment 691131 [details] [diff] [review]
WIP

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

I didn't realize that the issues would go as deep as they do, but it looks like you're getting them resolved.
I did not test the code or review it in depth, but skimming through it, it looks good to me.

I suspect that the onScroll thing only matters for asian input methods, and it might actually be vestigal. I'm okay with you removing it. If it needs to be put back in later when we need to support Asian IMs, then we can do it then.

If you're not using mouseover anymore, you may not need isPressing anymore, either.  Depending on your mousemove logic, I suppose.  

Thanks for taking this bug on!

::: apps/keyboard/js/keyboard.js
@@ +957,5 @@
>    IMERender.ime.setCapture(false);
>    evt.preventDefault();
>  
> +  if (touchKeys)
> +    return;

Is the idea here that if you have ever received a touch event you then just ignore the mouse events?  I think I might use a separate boolean flag instead of having touchKeys do double-duty here, but that seems like a good approach.

Comment 13

[David Flanagan [:djf]]

Comment on attachment 691131 [details] [diff] [review]
WIP

Forgot to set feedback+ when I commented earlier...

Comment 14

[:Margaret Leibovic]

Attachment: patch

I removed onScroll and isPressing, as suggested in comment 12, and this is working really well. I tried to do a whole bunch of things to break it, and I couldn't :)

I also tested with B2G desktop to verify that the mouse event logic didn't regress, and that still works as well.

Comment 15

[:Margaret Leibovic]

I should also note that I got rid of the mouseleave listener because I found that it wasn't ever firing, and the keyboard is acting fine without it. (Also, I didn't add a touchcancel listener because I found that, too, wasn't ever firing.)

Comment 16

[David Flanagan [:djf]]

Comment on attachment 691497 [details] [diff] [review]
patch

I haven't studied the code, but I've tried it out, and am giving an r- because I noticed the following problems:

1) In the feedback app, if I type something and then press and hold backspace until it backspaces past the beginning of the text, the autorepeat doesn't shut off when I lift my finger. Vibration is on, so I the phone just sits there and vibrates until I tap somewhere on the keyboard.

(On a related note, we should seek ux feedback about vibration during autorepeat backspacing.  It turns into one continuous vibrate, which feels wrong.)

2) If I press a with one thumb and then l with the other and then lift them (lift l first then a) and the timing is right, the accented character menu for a will pop up underneath the home row after I've released.  I have to press and release the keys before the accented characters would normally show up.  I'm guessing the problem is that there are two keys and only one timer.  When the l key is pressed, it overwrites the timer associated with the a key menu, so that timer never gets cancelled.

3) If I press and hold e with one finger, I see the accented character menu. Now if I select one of those characters with another finger, I get the accented character I just typed. But when I release with the first finger, I get another accented character.

4) If I put finger one on e and hold, I'll get the special character menu. Then, if I put finger 2 down on a key and then slide it over, the character menu for the e goes away.  I think it should either stay up or (maybe better) go away when the second finger first goes down.  We may want to simpify the implementationa and the UX by just saying that no special menus will be displayed if there is more than one finger on the screen

So this turns out to be harder than either of us anticipated. You may need to dig in and really redo the keyboard event handling code. Just tell yourself that the code needs a rewrite anyway.

Meanwhile, we should bring this bug back to the attention of UX to ask how vibration should interact with autorepeat.

And we should also make sure that everyone is onboard with this multitouch change. The early comments on this bug make it sound like it had turned into a more generic "make the keyboard fast" bug.  Are we all clear that this bug was marked blocking+ for the purpose of adding multi-touch support?

Comment 17

[David Flanagan [:djf]]

Needinfo'ing Josh for answers: 

1) Now that keyboard vibration is on by default, how do you want the autorepeat backspace to work?  It feels like one continuous vibration to me, which seems annoying.  That's not exactly part of this bug, but is something Margaret could probably fix.

2) How should the menus of accented characters work if more than one finger is down on the keyboard at a time? I suggest that the cleanest solution is to just not display the menus at all in this case.

3) However, if one finger presses and holds a key to bring up a menu of accented characters, can the other finger select one of those characters? I'd suggest here that the cleanest thing would be that once an accented character menu is displayed, a second finger can only select from that menu and cannot select any other keys on the keyboard.

Comment 18

[:Margaret Leibovic]

Thanks for the detailed comments. I'll look into these issues. I had noticed that autorepeat delete issue before, but I thought I fixed it. I must have failed!

(In reply to David Flanagan [:djf] from comment #16)

> And we should also make sure that everyone is onboard with this multitouch
> change. The early comments on this bug make it sound like it had turned into
> a more generic "make the keyboard fast" bug.  Are we all clear that this bug
> was marked blocking+ for the purpose of adding multi-touch support?

I think that multi-touch makes the keyboard experience way better. Without this change, I find that the keyboard often misses keys if I'm typing fast, and that's really frustrating.

We do need to sort out UX around the alternate characters menu, but I think those issues won't be too hard to fix once we figure out how we want things to behave. I'm interested to hear what Josh has to say.

Also, Josh, I'm not in the office today, but I'll be in tomorrow if you want to meet to play around with this.

Comment 19

[:Margaret Leibovic]

I've been investigating this permanent auto-repeat delete issue, and I found that the problem is that we don't get a touchend event in some cases, and then because of that we never clear the deleteInterval. I actually found that we get into a state where we stop sending any touch events.

I was using the SMS app for testing, and I found that this only happens in the textarea where the message goes, but not in the input where you enter a recipient.

I found that commenting out the inputMethod.click() line in sendDelete prevents us from getting into this state. I've been looking at latin.js to see if there's anything that would mess up our touch events, but nothing is jumping out at me. One thing I noticed is that we don't go through handlePunctuation() for the input, but we do for the textarea, so maybe that could be causing a problem?

I also tried commenting out the actual call to navigator.mozKeyboard.sendKey in keyboard.js, and I still got into the state without touch events, so I suspect the problem is on the gaia side. However, I'm cc'ing cjones in case he might know what could cause us to get into this state without touch events.

Comment 20

[:Margaret Leibovic]

Doing some more debugging, I figured out this is a problem with 'latin-prose' input mode, more specifically the fact that we're calling IMERender.draw() in setUpperCase. I assume we're removing the original touchstart target from the DOM, and that's why we're not hearing any more touch events. I'll try to figure out a way to make this work...

Comment 21

[:Margaret Leibovic]

I decided to just attach the touchmove/touchend listeners directly to the target itself, and the platform keeps firing events on that target even if it's removed from the DOM, so that fixes the problem! It also means I don't need to change that _menuKey logic in render.js anymore.

Now I just need to fix the alternate character menu behavior.

Comment 22

[:Margaret Leibovic]

(In reply to David Flanagan [:djf] from comment #17)

For comparison, I decided to check what my Android phone does...

> 1) Now that keyboard vibration is on by default, how do you want the
> autorepeat backspace to work?  It feels like one continuous vibration to me,
> which seems annoying.  That's not exactly part of this bug, but is something
> Margaret could probably fix.

Android just vibrates on the first delete press, then doesn't vibrate anymore in auto-repeat delete mode.

> 2) How should the menus of accented characters work if more than one finger
> is down on the keyboard at a time? I suggest that the cleanest solution is
> to just not display the menus at all in this case.

This is what Android does.

> 3) However, if one finger presses and holds a key to bring up a menu of
> accented characters, can the other finger select one of those characters?
> I'd suggest here that the cleanest thing would be that once an accented
> character menu is displayed, a second finger can only select from that menu
> and cannot select any other keys on the keyboard.

On Android, if a second finger hits another key, it enters the currently selected accented character from the first key, then enters the next key. Also, you can only select an accented character with the same touch that opened the alternate character menu. Also, once a touch opens that alternate character menu, it doesn't go away until the end of the touch (the user must either select one of those alternate characters or lift their finger in another part of the screen to cancel the touch).

Comment 23

[Josh Carpenter [:jcarpenter]]

Hi Margaret and David

(In reply to Margaret Leibovic [:margaret] from comment #22)
> (In reply to David Flanagan [:djf] from comment #17)
> 
> For comparison, I decided to check what my Android phone does...
> 
> > 1) Now that keyboard vibration is on by default, how do you want the
> > autorepeat backspace to work?  It feels like one continuous vibration to me,
> > which seems annoying.  That's not exactly part of this bug, but is something
> > Margaret could probably fix.
> 
> Android just vibrates on the first delete press, then doesn't vibrate
> anymore in auto-repeat delete mode.

Yep, agreed. I find vibrating for each key press feels excessive.

> > 2) How should the menus of accented characters work if more than one finger
> > is down on the keyboard at a time? I suggest that the cleanest solution is
> > to just not display the menus at all in this case.
> 
> This is what Android does.

Heh, I wrote a big long screed on this and then rethought it. I agree with the above :) If user is pressing an accented character, waiting for the menu to pop up, and they press another character with other finger, the popup on first press is canceled, and it never appears. The user must release both fingers and then press the key again.

> > 3) However, if one finger presses and holds a key to bring up a menu of
> > accented characters, can the other finger select one of those characters?
> > I'd suggest here that the cleanest thing would be that once an accented
> > character menu is displayed, a second finger can only select from that menu
> > and cannot select any other keys on the keyboard.
> 
> On Android, if a second finger hits another key, it enters the currently
> selected accented character from the first key, then enters the next key.

Sounds good.

> Also, you can only select an accented character with the same touch that
> opened the alternate character menu. 

Also sounds good.

> Also, once a touch opens that alternate
> character menu, it doesn't go away until the end of the touch (the user must
> either select one of those alternate characters or lift their finger in
> another part of the screen to cancel the touch).

Perfect.

Thanks! It's fantastic that you're both working on this.

Comment 24

[Josh Carpenter [:jcarpenter]]

(In reply to Margaret Leibovic [:margaret] from comment #18)
> Also, Josh, I'm not in the office today, but I'll be in tomorrow if you want
> to meet to play around with this.

Sorry I missed you yesterday, but I'll be in MV today (Friday), or SF on Monday?

Comment 25

[:Margaret Leibovic]

(In reply to Josh Carpenter [:jcarpenter] from comment #24)
> (In reply to Margaret Leibovic [:margaret] from comment #18)
> > Also, Josh, I'm not in the office today, but I'll be in tomorrow if you want
> > to meet to play around with this.
> 
> Sorry I missed you yesterday, but I'll be in MV today (Friday), or SF on
> Monday?

Aw, I have to be in SF today for an interview and MV on Monday for another interview :( Then I'm going out of town for the holidays on Tuesday, so maybe you'll just have to wait until this lands to try it out. I'll definitely be available to fix follow-ups or regressions! 

Yesterday I tried implementing the things I mentioned in comment 22. Points 1 and 2 were easy to implement, but I found point 3 to be more complicated. At the end of the day, I made a patch that just doesn't let you start a second touch if the alternate key menu is open, and this prevented a lot of the weird edge cases I came up against. I feel like we should consider just landing this first, since it doesn't regress our current behavior (where we only let you have one touch no matter what), and it minimizes risk. I'll clean this patch up and post it for review.

I can continue to try to make this work, but even Android's keyboard has some unpredictable weridness around multiple touches and the alternate characters menu, so I'm worried that this will be difficult to get right.

Comment 26

[:Margaret Leibovic]

Attachment: patch v2

This patch addresses the issues that were raised earlier. New things in this version:
- touchmove and touchend listeners attached directly to target, so we don't need to worry about what happens if the original touchstart target gets removed form the DOM
- removed hideMenuTimeout because it wasn't used anymore (we only called hideAlternatives(true) in the onMouseLeave handler that wasn't being fired)
- removed triggerFeedback() call for auto-repeat delete
- don't show alternative characters menu if there's more than 1 touch
- ignore new touches if the alternatives menu is open (my compromise solution from comment 25)
- created setMenuTimeout() helper function (this actually fixes a bug where we were setting menuTimeout in movePress without calling movePress again to redirect the current key)

Unfortunately, I found one new problem with auto-capitalization. If you have an empty 'latin-prose' textarea, press A with one finger, press P with another finger, lift the finger on A, the keyboard will be replaced with the lower-case keyboard after the A is entered. However, when you lift your finger that was holding the P, a P will still be entered because we were holding onto that as the current key for that touch. I'm not sure if this is expected or not.

This on its own isn't *too* bad, but I've also noticed that if you start typing really quickly while in this auto-capitalization mode, the keyboard won't switch back to being lower case. I don't know if this is a bad enough problem to stop us from landing this, but I'll start looking into a fix.

Comment 27

[David Flanagan [:djf]]

Comment on attachment 692509 [details] [diff] [review]
patch v2

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

Overall, the code looks quite solid, and on testing, it seems to work well.  Good work!

I've made a whole bunch of comments, however. Many are minor suggestions. A few look like actual bugs that you should fix.

This is almost an r+. I don't think any of the fixes should be hard.

::: apps/keyboard/js/keyboard.js
@@ -165,5 @@
> -// If user leave the original key and did not move to
> -// a key within the accent character menu,
> -// after HIDE_ALTERNATIVES_MENU_TIMEOUT the menu will be removed.
> -const HIDE_ALTERNATIVES_MENU_TIMEOUT = 700;
> -

I'm not sure why there were two constants here. Have you tested with multiple keyboard layouts enabled? That gives a globe key for switching layouts, and I fear that this alternatives menu thing might have been for that...

But when I test the multiple layout switcher works fine without it, so nevermind.

@@ +765,5 @@
>  }
>  
>  // Sends a delete code to remove last character
>  // The argument specifies whether this is an auto repeat or not.
>  function sendDelete(isRepeat) {

Could you leave a comment in here indicating that the initial vibration or click was already done and we're purposely not doing any more?

@@ +789,5 @@
> +  menuTimeout = window.setTimeout(function menuTimeout() {
> +    showAlternatives(target);
> +
> +    // Redirect the press over the first key in the menu
> +    movePress(target, coords, touchId);

You probably shouldn't do this if the showAlternatives() fails because touchCount > 1.  Maybe move that test to this function instead of in showAlternatives()?

@@ +898,3 @@
>  
> +  handleTouches(evt, function handleTouchStart(touch, touchId) {
> +    var target = evt.target;

I don't know whether gecko can ever deliver one touch start event with two distinct Touch objects in the changedTouches array.

But this code assumes that it might, and in that case, the two touches probably have different target elements, and you can't use evt.target.  Don't you have to use touch.target?

Is that property reliable, can you count on it?

@@ +899,5 @@
> +  handleTouches(evt, function handleTouchStart(touch, touchId) {
> +    var target = evt.target;
> +
> +    // Add touchmove and touchend listeners directly to the target so that we
> +    // will always hear these events, even if the target is removed from the DOM.

Continue this comment to explain that the target can be removed when the IM decides to switch cases. Is there any other way you know of that this can happen?  It just seems obscure enough that it is worth explaining.

@@ +910,5 @@
>  }
>  
> +function onTouchMove(evt) {
> +  handleTouches(evt, function handleTouchMove(touch, touchId) {
> +    // evt.target is the element that the touch started on, so we

touch.target, not evt.target, right?

@@ +912,5 @@
> +function onTouchMove(evt) {
> +  handleTouches(evt, function handleTouchMove(touch, touchId) {
> +    // evt.target is the element that the touch started on, so we
> +    // need to find the new key with elementFromPoint
> +    var target = document.elementFromPoint(touch.pageX, touch.pageY);

I wonder how expensive this call is...

Is it worth recording the initial pageX, pageY of the touchstart event, and only calling this if the move is greater than some threshold?  

If I tap the keyboard right between two keys, I can have the highlighted letter switch back and forth just by rocking my finger. I'm not conscious of moving my finger at all, but I am moving it a bit and the keyboard is respondinging when I do switching from one key to the next.  

Inserting a check here to see if the move is > 5px in either direction would probably eliminate 95% of the calls to elementFromPoint()

@@ +919,5 @@
> +}
> +
> +function onTouchEnd(evt) {
> +  touchCount = evt.touches.length;
> +

Have you confirmed that touches.length is the number you want here?  That is, do you have to subtract changedTouches.length?  I don't actually know myself, so I'm just checking.

@@ +921,5 @@
> +function onTouchEnd(evt) {
> +  touchCount = evt.touches.length;
> +
> +  handleTouches(evt, function handleTouchEnd(touch, touchId) {
> +    var target = evt.target;

touch.target instad of evt.target?

@@ +923,5 @@
> +
> +  handleTouches(evt, function handleTouchEnd(touch, touchId) {
> +    var target = evt.target;
> +    target.removeEventListener('touchmove', onTouchMove);
> +    target.removeEventListener('touchend', onTouchEnd);

To be safe, maybe register and unregister onTouchEnd as the handler for touchcancel events, too?  I think you said that you never saw them occur... But gecko's event stuff seems pretty unstable these days.

@@ +944,4 @@
>  
> +// Mouse events will fire after touch events. Because preventDefault()
> +// isn't working properly for touch events (bug 819102), we need to
> +// handle these subsequent mouse events ourselves.

Are you intentionally not calling preventDefault() in the touch handlers because of that bug?

If you call it, and the bug gets fixed, then the keyboard will automatically be a little more efficient without having to receive and reject the mouse events.

But you can't test.  I'm not sure whether it is better to put in the calls to preventDefault() or not.  If you don't put them in, maybe you could add FIXME comments to the touch event handlers marking where you think they should go once 819102 is fixed.

@@ +970,5 @@
>    triggerFeedback();
>  
> +  // Only show alternatives if there is one touch
> +  if (touchCount === 1)
> +    setMenuTimeout(target, coords, touchId);

I don't think you should test touchCount here, since you also test it in setMenuTimeout() or in showAlternatives.

With this code, if I press n then quickly press a, then very quickly release n, I still expect to see accented characters for a, but I don't. It seems to me that this might arise when typing quickly, and ought to be easy to fix by removing the test here.

I'm making this suggestion on the assumption that it is an easy change. If removing the test here would require bit workarounds elsewhere, it isn't worth it.

@@ +1000,1 @@
>    return (isShowingAlternativesMenu &&

Consider moving the isShowingAlternativesMenu test out of this function and into the caller.  Most of the time, it will be false, and the callers can test that themselves quickly without the overhead of a function call. 

Just a tiny optimization, but since we do it on every move event, it might be worth it.

@@ +1021,5 @@
>    // Control locked zone for menu
> +  if (inMenuLockedArea(coords)) {
> +    var menuChildren = IMERender.menu.children;
> +    var redirectTarget = menuChildren[Math.floor(
> +      (coords.pageX - menuLockedArea.left) / menuLockedArea.ratio)];

Ugh. This code will probably break if the alternatives menu ever mixes wide alternatives (like "4th") with narrow alternatives like "$".  That's unrelated to this patch though, and I don't think we currently have any menus that would break.

@@ +1063,3 @@
>  
>    // Control showing alternatives menu
> +  setMenuTimeout(target, coords, touchId);

I've argued above in startPress() that you shouldn't test for touchCount.  But if you do keep the touchCount test there, then you should have it here, too, right?

@@ +1066,4 @@
>  
> +  function setCurrentKey(value, touchId) {
> +    if (touchEventsPresent)
> +      touchKeys[touchId] = value;

It just occured to me that if you changed the name touchKeys to touchedKeys it would suddenly make a lot more sense to me.

Comment 28

[David Flanagan [:djf]]

(In reply to Margaret Leibovic [:margaret] from comment #26)
> 
> Unfortunately, I found one new problem with auto-capitalization. If you have
> an empty 'latin-prose' textarea, press A with one finger, press P with
> another finger, lift the finger on A, the keyboard will be replaced with the
> lower-case keyboard after the A is entered. However, when you lift your
> finger that was holding the P, a P will still be entered because we were
> holding onto that as the current key for that touch. I'm not sure if this is
> expected or not.

I can't think of an obvious fix, and I don't know whether this is worth worrying too much about. Certainly we ought to land this patch without worrying about this.

What's strange about it to me is that sometimes the second letter is uppercase and sometimes lowercase. Like it is timing dependent.  I'd consider that the bug.  If the user sees an uppercase letter when the touch the second letter, they should probably see an uppercase letter when they release it. 

Its kind of deep bug, though, because we display the preview menus on touch start, but don't send anything to the latin input method until touch end, and it is the input method that controls the upper/lower case transition.

I'd say that this is a corner case that we ignore for v1 unless we get a lot of user complaints.

The only way I can really see to address this is to start sending keys on touch start, and then get smart about accented character menus and send a backspace before sending the accented character.  That might actually make the keyboard feel very responsive.  (But if it is not what Android and iOS do, then maybe there is some other good reason that it won't work.)

Alternatively, I suppose we could disable multi-touch when the keyboard is displaying uppercase letters, but that doesn't really feel like the right level to fix this at.

> 
> This on its own isn't *too* bad, but I've also noticed that if you start
> typing really quickly while in this auto-capitalization mode, the keyboard
> won't switch back to being lower case. I don't know if this is a bad enough
> problem to stop us from landing this, but I'll start looking into a fix.

The latin IM stays in uppercase mode if you type two capital letters in a row. It thinks you're shouting or typing an acronym or something, so that is intentional.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Flanagan [:djf] from comment #27)
> > +    var target = document.elementFromPoint(touch.pageX, touch.pageY);
> 
> I wonder how expensive this call is...

It will flush layout. We have ways to avoid that from chrome JS (the nsIDOMWindowUtils version has an aFlushLayout parameter), but I don't know of any exposed to content.

Comment 30

[:Margaret Leibovic]

(In reply to David Flanagan [:djf] from comment #27)

Thanks for the detailed comments! I'm working on addressing them now, so I should have a new patch up later today.

> ::: apps/keyboard/js/keyboard.js
> @@ -165,5 @@
> > -// If user leave the original key and did not move to
> > -// a key within the accent character menu,
> > -// after HIDE_ALTERNATIVES_MENU_TIMEOUT the menu will be removed.
> > -const HIDE_ALTERNATIVES_MENU_TIMEOUT = 700;
> > -
> 
> I'm not sure why there were two constants here. Have you tested with
> multiple keyboard layouts enabled? That gives a globe key for switching
> layouts, and I fear that this alternatives menu thing might have been for
> that...
> 
> But when I test the multiple layout switcher works fine without it, so
> nevermind.

The HIDE_ALTERNATIVES_MENU_TIMEOUT constant was only used in hideAlternatives was called with addDelay = true, and that only happened in the mouseleave handler that I removed. I'm not sure why it was declared as a separate constant. Future flexibility maybe?

> @@ +789,5 @@
> > +  menuTimeout = window.setTimeout(function menuTimeout() {
> > +    showAlternatives(target);
> > +
> > +    // Redirect the press over the first key in the menu
> > +    movePress(target, coords, touchId);
> 
> You probably shouldn't do this if the showAlternatives() fails because
> touchCount > 1.  Maybe move that test to this function instead of in
> showAlternatives()?

Good catch. We probably shouldn't be calling movePress at all if showAlternatives failed.

> @@ +898,3 @@
> >  
> > +  handleTouches(evt, function handleTouchStart(touch, touchId) {
> > +    var target = evt.target;
> 
> I don't know whether gecko can ever deliver one touch start event with two
> distinct Touch objects in the changedTouches array.
> 
> But this code assumes that it might, and in that case, the two touches
> probably have different target elements, and you can't use evt.target. 
> Don't you have to use touch.target?
> 
> Is that property reliable, can you count on it?

Good call, I'll use touch.target. I forgot that I could use that, but it's better for our purposes.

> @@ +899,5 @@
> > +  handleTouches(evt, function handleTouchStart(touch, touchId) {
> > +    var target = evt.target;
> > +
> > +    // Add touchmove and touchend listeners directly to the target so that we
> > +    // will always hear these events, even if the target is removed from the DOM.
> 
> Continue this comment to explain that the target can be removed when the IM
> decides to switch cases. Is there any other way you know of that this can
> happen?  It just seems obscure enough that it is worth explaining.

This also happens when we show the alternative characters menu, since we do that by actually replacing the key node with a new node for the alternatives menu (then putting it back when we hide the menu).

> @@ +910,5 @@
> >  }
> >  
> > +function onTouchMove(evt) {
> > +  handleTouches(evt, function handleTouchMove(touch, touchId) {
> > +    // evt.target is the element that the touch started on, so we
> 
> touch.target, not evt.target, right?

This is true of evt.target, but also true of touch.target. I'll switch the comment now that I'm switching my code to use touch.target.

> @@ +912,5 @@
> > +function onTouchMove(evt) {
> > +  handleTouches(evt, function handleTouchMove(touch, touchId) {
> > +    // evt.target is the element that the touch started on, so we
> > +    // need to find the new key with elementFromPoint
> > +    var target = document.elementFromPoint(touch.pageX, touch.pageY);
> 
> I wonder how expensive this call is...
> 
> Is it worth recording the initial pageX, pageY of the touchstart event, and
> only calling this if the move is greater than some threshold?  
> 
> If I tap the keyboard right between two keys, I can have the highlighted
> letter switch back and forth just by rocking my finger. I'm not conscious of
> moving my finger at all, but I am moving it a bit and the keyboard is
> respondinging when I do switching from one key to the next.  
> 
> Inserting a check here to see if the move is > 5px in either direction would
> probably eliminate 95% of the calls to elementFromPoint()

This sounds smart, especailly to avoid the highlighted letter switching back and forth. Like Gavin said, this call is expensive, so it would be good to reduce how often we need to call it. I can try playing around with this, but maybe it would also make for a good follow-up optimization.

I did try my patch with paint flashing enabled, and it didn't look like we're repainting more than we need to, so even if we are triggering reflows, they don't seem to be causing repaints.

> @@ +919,5 @@
> > +}
> > +
> > +function onTouchEnd(evt) {
> > +  touchCount = evt.touches.length;
> > +
> 
> Have you confirmed that touches.length is the number you want here?  That
> is, do you have to subtract changedTouches.length?  I don't actually know
> myself, so I'm just checking.

Yes, the touch is removed from the touches array before touchend fires.

> @@ +921,5 @@
> > +function onTouchEnd(evt) {
> > +  touchCount = evt.touches.length;
> > +
> > +  handleTouches(evt, function handleTouchEnd(touch, touchId) {
> > +    var target = evt.target;
> 
> touch.target instad of evt.target?

Yes, I'll switch that.

> @@ +923,5 @@
> > +
> > +  handleTouches(evt, function handleTouchEnd(touch, touchId) {
> > +    var target = evt.target;
> > +    target.removeEventListener('touchmove', onTouchMove);
> > +    target.removeEventListener('touchend', onTouchEnd);
> 
> To be safe, maybe register and unregister onTouchEnd as the handler for
> touchcancel events, too?  I think you said that you never saw them occur...
> But gecko's event stuff seems pretty unstable these days.

This sounds like a good precaution.

> @@ +944,4 @@
> >  
> > +// Mouse events will fire after touch events. Because preventDefault()
> > +// isn't working properly for touch events (bug 819102), we need to
> > +// handle these subsequent mouse events ourselves.
> 
> Are you intentionally not calling preventDefault() in the touch handlers
> because of that bug?
> 
> If you call it, and the bug gets fixed, then the keyboard will automatically
> be a little more efficient without having to receive and reject the mouse
> events.
> 
> But you can't test.  I'm not sure whether it is better to put in the calls
> to preventDefault() or not.  If you don't put them in, maybe you could add
> FIXME comments to the touch event handlers marking where you think they
> should go once 819102 is fixed.

Yes, I intentionally left it out to avoid unpredictability if the bug gets fixed. However, thinking about it more now, I don't think there's really any unpredictability, so I'll add that back in. We should actually be able to get rid of the touchEventsPresent checks once we fix bug 819102, so I put in a FIXME comment about that.

> @@ +970,5 @@
> >    triggerFeedback();
> >  
> > +  // Only show alternatives if there is one touch
> > +  if (touchCount === 1)
> > +    setMenuTimeout(target, coords, touchId);
> 
> I don't think you should test touchCount here, since you also test it in
> setMenuTimeout() or in showAlternatives.
> 
> With this code, if I press n then quickly press a, then very quickly release
> n, I still expect to see accented characters for a, but I don't. It seems to
> me that this might arise when typing quickly, and ought to be easy to fix by
> removing the test here.
> 
> I'm making this suggestion on the assumption that it is an easy change. If
> removing the test here would require bit workarounds elsewhere, it isn't
> worth it.

I added this because without it I was seeing some buggy behavior caused by the fact that there's only one menuTimeout id variable. Without this check, setMenuTimeout could get called for two successive touches, and if the touches are short enough to not show an alternatives menu, clearTimeout would only get called for the second timeout. When this happens, the keyboard would get into a borked state where a bad alternatives menu is showing. Then because of the check for isShowingAlternativesMenu in the touchstart handler, we could never start a new touch! Rather than try to support multiple simultaneous timeouts, I thought it would be simpler to just start the timeout when there's only one touch.

(We still need the check before actually showing the alternatives menu, because a new touch could have popped up between when we started the timeout and when we decide to show the alternatives menu.)

> @@ +1000,1 @@
> >    return (isShowingAlternativesMenu &&
> 
> Consider moving the isShowingAlternativesMenu test out of this function and
> into the caller.  Most of the time, it will be false, and the callers can
> test that themselves quickly without the overhead of a function call. 
> 
> Just a tiny optimization, but since we do it on every move event, it might
> be worth it.
> 
> @@ +1021,5 @@
> >    // Control locked zone for menu
> > +  if (inMenuLockedArea(coords)) {
> > +    var menuChildren = IMERender.menu.children;
> > +    var redirectTarget = menuChildren[Math.floor(
> > +      (coords.pageX - menuLockedArea.left) / menuLockedArea.ratio)];
> 
> Ugh. This code will probably break if the alternatives menu ever mixes wide
> alternatives (like "4th") with narrow alternatives like "$".  That's
> unrelated to this patch though, and I don't think we currently have any
> menus that would break.

We can file a separate bug to track this.

> @@ +1063,3 @@
> >  
> >    // Control showing alternatives menu
> > +  setMenuTimeout(target, coords, touchId);
> 
> I've argued above in startPress() that you shouldn't test for touchCount. 
> But if you do keep the touchCount test there, then you should have it here,
> too, right?

Good catch. I just moved this check into setMenuTimeout.

> @@ +1066,4 @@
> >  
> > +  function setCurrentKey(value, touchId) {
> > +    if (touchEventsPresent)
> > +      touchKeys[touchId] = value;
> 
> It just occured to me that if you changed the name touchKeys to touchedKeys
> it would suddenly make a lot more sense to me.

Sorry about that! I can change it.

Comment 31

[:Margaret Leibovic]

Attachment: patch v3

I believe this addresses all the comments (let me know if I missed anything!).

One weird issue I ran into is that when I started caching pageX/pageY along with the target in touchedKeys, if I typed really fast, sometimes I would get an error "TypeError: touchedKeys[touchId] is undefined" in onTouchEnd. It turned out that I was hearing two touchend events for the same touch sometimes, so I had already called delete touchedKeys[touchId]. I couldn't figure out exactly how to reproduce this, since it only happened if I typed really fast, so I decided to just add a check for touchedKeys[touchId]. Because I'm not sure what's causing this, I still want to call endPress for its cleanup logic.

The thought crossed my mind that I was calling onTouchEnd twice because I was getting a touchcancel event, but I checked to see that it was actually two touchend events. Maybe if I look at this tomorrow with fresh eyes I can figure out what's going wrong. This could have been an issue that existed before, but is only now exposed because I'm storing an object at touchedKeys[touchId], instead of just passing it along itself.

Comment 32

[David Flanagan [:djf]]

I wonder if this is a regression of https://bugzilla.mozilla.org/show_bug.cgi?id=785554

See line 96 of https://github.com/mozilla-b2g/gaia/blob/master/shared/js/gesture_detector.js

If that message is showing up in logcat when you end a pinch to zoom gesture in the Gallery app, then that bug has regressed and should be reopened.

Comment 33

[David Flanagan [:djf]]

I've confirmed that that message does show up in the console, so I have reopened #785554.  I'm not sure that we need to mark this bug as depending on that one, though.  I think we can pretty reliably work around it by just ignoring any touchend touch other than the first.

Comment 34

[Dietrich Ayala (:dietrich)]

Driver retriage: All drivers present were concerned about the risk in this patch. However, the rollup of keyboard fixes would be wonderful to have.

Leaving this blocking for now, with the requirement that it land in by Wednesday in order to get into the pre-holiday Stable branch deployment, so that there's enough dogfooding testing of the change before code freeze.

Comment 35

[David Flanagan [:djf]]

I've reclosed #785555 and opened a new bug #822558

Comment 36

[David Flanagan [:djf]]

Comment on attachment 693214 [details] [diff] [review]
patch v3

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

Don't update the cached mouse position every time you get a move event. Just store the initial coordinates, and then register the move once the touch has moved beyond your 5 (or 10) pixel threshold from the initial press.

And figure out how you want to workaround the double touches in touchend events from bug 822558.

And with those fixes, r+, and let's get it landed.

::: apps/keyboard/js/keyboard.js
@@ +796,5 @@
>  
> +  menuTimeout = window.setTimeout(function menuTimeout() {
> +    // Don't try to show the alternatives menu if it's already showing,
> +    // or if there's more than one touch on the screen.
> +    if (isShowingAlternativesMenu || touchCount > 1)

I get your point about not wanting to deal with multiple pending timeouts, so I'm find leaving this touchCount check in. If it has to change in the future, I can imagine not setting a timeout if a key has no alternatives, and then only setting the timeout if there is not already one pending or something like that. 

But let's just leave it as is for now.

@@ +940,5 @@
> +      return;
> +
> +    // Update our cached x/y values.
> +    touchedKeys[touchId].x = touch.pageX;
> +    touchedKeys[touchId].y = touch.pageY;

I don't think you should update these.  With the code this way, if you moved your finger very slowly, you'd never cross the threshold and the move would never be registered. I was thinking of an absolute distance threshold, not a move velocity threshold.  (And 5 pixels was just a wild guess...)

@@ +955,5 @@
> +  evt.preventDefault();
> +  touchCount = evt.touches.length;
> +
> +  handleTouches(evt, function handleTouchEnd(touch, touchId) {
> +    // Remove the event listeners from the original target.

Because of bug 822558, you might just want to handle the first touch in changedTouches manually instead of using your handleTouches function.

@@ +972,5 @@
> +// Helper function to iterate through a touch event's
> +// changedTouches array. For each touch, it calls a callback
> +// function with the touch and touchId as arguments.
> +function handleTouches(evt, callback) {
> +  for (var i = 0; i < evt.changedTouches.length; i++) {

I suppose you could also workaround 822558 here by checking evt.type === 'touchend'

Comment 37

[:Margaret Leibovic]

(In reply to David Flanagan [:djf] from comment #36)

> Don't update the cached mouse position every time you get a move event. Just
> store the initial coordinates, and then register the move once the touch has
> moved beyond your 5 (or 10) pixel threshold from the initial press.
> 
> And figure out how you want to workaround the double touches in touchend
> events from bug 822558.
> 
> And with those fixes, r+, and let's get it landed.

Okay, I'll update the patch and land it tomorrow morning. I just have a few questions...

> @@ +940,5 @@
> > +      return;
> > +
> > +    // Update our cached x/y values.
> > +    touchedKeys[touchId].x = touch.pageX;
> > +    touchedKeys[touchId].y = touch.pageY;
> 
> I don't think you should update these.  With the code this way, if you moved
> your finger very slowly, you'd never cross the threshold and the move would
> never be registered. I was thinking of an absolute distance threshold, not a
> move velocity threshold.  (And 5 pixels was just a wild guess...)

I think you would cross the threshold... they're only updated if we're actually going ahead with the movePress. For example, let's say you get a touchstart at (0,0) and move your finger horizonally. When you move to (1,0) or (4,0) we return and don't update the cache, but if you move to (5,0), we update the cache to now be (5,0) and call movePress(). Then, on the next touchmove, we won't update the cache and move again until you move to (10,0), but we will update the cache call movePress() eventually, even if you're dragging your finger really slowly.

With my testing 5px doesn't seem to feel any different from no threshold, so I think this is right (we can always tweak this number if people complain).

> @@ +955,5 @@
> > +  evt.preventDefault();
> > +  touchCount = evt.touches.length;
> > +
> > +  handleTouches(evt, function handleTouchEnd(touch, touchId) {
> > +    // Remove the event listeners from the original target.
> 
> Because of bug 822558, you might just want to handle the first touch in
> changedTouches manually instead of using your handleTouches function.

When I started working on this patch, I did notice getting two touches in changedTouches on touchmove if I was moving two fingers quickly at the same time. I assumed this was expected, and if it is, we should avoid ignoring valid touches.

> @@ +972,5 @@
> > +// Helper function to iterate through a touch event's
> > +// changedTouches array. For each touch, it calls a callback
> > +// function with the touch and touchId as arguments.
> > +function handleTouches(evt, callback) {
> > +  for (var i = 0; i < evt.changedTouches.length; i++) {
> 
> I suppose you could also workaround 822558 here by checking evt.type ===
> 'touchend'

Does the bug only affect touchend? If so, I don't need to worry about messing with touchmove like I mentioned above.

Comment 38

[David Flanagan [:djf]]

(In reply to Margaret Leibovic [:margaret] from comment #37)
> (In reply to David Flanagan [:djf] from comment #36)

> Okay, I'll update the patch and land it tomorrow morning. I just have a few
> questions...
> 
> > @@ +940,5 @@
> > > +      return;
> > > +
> > > +    // Update our cached x/y values.
> > > +    touchedKeys[touchId].x = touch.pageX;
> > > +    touchedKeys[touchId].y = touch.pageY;
> > 
> > I don't think you should update these.  With the code this way, if you moved
> > your finger very slowly, you'd never cross the threshold and the move would
> > never be registered. I was thinking of an absolute distance threshold, not a
> > move velocity threshold.  (And 5 pixels was just a wild guess...)
> 
> I think you would cross the threshold... they're only updated if we're
> actually going ahead with the movePress.

Sorry; you're right.

 For example, let's say you get a
> touchstart at (0,0) and move your finger horizonally. When you move to (1,0)
> or (4,0) we return and don't update the cache, but if you move to (5,0), we
> update the cache to now be (5,0) and call movePress(). Then, on the next
> touchmove, we won't update the cache and move again until you move to
> (10,0), but we will update the cache call movePress() eventually, even if
> you're dragging your finger really slowly.

Most of the time during typing there will be no intentional move, so it is that first threshold that matters most. Once we've established that the user is actually moving their finger, we assume they're doing it on purpose. So in that case, I'm fine updating to follow their finger as closely as we can.  So I slightly favor not updating the cached values.  But if it works well as is I don't feel strongly about it.


> 
> With my testing 5px doesn't seem to feel any different from no threshold, so
> I think this is right (we can always tweak this number if people complain).
> 
> > @@ +955,5 @@
> > > +  evt.preventDefault();
> > > +  touchCount = evt.touches.length;
> > > +
> > > +  handleTouches(evt, function handleTouchEnd(touch, touchId) {
> > > +    // Remove the event listeners from the original target.
> > 
> > Because of bug 822558, you might just want to handle the first touch in
> > changedTouches manually instead of using your handleTouches function.
> 
> When I started working on this patch, I did notice getting two touches in
> changedTouches on touchmove if I was moving two fingers quickly at the same
> time. I assumed this was expected, and if it is, we should avoid ignoring
> valid touches.

Right, but this comment was on the handleTouchEnd() function. Don't change the touchstart or touchmove handlers.

> 
> > @@ +972,5 @@
> > > +// Helper function to iterate through a touch event's
> > > +// changedTouches array. For each touch, it calls a callback
> > > +// function with the touch and touchId as arguments.
> > > +function handleTouches(evt, callback) {
> > > +  for (var i = 0; i < evt.changedTouches.length; i++) {
> > 
> > I suppose you could also workaround 822558 here by checking evt.type ===
> > 'touchend'
> 
> Does the bug only affect touchend? If so, I don't need to worry about
> messing with touchmove like I mentioned above.

As far as I know it only affects touchend.

Comment 39

[:Margaret Leibovic]

(In reply to David Flanagan [:djf] from comment #38)

> Most of the time during typing there will be no intentional move, so it is
> that first threshold that matters most. Once we've established that the user
> is actually moving their finger, we assume they're doing it on purpose. So
> in that case, I'm fine updating to follow their finger as closely as we can.
> So I slightly favor not updating the cached values.  But if it works well as
> is I don't feel strongly about it.

This makes sense to me, but if we don't update the cache, we'd need to change the logic to do something to allow the user to move their finger back to the original key if they want. But yeah, it's working well as it is, so I think it's okay for now.

> > 
> > With my testing 5px doesn't seem to feel any different from no threshold, so
> > I think this is right (we can always tweak this number if people complain).
> > 
> > > @@ +955,5 @@
> > > > +  evt.preventDefault();
> > > > +  touchCount = evt.touches.length;
> > > > +
> > > > +  handleTouches(evt, function handleTouchEnd(touch, touchId) {
> > > > +    // Remove the event listeners from the original target.
> > > 
> > > Because of bug 822558, you might just want to handle the first touch in
> > > changedTouches manually instead of using your handleTouches function.
> > 
> > When I started working on this patch, I did notice getting two touches in
> > changedTouches on touchmove if I was moving two fingers quickly at the same
> > time. I assumed this was expected, and if it is, we should avoid ignoring
> > valid touches.
> 
> Right, but this comment was on the handleTouchEnd() function. Don't change
> the touchstart or touchmove handlers.

Okay, I failed to notice where this comment was. It was late for me :)

> > 
> > > @@ +972,5 @@
> > > > +// Helper function to iterate through a touch event's
> > > > +// changedTouches array. For each touch, it calls a callback
> > > > +// function with the touch and touchId as arguments.
> > > > +function handleTouches(evt, callback) {
> > > > +  for (var i = 0; i < evt.changedTouches.length; i++) {
> > > 
> > > I suppose you could also workaround 822558 here by checking evt.type ===
> > > 'touchend'
> > 
> > Does the bug only affect touchend? If so, I don't need to worry about
> > messing with touchmove like I mentioned above.
> 
> As far as I know it only affects touchend.

Okay, I just updated onTouchEnd's handleTouches callback to bail if there's no touchedKeys[touchId], because that means that we already handled the touchend event for that touch (I put a comment in there saying such).

I'm going to do a little more testing and post a PR soon.

Comment 40

[:Margaret Leibovic]

https://github.com/mozilla-b2g/gaia/commit/020b11fec2321df3c683194e559b65b522b0da85

Comment 41

[Joe Drew (not getting mail)]

squee

Comment 42

[David Flanagan [:djf]]

Dietrich: regarding comment 34, I'm not sure what "rollup of keyboard fixes" you were talking about. This bug was pretty strictly about multitouch for the keyboard.  The bugs mentioned in comment 4 were never part of this.  Just want to make sure that everyone is clear about what this bug was.