Closed Bug 875963 Opened 11 years ago Closed 10 years ago

[keyboard] switch case without rebuilding the keyboard

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Keyboard
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed, b2g-v1.4 fixed)

Status:
VERIFIED FIXED
Milestone:
1.3 C2/1.4 S2(17jan)
Project Flags:
blocking-b2g 1.3T+
Tracking Flags:
Tracking Status
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: djf, Assigned: janjongboom)

References

Details

(Keywords: perf, Whiteboard: [c=effect p= s=2014.01.17 u=])

Attachments

(1 file)

Prototype
46 bytes, text/x-github-pull-request
rudyl
: review+
rudyl
: feedback+
Details | Review 
Bug 860318 changed the keyboard visuals so that keycaps were always in upper case regardless of the state of the shift key. This was done to improve performance.

But the setUpperCase() function in keyboard.js is still calling the draw() function in render.js every time case changes. And that draw() function completely rebuilds the keyboard (recreating all the HTML elements) every time it is called.

The visual appearance of the keyboard no longer changes, but we're still doing all the work we used to do. So the performance justification for switching to all caps all the time turns out to be completely bogus.

I'm filing this bug to improve performance by adding a new function to render.js that is responsible for just changing the case of the keyboard and doing so without rebuilding anything.  If we do this, we can probably restore the uppercase/lowercase keycaps and still have a performance gain over what we have today.
Blocks: 860318
Rudy: it looks like you did most of the work on bug 860318. Do you want to take this one as a follow up?
Flags: needinfo?(rlu)
Hi David,

Yeah, I can look into this but the priority would be lower than tef/leo+ bugs that I have in my plate.

