Closed
Bug 875963
Opened 12 years ago
Closed 11 years ago
[keyboard] switch case without rebuilding the keyboard
Categories
(Firefox OS Graveyard :: Gaia::Keyboard, defect)
Tracking
(blocking-b2g:1.3T+, 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)
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.
Reporter | ||
Comment 1•12 years ago
|
||
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)
Comment 2•12 years ago
|
||
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)
Updated•12 years ago
|
Assignee: nobody → rlu
Comment 3•12 years ago
|
||
(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!
Comment 4•12 years ago
|
||
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?
Comment 5•12 years ago
|
||
(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.
Comment 6•12 years ago
|
||
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?
Comment 7•12 years ago
|
||
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)
Comment 8•12 years ago
|
||
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)
Comment 9•12 years ago
|
||
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)
Comment 10•12 years ago
|
||
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.
Comment 11•12 years ago
|
||
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? → ---
Comment 12•11 years ago
|
||
(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.
Comment 13•11 years ago
|
||
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= ]
Comment 14•11 years ago
|
||
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.
Comment 15•11 years ago
|
||
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)
Reporter | ||
Comment 16•11 years ago
|
||
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)
Comment 17•11 years ago
|
||
Thanks David. Looks like tracking for possible inclusion in koi is the path forward.
Flags: needinfo?(rlu)
Comment 19•11 years ago
|
||
No, please take it if you think this can resolved in your work to refine rendering.
Thanks.
Flags: needinfo?(rlu)
Assignee | ||
Updated•11 years ago
|
Assignee: rlu → sergi.mansilla
Assignee | ||
Updated•11 years ago
|
Assignee: sergi.mansilla → janjongboom
Assignee | ||
Comment 20•11 years ago
|
||
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)
Updated•11 years ago
|
Whiteboard: [c= ] → [c=effect p= s= u=]
Assignee | ||
Updated•11 years ago
|
blocking-b2g: --- → koi?
Assignee | ||
Comment 21•11 years ago
|
||
Nominating because of the performance impact
Comment 22•11 years ago
|
||
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? → -
Assignee | ||
Updated•11 years ago
|
Attachment #809956 -
Flags: review?(rlu) → review?(dflanagan)
Updated•11 years ago
|
Target Milestone: --- → 1.2 C3(Oct25)
Updated•11 years ago
|
Attachment mime type: text/plain → text/x-github-pull-request
Reporter | ||
Comment 24•11 years ago
|
||
I'm buried under a bunch of reviews, but this is in my queue!
Flags: needinfo?(dflanagan)
Reporter | ||
Comment 25•11 years ago
|
||
I really want to get this reviewed soon, but have to handle my koi+ stuff first
Comment 26•11 years ago
|
||
Should this bug be koi+?
Assignee | ||
Comment 27•11 years ago
|
||
I think so, the other performance stuff for keyboard is also koi+.
blocking-b2g: - → koi?
Assignee | ||
Comment 28•11 years ago
|
||
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.
Comment 29•11 years ago
|
||
(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? → -
Assignee | ||
Comment 30•11 years ago
|
||
F.e. bug 863662.
Assignee | ||
Comment 31•11 years ago
|
||
Which is not koi+ for some reason but has been triaged.
Updated•11 years ago
|
Target Milestone: 1.2 C3(Oct25) → ---
Assignee | ||
Updated•11 years ago
|
Attachment #809956 -
Flags: review?(dflanagan) → review?(rlu)
Comment 32•11 years ago
|
||
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.
Assignee | ||
Comment 33•11 years ago
|
||
Simple test: put a console.time() around the draw call. Currently is all sync code that blocks for 50 ms. on device.
Comment 36•11 years ago
|
||
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)
Comment 38•11 years ago
|
||
Comment on attachment 809956 [details]
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+
Assignee | ||
Comment 39•11 years ago
|
||
Comment on attachment 809956 [details]
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 40•11 years ago
|
||
Comment on attachment 809956 [details]
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)
Assignee | ||
Comment 41•11 years ago
|
||
Thanks, I'll take a look. Time to brush up my Chinese skills!
Assignee | ||
Updated•11 years ago
|
Attachment #809956 -
Flags: review?(rlu)
Assignee | ||
Comment 42•11 years ago
|
||
Updated PR
Comment 43•11 years ago
|
||
Comment on attachment 809956 [details]
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.
Assignee | ||
Comment 44•11 years ago
|
||
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.
Assignee | ||
Comment 45•11 years ago
|
||
I updated the PR to also take supportsSwitching into account..
Comment 46•11 years ago
|
||
Comment on attachment 809956 [details]
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+
Comment 47•11 years ago
|
||
My comment is here,
https://github.com/mozilla-b2g/gaia/pull/12448#discussion_r8718271
Comment 48•11 years ago
|
||
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
Assignee | ||
Comment 49•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(rdaub)
Assignee | ||
Comment 50•11 years ago
|
||
Assignee | ||
Comment 51•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
blocking-b2g: - → 1.3?
Comment 53•11 years ago
|
||
Needinfo whsu@mozilla.com to remind myself to verify this bug.
Thanks!
Flags: needinfo?(whsu)
Comment 55•11 years ago
|
||
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? → -
Comment 56•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 57•11 years ago
|
||
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)
Updated•11 years ago
|
Whiteboard: [c=effect p= s= u=] → [c=effect p= s=2014.01.17 u=]
Updated•11 years ago
|
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Comment 59•11 years ago
|
||
Did this land on 1.3 branch yet?
Comment 60•11 years ago
|
||
(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-.
Comment 61•11 years ago
|
||
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.
Assignee | ||
Comment 62•11 years ago
|
||
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)
Comment 63•11 years ago
|
||
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?
Comment 64•11 years ago
|
||
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)
Comment 65•11 years ago
|
||
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)
Comment 66•11 years ago
|
||
traige: 1.3T+ to get this into tarako
Updated•11 years ago
|
status-b2g-v1.4:
--- → fixed
tracking-b2g18:
? → ---
Comment 67•11 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•