796408 - Display key pop-ups within 140ms of touch start

Status: RESOLVED FIXED

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

Description

[GH to BZ]

[GitHub issue by dhylands on 2012-07-20T08:09:13Z, https://github.com/mozilla-b2g/gaia/issues/2655]
While typing on the keyboard, sometimes it doesn't show the blue highlight to indicate which key you type.

This isn't too bad for normal text fields, but its really annoying when trying to enter a password, since you can't tell what you really tapped.

Comment 1

[GH to BZ]

[GitHub comment by lodr on 2012-07-20T08:37:37Z]
It may sound ridiculous but may are you typing so fast? We are studying to keep the highlight few milliseconds to reinforce the feedback, I keep this issue in mind.

Comment 2

[GH to BZ]

[GitHub comment by vingtetun on 2012-07-20T13:11:49Z]
On 20/07/2012 10:09, Dave Hylands wrote:
> While typing on the keyboard, sometimes it doesn't show the blue highlight to indicate which key you type.
>
> This isn't too bad for normal text fields, but its really annoying when trying to enter a password, since you can't tell what you really tapped.

On the password field you have a feedback of the key you type. Is is not 
enough?

Comment 3

[GH to BZ]

[GitHub comment by dhylands on 2012-07-20T15:39:36Z]
I was expecting to see that (and I see if I go into the UI tester, that I do see it).

I think where I didn't see it was when I logged into Persona for getting into the Dev Marketplace. I can't figure out how to log out of Persona, so I can try logging back in.

Comment 4

[GH to BZ]

[GitHub comment by nhirata on 2012-08-08T20:34:32Z]
I agree w/ @dhylands .  I just looked at this, and I can type faster with 2 thumbs and not get the feedback.

Verified:
Otoro phone, build 2012-08-08
Taken from default.xml in b2g-distro: 
* "platform_build" revision= 9d84ba558d495cb515d38bd1b4327b7bd3577854
* "gaia" revision= 8487a6683c77d28a1120342f9c3fc31be6bf63e4
* "mozilla-central" revision= 0ec22da9cb6cf5a2c2953370a4c915cae7fd5ceb
* "gonk-misc" revision= 773ceab8795053694453841ac2c5dca2d039ffa6

Comment 5

[GH to BZ]

[GitHub comment by vingtetun on 2012-08-16T20:18:03Z]
If you type with 2 fingers that make sense! the keyboard application does not support touch events right now, only mouse events :/

Assigning @lodr

Comment 6

[GH to BZ]

[GitHub comment by dhylands on 2012-08-17T18:18:33Z]
I typically only type with one finger on my otoro, and I noticed the problem again while typing in a wifi password.

I was deliberately going quite slow, at least one second between each keypress, and there was one letter in my password which got typed but didn't show the blue feedback.

Comment 7

[Alex Keybl [:akeybl]]

P1 until this is demonstrated to not be an issue on pre-production phones.

Comment 8

[Alex Keybl [:akeybl]]

QA - can you confirm whether or not this is still occurring?

Comment 9

[Alex Keybl [:akeybl]]

Milestoning for C2 (deadline of 12/10), as this meets the criteria of "known bugs with LOE:M".

Comment 10

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

This still does occur for me but not as bad as it used to be.  add info to dhylands to confirm.

This is on 11/13 build.

Comment 11

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

To note, all the characters do appear now; it's more that the keys sometimes has a delay in terms of what is hit.  (feedback)

Comment 12

[Dave Hylands [:dhylands]]

(In reply to Naoki Hirata :nhirata from comment #10)
> This still does occur for me but not as bad as it used to be.  add info to
> dhylands to confirm.
> 
> This is on 11/13 build.

I haven't noticed this lately. The spaceebar doesn't give any feedback, but all of the other keys seem to be working.

Comment 13

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I think this issue depends on Bug 808724 to improve the performance of Keyboard by removing the keyboard overlay.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Rudy Lu [:rudyl] from comment #13)
> I think this issue depends on Bug 808724 to improve the performance of
> Keyboard by removing the keyboard overlay.

I landed it now but it also seems like there is a regressionb on the keyboard. Every time I type on a key all the keyboard is repainted. I wonder if this is a gecko regression :/

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Tim, I'm redirecting that to you since it feels like you are looking at some of the performance issue there.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The performance of the keyboard itself is quite good.  The perf problems we see from it seem to be app-specific issues and small jank from word suggestions.

So I think this is a meta bug and we should track specific issues in their own bugs.  Please re-nom if there's a specific work item here.

Comment 18

[Josh Carpenter [:jcarpenter]]

On 12/5 build this is wildy slow again. Regression? Tested in Browser, Contacts and SMS. In each case:

* Key strikes were sometimes missed
* Key press highlight drew long after my finger had left the key
* Frustration level is at maximum. Current build is essentially unusable. Far below acceptable performance standards.

WRT to the second item, the key highlights must appear as soon as I touch, not after my finger has left the key. Having them pop up once I'm already on the next letter (or the one after that) totally eliminates their usefulness, and actually makes them a distraction.

Gut check time. If we can't make this performant, we should pull the highlights entirely and do something less ambitious (swap color on button, for example).

Renoming. I realize this is a meta bug, but I don't see any dependents.

Comment 19

[David Scravaglieri [:scravag]]

Renominate if reproducible

Comment 20

[Josh Carpenter [:jcarpenter]]

I am still seeing this issue in 12/12 builds, and on the latest work from Margaret. Per my last comment, we need to either improve the performance of the key strikes to be reliably instantaneous, or we have to remove them and come up with another solution. Delayed highlights are distracting, making them worse than none at all.

Renoming. This is a serious quality issue with a vital input component. We need to ensure devs are focusing energies on addressing (and of course I'll work with them closely to provide feedback, etc)

Comment 21

[Tony Chung [:tchung]]

I agree we should try to fix this in bb+, p3.   certain apps are slower for the keys to respond (eg. adding a name in Contact app, typing in a URL in the browser).   It deteriorates over time.  we need to fix this, as the keyboard experience is really laggy -- especially if you make a typo and need to delete!

Comment 22

[Lucas Adamski [:ladamski]]

This sucks but is it really a stop-ship issue?  Keyboard lag is not that uncommon on mobile devices.

Comment 23

[Dietrich Ayala (:dietrich)]

Driver retriage: Two phones, two totally different levels of performance. Unclear so far.

Comment 24

[:Margaret Leibovic]

I just landed bug 806540, which changed our keyboard event handling, so I would encourage you to test again with the lastest gaia.

I know that Josh mentioned in comment 20 that he was still seeing lagginess with my patch applied, but I think the issue he was describing is the key highlights lagging behind the user. This seems different than the issue Tony described in comment 21, which sounds like it's about the text actually being slow to enter. I've seen the lagging highlight, but I don't think I've seen the text lag (that seems like the worse problem).

On quick look, I'm worried that the slow highlighting may be hard to fix. Maybe Gordon can take a look at the CSS to see if we're doing anything dumb.

Comment 25

[Kevin Grandon :kgrandon]

I'll also spend some time on this one. I've create a sample prototype keyboard, and it appears to perform much better than the current one. I have some long term ideas - but want to see if I can come up with any short-term solutions.

The main issue I see right now:  On key tap, we re-paint the entire keyboard, likely due to some javascript/styles. I think we should be able to only repaint a small area around each key instead. I'll see if I can come up with any quick solutions for this as well.

Comment 26

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Kevin,

I did not see that an key tap would cause the repaint of the entire keyboard, with latest Gaia + Gecko.
That happened before but should have been fixed now.

I am going to look into this with some CSS tweaks.
Please let me know if you have any suggestions.

Comment 27

[:Margaret Leibovic]

(In reply to Rudy Lu [:rudyl] from comment #26)

> I did not see that an key tap would cause the repaint of the entire
> keyboard, with latest Gaia + Gecko.
> That happened before but should have been fixed now.

Is the repainting a recent regression? I didn't think I was seeing repainting when testing bug 806540, but I'm worried I missed something and may have caused it with our document.elementFromPoint calls (which unfortunately cause reflows).

Comment 28

[Kevin Grandon :kgrandon]

Apologies, flashing the latest B2G to my phone appears to resolve the repaint issue.

Comment 29

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Going to do profiling on this performance issue to see if anything we could improve.

Comment 30

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: profiling data for key highlighting

Please refer to the attachment for the profiling data.
Or see it here,
http://people.mozilla.org/~bgirard/cleopatra/#report=69f85c04caed0e13eb8720e2085773f148901603

I might need someone to help interpret the data so that I can follow up to do some improvements. 
Thank you.

Comment 31

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Sorry, forgot to mention that what I did in the profiling is:
 - Repeatedly click the keys on the keyboards
 - Can see the highlighted effect most of the time

Test cases not covered,
 - press the key and move to other keys

Comment 32

[:Margaret Leibovic]

(In reply to Rudy Lu [:rudyl] from comment #30)

> I might need someone to help interpret the data so that I can follow up to
> do some improvements. 

Have you used the cleopatra UI before? If not, I wonder if there is a demo video somewhere (and if there isn't, that would be cool for someone to make).

Honing in on the areas where there are sync reflows, I'm seeing that we're causing a sync reflow in getOffset(). I'm wondering if our calls to el.offset* and/or el.scroll* are triggering these reflows. It could be worth experimenting to see what happens if we get rid of those lines.

I'm also seeing that calls to ContactService.jsm and PermissionPromptHelper.jsm are taking a decent bit of time, and happening right before GC (maybe forcing it?), but that's not relevant to the keyboard :)

Comment 33

[:Margaret Leibovic]

Attachment: test patch to remove offset

I decided to try playing around with removing the offset stuff, and I found that got rid of a bunch of sync reflows. Here's the profile:
http://people.mozilla.com/~bgirard/cleopatra/#report=1331aac2e13ed88f6927203b58e52a758734f19c

Right now I don't think our IME is even using the offset, so maybe we should consider leaving it out for v1 if it's causing performance trouble, but I'm not familiar enough with the history of this to assess the tradeoffs.

The other thing I notice in there is reflows during calls to _findPannable in BrowserElementScrolling.js:
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementScrolling.js#136

I noticed in there that there's a call to getBoundingClientRect(), and I believe that causes reflows, which is unfortunate.

Comment 34

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Margaret Leibovic [:margaret] from comment #32)
> (In reply to Rudy Lu [:rudyl] from comment #30)
> 
> > I might need someone to help interpret the data so that I can follow up to
> > do some improvements. 
> 
> Have you used the cleopatra UI before? If not, I wonder if there is a demo
> video somewhere (and if there isn't, that would be cool for someone to make).
> 
> Honing in on the areas where there are sync reflows, I'm seeing that we're
> causing a sync reflow in getOffset(). I'm wondering if our calls to
> el.offset* and/or el.scroll* are triggering these reflows. It could be worth
> experimenting to see what happens if we get rid of those lines.
> 

Can't we cache those values the first time the key is hit?

Comment 35

[:Margaret Leibovic]

(In reply to Vivien Nicolas (:vingtetun) from comment #34)

> > Honing in on the areas where there are sync reflows, I'm seeing that we're
> > causing a sync reflow in getOffset(). I'm wondering if our calls to
> > el.offset* and/or el.scroll* are triggering these reflows. It could be worth
> > experimenting to see what happens if we get rid of those lines.
> > 
> 
> Can't we cache those values the first time the key is hit?

Right now we're just calling getOffest() when the key is released, since we don't need the offset until then, and the user can drag a finger across multiple keys before releasing. Do you mean caching the value for that key, so that we have it when the user enters that key again? I feel like there are so many different keys that this might not make a noticeable improvement (and we'd need to cache a lot of stuff).

Comment 36

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Dear Margaret,

Thanks for your input and yes, I noticed that getOffset() may cause reflow.
However, removing that function call did not bring much performance improvement from my test.

I also tried to create a hidden "highlighted key" for each key, and then show it (by changing its opacity) and the key is pressed, but still with no luck. :-|

Next, I'll try to see if we can simplify the CSS selector we are using to style the highlighting effect.

Comment 37

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Are you testing in input fields with word suggestions?

Comment 38

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Chris,

No, what I tested is using a timer to repeatedly highlight each key on the keyboard to see the rendering performance.
For my test, the FPS would be about < 20, so I am trying to modify the CSS part to see if we can get any performance gain from that.

Thanks.

Comment 39

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

FYR, this is a profiling for repeatedly highlight each key on the keyboard, which uses requestAnimationFrame to apply .highlighted class to the key, no word suggestion/key press handling is involved.

--
profile for the highlighting:
http://people.mozilla.org/~bgirard/cleopatra/#report=c8a7a40bf0d8582da4506c72a18ea0cc7fe40bb0

Full profile:
http://people.mozilla.org/~bgirard/cleopatra/#report=d6065fddd7a5146b645e5a7472c2386114d4e45c

Comment 40

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

Josh,

I found that this issue is actually referring to not-having milti-touch support:

This bug is filed by BH to BZ from
https://github.com/mozilla-b2g/gaia/issues/2655
which is a dup of
https://github.com/mozilla-b2g/gaia/issues/1908
that I asked Tony to file 6 months ago.

Multi-touch have since landed in bug 806540. I am not sure your comment 20 on "Margaret's work" is referring to that or not.

Would you verify the keyboard performance again and see if it is acceptable for v1? Rudy had spent a week on profiling keyboard performance and have identify something that *might* improve the performance, but I really wish we could release the eng effort to somewhere else more urgent. Thanks.

Rudy, would you comment on what you would do if we need to do that?

Comment 41

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

If this issue is about rendering delay of the highlighted key, the only thing that I think could improve that is to use images for the keys of normal/highlighted state.

The current implementation for keys are multiple layers of Dom elements and CSS pseudo-elements (:before/:after) and from my test, using background images to replace that should get us some performance gain.

I just got the assets of the visual design, and plan to give a patch tomorrow.
Thanks.

Comment 42

[Kevin Grandon :kgrandon]

Rudy - just adding a few thoughts.

1 - The background needs to be able to scale as I'm sure you're aware. (Long-tap on the 'e' character for example) - I suppose you could change the approach for a long-tap though.

2 - Maybe it would be worth doing a test where we remove all images, extra dom layers, and fancy CSS properties? I wonder if UX would be willing to make some concessions on the UI if the performance were much better. I love typing on my android 4x devices, and the keyboard is much more simple than ours.

Comment 43

[Josh Carpenter [:jcarpenter]]

(In reply to Kevin Grandon from comment #42)
> 2 - Maybe it would be worth doing a test where we remove all images, extra
> dom layers, and fancy CSS properties? I wonder if UX would be willing to
> make some concessions on the UI if the performance were much better. I love
> typing on my android 4x devices, and the keyboard is much more simple than
> ours.

Absolutely. The look of the popups is far less important than the speed with which they appear. If we need to jettison borders, rounded corners etc, that is totally fine.

Comment 44

[Josh Carpenter [:jcarpenter]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #40)
> Would you verify the keyboard performance again and see if it is acceptable
> for v1? Rudy had spent a week on profiling keyboard performance and have
> identify something that *might* improve the performance, but I really wish
> we could release the eng effort to somewhere else more urgent. Thanks.
> 
> Rudy, would you comment on what you would do if we need to do that?

Hi Tim, based on the 1/4 build I have in my hand, we should still try to improve performance for release. There has been significant improvement, but there are still delays that detract from the efficacy of typing. Let's discuss tomorrow in Berlin.

Comment 45

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

Update:

Rudy and I have just talked to Caesy. He thinks that the visual responsiveness of the keyboard is faster enough, and we shouldn't spend any more time on that. What we should look into is other bugs that cause problems on keyboards. He will be filing bugs on that once there are specific steps or he think it should lock.

Marking this bug as blocking---.

Comment 46

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

(In reply to Rudy Lu [:rudyl] from comment #39)
> FYR, this is a profiling for repeatedly highlight each key on the keyboard,
> which uses requestAnimationFrame to apply .highlighted class to the key, no
> word suggestion/key press handling is involved.
> 
> --
> profile for the highlighting:
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=c8a7a40bf0d8582da4506c72a18ea0cc7fe40bb0
> 
> Full profile:
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=d6065fddd7a5146b645e5a7472c2386114d4e45c

I revisit this profile and I found that we spent 77% of the time painting. So it's obvious to make the highlight show even faster after v1, we should invest if there are super fast way to draw them.

Comment 47

[:Margaret Leibovic]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #46)

> I revisit this profile and I found that we spent 77% of the time painting.
> So it's obvious to make the highlight show even faster after v1, we should
> invest if there are super fast way to draw them.

Are we just painting the new key, or are we repainting the whole keyboard? There could be definite perf gains if we're painting more than we need to be.

Comment 48

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi, from my test, we are just painting the new key when doing the highlighting.

Comment 49

[:Margaret Leibovic]

(In reply to Rudy Lu [:rudyl] from comment #48)
> Hi, from my test, we are just painting the new key when doing the
> highlighting.

Thanks, that's good to know. Maybe we should get some platform people to look at these profiles to see if there's something to improve there. I imagine games might run into similar perf problems.

(jrmuizel is cc'd here, maybe he'd be interested in taking a look if he has the time :)

Comment 50

[Josh Carpenter [:jcarpenter]]

With keyboard quality resurfacing as a major pre-release priority, I want to revisit this bug. We've made big improvements since this bug was created (October), but we still lag the competition.

Please refer to the following video:

https://www.dropbox.com/s/0785zq5ijfznug8/FFOS_KeyboardCompAnalysis_20130331.zip

That was shot on a 240 fps-capable camera on March 31. In particular please watch the "Stress Test" 120 fps segment at the end. At high speeds with multiple fingers the FFOS stops rendering previews completely, while Android manages to keep up (albeit on superior Nexus S hardware). Even more seriously, FFOS stops printing selected characters to the text field until input activity has stopped and all previews have disappeared. I've created bug # for that issue.

To set the goal:

* Consistency: We don't need to draw _every_ preview. The competition misses some too, at high speeds. But we should be doing _better. Especially in multitouch high-speed scenarios.
* Speed: We should target a maximum 140ms delay between touchstart (or whatever we use) and display of the preview. 

Please also note my previous comment:

(In reply to Josh Carpenter [:jcarpenter] from comment #43)
> (In reply to Kevin Grandon from comment #42)
> > 2 - Maybe it would be worth doing a test where we remove all images, extra
> > dom layers, and fancy CSS properties? I wonder if UX would be willing to
> > make some concessions on the UI if the performance were much better. I love
> > typing on my android 4x devices, and the keyboard is much more simple than
> > ours.
> 
> Absolutely. The look of the popups is far less important than the speed with
> which they appear. If we need to jettison borders, rounded corners etc, that
> is totally fine.

Per Kevin, it would be great to create a build that stripped the visuals down to the bare bones and see if we see performance benefits.

Comment 51

[Josh Carpenter [:jcarpenter]]

Kevin, did you have a chance to work on this further?

Comment 52

[Kevin Grandon :kgrandon]

I went through and quickly stripped out all fancy styles, and here is the current branch: https://github.com/KevinGrandon/gaia/compare/bug_796408_keyboard_perf

I don't really have a camera to make the same kind of video - Josh, maybe we could test out the changes under the same conditions/camera?

Comment 53

[Josh Carpenter [:jcarpenter]]

Thanks Kevin. I'll see if I can do that Thursday / Friday before I head to Europe, and report back.

Comment 54

[Josh Carpenter [:jcarpenter]]

Updating bug title to reflect the new performance target (per my comment #50): we want to display key popups within 140ms of a key touch start event. This is per cogsci guidelines for responsiveness in interactive products. Feel free to contact UX for a deck we've put together on the topic. The brain's perception of time elapsed and cause/effect is actually a very interesting topic. :)

Comment 55

[Mike Lee [:mlee]]

Josh, were you able to make a new video with Kevin's CSS changes?

Jed will synchup with you at the Madrid work-week. He's currently measuring the perf impact of these changes at a the api level.

Comment 56

[Josh Carpenter [:jcarpenter]]

Hi Michael, we're discussing in Madrid and targeting to finalize improvements via markup revisions later this week.

Comment 57

[Walter Chen[:ypwalter][:wachen]]

Hi, how's the progress on this bug after 2013/4/14? It has passed later that week for a long time I guess.

Comment 58

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Walter Chen from comment #57)
> Hi, how's the progress on this bug after 2013/4/14? It has passed later that
> week for a long time I guess.

The time from touch start to the key popup being painted does appear to be less than 140ms, and usually significantly less, even without the style changes from bug 860318.