Thanks for the filing this bug.
--
I did not handle this in bug 860318 because it involved changing the logic which are currently using "data-keycode" in each key to output the correct character case.
Flags: needinfo?(rlu)
Assignee: nobody → rlu
(In reply to David Flanagan [:djf] from comment #0)
> Bug 860318 changed the keyboard visuals so that keycaps were always in upper
> case regardless of the state of the shift key. This was done to improve
> performance.
> 
> But the setUpperCase() function in keyboard.js is still calling the draw()
> function in render.js every time case changes. And that draw() function
> completely rebuilds the keyboard (recreating all the HTML elements) every
> time it is called.
> 
> The visual appearance of the keyboard no longer changes, but we're still
> doing all the work we used to do. So the performance justification for
> switching to all caps all the time turns out to be completely bogus.

_Perceived_ performance. We hide the delay from user. It was our band aid solution in Madrid.

> I'm filing this bug to improve performance by adding a new function to
> render.js that is responsible for just changing the case of the keyboard and
> doing so without rebuilding anything.  If we do this, we can probably
> restore the uppercase/lowercase keycaps and still have a performance gain
> over what we have today.

It would be fantastic to fix this. Any keyboard perf improvements are hugely welcome!
Agree, it would be really great if this bug got addressed as it's an important performance improvement. 

Stephany, can we do something to raise the priority of this bug?
(In reply to Francis Djabri [:djabber] from comment #4)
> Agree, it would be really great if this bug got addressed as it's an
> important performance improvement. 
> 
> Stephany, can we do something to raise the priority of this bug?

Ideally, so it can get addressed in 1.1 so that users upgrading from 1.0.1 don't see a difference in the uppercase/lowercase behavior of the keyboard.
I will suggest leo? for this in the next triage. 

This fix would make for a significant feature release, as it would improve performance so much that we could launch with upper and lower case. 

To be clear: fixing this would mean that there would be NO change in feature sets between 1.0.1 and 1.1. If we do not fix this, 1.1 will continue to have upper case only, as it does in the nightly today. If we DO fix this, there will be no keyboard difference (and consequent downgrade in behavior) between 1.0.1 and 1.1 for the user.

Previously, performance was so poor that we thought 1.1 would have to ship with only the upper case keyboard, but if we fix this bug that is not the case.
blocking-b2g: --- → leo?
I am a little confused here, while bug 860318 has not been uplifted to v1-train, we still have "upper/lower" case switching for our keyboard on v1-train.

However, I also heard from Tim and Neo that we would like to have "All uppercase" style for v1.1.

Can someone clarify the requirement here, Neo?
Thank you.
Flags: needinfo?(nhsieh)
Rudy, I'm re-flagging Francis for timezone purposes. :)

Tim and Neo can also weigh in but, from what I understand, we only wanted "all uppercase" for 1.1 because there would be performance issues otherwise. All uppercase is not an ideal experience for the user, so this was more a workaround to deal with performance issues.
Flags: needinfo?(fdjabri)
Hi Rudy, 

Let me just echo what Stephany said. We wanted to go to all uppercase because we wanted to hide the delay of switching between uppercase and lowercase from the user. However, if I understand correctly, fixing this bug would dramatically reduce that delay, and so  switching between lowercase and uppercase would no longer be an issue.
Flags: needinfo?(fdjabri)
Current behavior is theoretically shippable, but we'd rather not. Please let us know when you're done with a fix and risk evaluation, and we'll evaluate final state.
Per discussion with Rudy, this bug should have no impact on v1-train as the all-caps implementation was applied only to master.

removing leo?, please re-nom if our understanding is incorrect.
blocking-b2g: leo? → ---
Keywords: perf
Whiteboard: c= ,
(In reply to Wayne Chang [:wchang] from comment #11)
> Per discussion with Rudy, this bug should have no impact on v1-train as the
> all-caps implementation was applied only to master.

This bug is still valid -- it's about improving the performance when switching cases, not strictly about improving efficiency for the all-caps keyboard.
After reading through this bug's comments and speaking with Dylan this is my understanding:

+ Visually switching the keyboard's character case is the user-perceived performance issue here.

+ To work around the user's perceived performance, the keyboard was updated in gaia master to only display a single character case at all times eliminating the visual transition.

+ David F. filed this bug because although the perceived performance is addressed in gaia master by always showing a single character case, the code still performs the lower<->UPPER case transition non-visually.

+ David F. proposed to add a new function to render.js that performs the case transition without the slow performing full-keyboard rebuild.

+ The UX team supports David's proposal because it's expected to make displaying
both lower and UPPER case keyboard modes perform well.

+ We haven't been provided with any measurements showing the time currently spent rebuilding the keyboard vs. not doing so as David's proposing so it's unclear how much user-perceived performance will be improved.


With that understanding, the outstanding questions to David and Rudy are:

+ Can either of you provide measurements of the time being spent to rebuild the keyboard and the reduction David's render.js change is expected to produce in that time?

+ Can either of you commit to delivering David's proposed change in the leo (1.1) timeframe?


If the answer to either of those questions isn't a clear YES this shouldn't be marked as a [leo+] blocker because we'll have no concrete justification. The path forward at that point would be to either:

+ Uplift the gaia master single-case workaround to v1-train for its user-perceived performance gain.

+ Or ship with the undesired case-transition performance currently in v1-train.
Status: NEW → ASSIGNED
Flags: needinfo?(rlu)
Flags: needinfo?(dflanagan)
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Whiteboard: c= , → [c= ]
Based on the discussions taken place with partner, my comments inline below for your reference on moving forward.

> 
> If the answer to either of those questions isn't a clear YES this shouldn't
> be marked as a [leo+] blocker because we'll have no concrete justification.
> The path forward at that point would be to either:
> 
> + Uplift the gaia master single-case workaround to v1-train for its
> user-perceived performance gain.
> 
This is undesired. Their perspective is what they don't see anything wrong with what is now on v1-train. By introducing this it would be a feature change and one that's not for the better in their perspective.

> + Or ship with the undesired case-transition performance currently in
> v1-train.
This is what they have opt for for now.
Thanks Wayne. I'm nominating this for tracking [tracking-b2g18:?] and clearing Neo's needinfo.

David and Rudy, we still need your response(s) on performance measurements.
tracking-b2g18: --- → ?
Flags: needinfo?(nhsieh)
The call to IMERender.draw() in setUpperCase() in keyboard.js takes between 60 and 100ms.  Eyeballing the timings, I'd say that the average is about 80ms.

These 80ms produce no visible change in the keyboard.  If we just comment out the call to IMERender.draw(), then the shift key no longer works right. So we can't remove all 80ms of computation. But I think a fix could easily reduce this by at least a factor of 4 down to under 20ms.

I can't get you the timing for a fix without actually doing the fix.

Note that because we have different keyboard UX on master and v1-train, fixing this for v1.1 will require more work than fixing it for v1.2.

We should be make sure, at least, that this is on someone's list to fix for 1.2 if we don't fix it for v1.1.
Flags: needinfo?(dflanagan)
Thanks David. Looks like tracking for possible inclusion in koi is the path forward.
Flags: needinfo?(rlu)
You still working on this?
Flags: needinfo?(rlu)
No, please take it if you think this can resolved in your work to refine rendering.
Thanks.
Flags: needinfo?(rlu)
Assignee: rlu → sergi.mansilla
Assignee: sergi.mansilla → janjongboom
Attached file Prototype — 
Hi Rudy, Here is a prototype based on the comments in bug 863662. Creates keyboard on demand and doesn't destruct. Plus some additional perf stuff found in the other issue. Plus tests.
Attachment #809956 - Flags: review?(rlu)
Whiteboard: [c= ] → [c=effect p= s= u=]
blocking-b2g: --- → koi?
Nominating because of the performance impact
Not blocking release (not a functional issue), but you are very welcome to fix it as early as possible so we can safely landed it in v1.2 branch upon approval.
blocking-b2g: koi? → -
Attachment #809956 - Flags: review?(rlu) → review?(dflanagan)
Target Milestone: --- → 1.2 C3(Oct25)
Attachment mime type: text/plain → text/x-github-pull-request
David, when can you review Jan's patch?
Flags: needinfo?(dflanagan)
I'm buried under a bunch of reviews, but this is in my queue!
Flags: needinfo?(dflanagan)
Blocks: 863662
I really want to get this reviewed soon, but have to handle my koi+ stuff first
Should this bug be koi+?
I think so, the other performance stuff for keyboard is also koi+.
blocking-b2g: - → koi?
Oh, this is what Tim said.

> Not blocking release (not a functional issue), but you are very welcome to fix it as early as possible so we can safely landed it in v1.2 branch upon approval.
(In reply to Jan Jongboom [:janjongboom] from comment #27)
> I think so, the other performance stuff for keyboard is also koi+.

What other performance stuff for keyboard is koi+? If you find them I am very happy to -'ing them.

We really need to wrap up keyboard feature work before working on performance here...
blocking-b2g: koi? → -
Which is not koi+ for some reason but has been triaged.
Target Milestone: 1.2 C3(Oct25) → ---
Attachment #809956 - Flags: review?(dflanagan) → review?(rlu)
Blocks: 927324
The reason why I've not started the review because I still don't have a concrete idea about how to measure the actual performance gain this patch could bring us.

I might need to consult some people from performance team to get a clear clue on this, so please stay tuned.
Thanks.
Simple test: put a console.time() around the draw call. Currently is all sync code that blocks for 50 ms. on device.
Jan,

Really sorry for delaying the review for such a long time.
Since the code has conflicted with the recent changes in keyboard app, could you please update the patch?

Corey,

I've seen you made some comments on the pull request, are you willing to continue the review, or I can take care of it?

Thanks to you guys for working on this!
Flags: needinfo?(janjongboom)
Flags: needinfo?(gnarf37)
please take the review rudy!
Flags: needinfo?(gnarf37)
Comment on attachment 809956 [details] [review]
Prototype

I think the idea applied here is good, so I am giving f+.
Please help update the patch, so that I can have a test on the device to verify this won't break anything, including Chinese IME.

Thanks, Jan.
Attachment #809956 - Flags: review?(rlu) → feedback+
Comment on attachment 809956 [details] [review]
Prototype

Rebased patch & updated unit tests. It works fine on latin locales, on Chinese there seems to be something wrong with composition on the device (see the email I sent) but it works fine in FF Nightly.
Attachment #809956 - Flags: review?(rlu)
Flags: needinfo?(janjongboom)
Comment on attachment 809956 [details] [review]
Prototype

Jan,

Thanks for your work, I think we are almost there to get this patch in.
I found an issue then when using Chinese (Pinyin or Zhuyin) input method, if I pressed the "arrow down" button right to the word candidates, it would output some error messages as follows, and the "full candidate panel" won't show up.

==
01-06 16:53:23.749: E/GeckoConsole(2605): [JavaScript Error: "TypeError: candidatePanel is null" {file: "app://keyboard.gaiamobile.org/js/keyboard.js" line: 1438}]
==

Besides, I also comments some nits on the pull request.
Attachment #809956 - Flags: review?(rlu)
Thanks, I'll take a look. Time to brush up my Chinese skills!
Attachment #809956 - Flags: review?(rlu)
Updated PR
Comment on attachment 809956 [details] [review]
Prototype

I found another issue that looks like, 
 1. Launch the keyboard, make sure the globe is not there.
 2. Go to settings to enable more keyboards.
 3. Find the keyboard is kind-of strange, like 'cannot switch the keyboard layout', or something.

If I restarted the phone, then it is ok.

I'm going to test this more, please stay tuned.
Yeah, I saw it as well. It's because the keyboard is cached and we don't render the globe for that matter. We should re-render if we see that the value for mgmt.supportsSwitching() changed.
I updated the PR to also take supportsSwitching into account..
Comment on attachment 809956 [details] [review]
Prototype

r=me.

Jan, thanks for taking care of this.
Leave a minor comment here, https://github.com/mozilla-b2g/gaia/pull/12448/files#r8718271.
Attachment #809956 - Flags: review?(rlu) → review+
Blocks: 957544
Asking for some clarification so that I can report back to the SUMO team - 

Does this patch make the keyboard always show as ALL-CAPS, or was it possible to improve the performance while still keeping the change between lower-case and ALL-CAPS when pressing the shift button?

Also, which version is this targeted for?

Thanks!! =)

- Ralph
Hi Ralph, This is going in to 1.4 but I hope we can land it in 1.3 as well. This patch does not change the looks of the keyboard. So no effect on current ALLCAPS behavior.
Flags: needinfo?(rdaub)
blocking-b2g: - → 1.3?
Needinfo whsu@mozilla.com to remind myself to verify this bug.
Thanks!
Flags: needinfo?(whsu)
Triagers were concerned about the implied regressions (or unrelated changes) in comment 43.  We'd also like to discuss this as an uplift (would get backed out if regressions) and not as a blocker (we'd hold the release to get this right).
blocking-b2g: 1.3? → -
Hi Jan Jongboom,

Thanks for the details on comment 49. Removing myself from needinfo since I'm not sure what I could help with.

Please let me know if there are any questions or comments that relates to SUMO (Support).

Thanks!! =)
Flags: needinfo?(rdaub)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Thanks for your help!
Great! I cannot reproduce this bug(Bug 875963) and bug 958027.
But, I still can reproduce Bug 958035. Maybe it was different root cause.

* Verification Build:
 - Gaia      3a41d9c2c6dce6a5a248495a1847f8eaa703ef83
 - Gecko     http://hg.mozilla.org/mozilla-central/rev/171857e7e334
 - BuildID   20140112040202
 - Version   29.0a1
Status: RESOLVED → VERIFIED
Flags: needinfo?(whsu)
Whiteboard: [c=effect p= s= u=] → [c=effect p= s=2014.01.17 u=]
Depends on: 959175
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Did this land on 1.3 branch yet?
(In reply to Dietrich Ayala (:dietrich) from comment #59)
> Did this land on 1.3 branch yet?

No, not yet, since this got 1.3-.
What kind of impact would this have on keyboard performance in 1.3? We might want to take it on Tarako branch if it would be useful to do so.
Rendering keyboard (which happens on every show, language switch and ucase/lcase switch) went down from 100 ms. blocking code to 35 ms. with this patch on Keon device.

https://bugzilla.mozilla.org/show_bug.cgi?id=875963#c55 was open to uplifting this patch to 1.3 when it was done.

If someone wants to do a profile run of this patch on Tarako then I would be happy (I don't have a device).
Flags: needinfo?(dietrich)
I think we need this for tarako 1.3t.  It helps with the significant pause when you first try to use the keyboard in an app.
blocking-b2g: - → 1.3T?
Joe, if its not too late for the demo, I think we should get this 1.3T+ and uplifted.  Also, bug 972276.  These both helped with the initial keyboard pause on my tarako device testing.
Flags: needinfo?(jcheng)
Jan and Ben, what's the level of risk in this patch? How much of this code or this particular change is covered by regression tests?
Flags: needinfo?(dietrich)
traige: 1.3T+ to get this into tarako
blocking-b2g: 1.3T? → 1.3T+
status-b2g-v1.3T: --- → affected
Flags: needinfo?(jcheng)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: