814778 - [Gaia::Keyboard] Keyboard tooltips render slowly and sometimes don't render at all

Status: VERIFIED FIXED

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

Description

[Peter La]

What makes it feel slow/broken?

There is a noticeable delay between when a user taps a key on the keyboard and when the large blue tooltip containing that letter is rendered.  Sometimes it doesn't render at all (it feels like 1/4 - 1/3 of the time).  The faster I type, the more often it will not render.

Did it prevent you from doing what you wanted? Why?

This prevents me from being confident in what I have typed.  Sometimes I think I haven't typed a certain character because the tooltip didn't show up.  It also slows down my typing overall due to making more mistakes or just not being sure what I have typed was registered.

The responsiveness needs to improve.

How does this make you feel?

[ ]  :)  I feel happy about it
[ ]  :|  Meh
[ ]  :(  I'm upset
[X] >:O  I'm angry

Device: Unagi, Nov. 22 Nightly.

Details:
There should be no delay at all in this crucial visual feedback to the user on such an often used component of the system.  See videos showing the difference between B2G and our reference platform (Otoro running ICS).

Bonus: can you attach a video of the problem?
Yes

Comment 1

[Peter La]

Attachment: Keyboard tooltip delay - B2G

Comment 2

[Peter La]

Attachment: Keyboard tooltip delay - Android

Comment 3

[Peter La]

Nominating for basecamp-blocking+.

Typing quickly and accurately is a core interaction that must be as performant as possible, and user feedback when typing is an essential component.

Comment 4

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

This is just atrociously bad now, to the point where we need to either fix this or drop the "tooltips" for being too broken.  bb+ based on that.

mattwoodrow/gal, one of you guys want to poke?  Turn on paint flashing and prepare for a big unpleasant surprise! ;)

Comment 5

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

I should add that perf has regressed twice in the last couple of weeks.  I suspect the more recent regression is the switch to using a mozpasspointerevents iframe for the keyboard.

Comment 6

[Andreas Gal :gal]

Clint, I can't begin to explain how extremely urgently we need this automated to measure performance and avoid regressions. When can this be stood up? We really, really, desperately need this. Can your team help?

Comment 7

[Andreas Gal :gal]

Two bugs here. Please file a separate bug with STR if tool tips don't show up. This bug will be about performance only.

Comment 8

[Matt Woodrow (:mattwoodrow)]

We are still using -moz-element for the keyboard.

Invalidation log and stack trace to what caused the biggest invalidation:

http://pastebin.mozilla.org/1964898

Lines 2-27 are the tooltip being added, these look like reasonable invalidations.
28-31 are the changed text as a result of typing.

32-32 is us detecting that the -moz-element content of the background (keyboard) has changed, and invalidate all of it. The stack trace for the bottom is how we detected this change.

Lines 38-786 are painting the keyboard.

Comment 9

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

(In reply to Andreas Gal :gal from comment #7)
> Two bugs here. Please file a separate bug with STR if tool tips don't show
> up. This bug will be about performance only.

The tool tips do show up once in a while.  We're just repainting so slowly that they're not consistently appearing.

Comment 10

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> We are still using -moz-element for the keyboard.

Ohnoes, I thought Vivien landed a patch to do away with that.

Comment 11

[Kevin Grandon :kgrandon]

I was just looking at a few random bugs and noticed that in B2G desktop it appears to only repaint a small area around each key, but in the phone it will repaint the entire keyboard. Just noting that in case it's helpful!

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

I started bisecting this. Reverting Gecko to 21e653481845 is *so much* better. And it only repaints a small area around the tooltip.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

This is caused by bug 803908.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Settings font.size.inflation.minTwips to 0 makes the keyboard usable again.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Assigning to cjones because he should know best how to fix that.

Comment 16

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

WTF!  Nice find.

Comment 17

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> > We are still using -moz-element for the keyboard.
> 
> Ohnoes, I thought Vivien landed a patch to do away with that.

I reverted it because the mozpasspointerevents changes did not make it on beta. I need to reland that :/

Comment 18

[Andreas Gal :gal]

We have our own b2g branch now. Vivien, who is landing mozpasspointerevents?

Comment 19

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

Attachment: Disable font inflation for the b2g master process

Fixes the unfortunate thinko and gets us back to previous levels of slow.  We want to take this patch for b2g regardless of the regression because we always font inflation disabled for master-process UI.

I owe filing a followup to diagnose why font inflation hurts the keyboard performance so badly.

Comment 20

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

After this patch, I still see a relatively unresponsive keyboard in the SMS app, however I suspect that's in large part due to bug 812987.

That said, we still have room to improve.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 686205 [details] [diff] [review]
Disable font inflation for the b2g master process

r=dbaron, though I'd probably prefer the naming as "disabled in master process", which I think could be accomplished with an s/isabledMasterProcess/isabledInMasterProcess/g on the patch file.

Comment 22

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

(In reply to David Baron [:dbaron] from comment #21)
> Comment on attachment 686205 [details] [diff] [review]
> Disable font inflation for the b2g master process
> 
> r=dbaron, though I'd probably prefer the naming as "disabled in master
> process", which I think could be accomplished with an
> s/isabledMasterProcess/isabledInMasterProcess/g on the patch file.

Done.

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/134f3f1f9651

Comment 24

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

Let's file a new bug to rerun the performance analysis after this change merges around and consider further fixes in followups.

Comment 25

[cmtalbert]

Andreas, Ted has the app startup and FPS measurements nearly ready and we should be able to deploy them by the end of the week; however, I'm at a loss as to how that would have helped capture this specific regression.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for beta

The original patch doesn't apply on beta, here's one that does.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for beta

Oops, forgot to include the header. Here's the correct version.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 686205 [details] [diff] [review]
Disable font inflation for the b2g master process

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 803908
User impact if declined: *Very* slow keyboad, mostly unusable.
Testing completed (on m-c, etc.): Manual tests look good.
Risk to taking this patch (and alternatives if risky): Low risk, introduces a new pref that disables the feature introduced by bug 803908 for the master process.
String or UUID changes made by this patch: None.

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 686205 [details] [diff] [review]
Disable font inflation for the b2g master process

It's bb+, we don't need to ask for approval.

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a1e780287d4b
https://hg.mozilla.org/releases/mozilla-aurora/rev/f1c2a3d8d38c

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Andreas Gal :gal from comment #18)
> We have our own b2g branch now. Vivien, who is landing mozpasspointerevents?

This is now on all branches. The keyboard/iframe fix is back on gaia-master.

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/134f3f1f9651

Comment 33

[sarsenyev]

Reviewed and verified on "Unagi" device
BuildId:201301303070201
 No delay between when a user taps a key on the keyboard and when the large blue tooltip containing that letter is rendered.