Closed Bug 860318 Opened 7 years ago Closed 7 years ago

Update keyboard visuals to improve performance

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(b2g18? affected, b2g18-v1.0.0 affected, b2g18-v1.0.1 affected)

RESOLVED FIXED
Tracking Status
b2g18 ? affected
b2g18-v1.0.0 --- affected
b2g18-v1.0.1 --- affected

People

(Reporter: jcarpenter, Assigned: rudyl)

References

Details

(Keywords: perf, Whiteboard: visual design, yedo, ux-tracking, [TAIPEI_FND_TRACKING] [c= s=2013.05.31])

Attachments

(7 files)

For the sake of bug #835404, update keyboard visual design to a more minimalist visual design, less reliant on complex borders, gradients, background images, etc.
Whiteboard: visual design, u=user c=keyboard s=ux-most-wanted
(In reply to Andreas Gal :gal from bug 827811 comment #40)
> jld, please make sure to pick up the latest patch I landed on v1-train as
> well as the sendKey patch from mozilla-central. I re-profiled with those and
> the next biggest chunk of time now is spent painting backgrounds. I think we
> are rescaling the background of keys. ARM is known to be insanely slow
> scaling on the CPU. Can you see whether removing the background images makes
> a big difference? And if so, thats a really easy patch to uplift (e.g. we
> can make sure the background images have the right size, or remove them if
> UX approves).

The key backgrounds, at least in portrait mode, seem to be costing us only around 2-3 ms per repaint.  (The CPU times for key-up and key-down seem to be roughly similar; so, multiply these times by 2 for the full key-press contribution.)

The drop shadows on the keys, which are misrendered by bug 826152 in any case, appear to cost something like 5-10 ms.  And the key border decorations (the multi-image background attributes on |... > .visual-wrapper > span|) are more like 15-20 ms.

So, the stock keyboard has mean 50 ms, std dev 8 ms, quartiles 43 47 57.  With flat square unshadowed keys (and no other changes; the rounded corners on the popups remain, though they look a little out of place visually now, and probably aren't free): mean 24 ms, std dev 3 ms, quartiles 22 24 26.
We're seeing 60% keyboard perf improvements with new markup, thanks to Pavel's efforts. Will apply for review and merge to master tomorrow (Friday).
Assignee: nobody → pivanov
Comment on attachment 739468 [details]
patch for Gaia repo

I have added a few comments, mostly about JS. Feel free to reask the r? flag once they have been addressed.
Attachment #739468 - Flags: review?(21)
Comment on attachment 739468 [details]
patch for Gaia repo

Thanks Vivien :)
Attachment #739468 - Flags: review?(21)
Hi Pavel,

I have a local branch here to make some modifications as follows,
https://github.com/RudyLu/gaia/tree/keyboard/new_layout
(commit: https://github.com/RudyLu/gaia/commit/64c96e17b9e4d4860f8f3b3cd2cd07d2fa5170d3)

1. Remove the comma key in the bottom row
2. Switch the position of lang-switching key and the alternative layout key

Please help take a look and incorporate this if needed.
Thanks.
(In reply to Rudy Lu [:rudyl] from comment #7)
> Hi Pavel,
> 
> I have a local branch here to make some modifications as follows,
> https://github.com/RudyLu/gaia/tree/keyboard/new_layout
> (commit:
> https://github.com/RudyLu/gaia/commit/
> 64c96e17b9e4d4860f8f3b3cd2cd07d2fa5170d3)
> 
> 1. Remove the comma key in the bottom row
> 2. Switch the position of lang-switching key and the alternative layout key
> 
> Please help take a look and incorporate this if needed.
> Thanks.

Let's definitely pull these in as well. Rudy or Pavel, can you attach a screenshot and ask Francis Djabri for review?

Francis Djabri <fdjabri@mozilla.com>
Hi Josh, Francis,

Please help take a look at the new layout after integrate Pavel's work and my local changes,
https://www.dropbox.com/sh/rl36znhsr7e19st/LvRzluLMxC

Please note that I did not change the symbol page (the secondary level of keyboard layout) for the comma key.
Could you suggest where we should put that key?

Thanks.
Flags: needinfo?(fdjabri)
Is it expected that the keyboard only displays UPPERCASE letters even when entering lowercase ones? That's pretty broken imho, so I'm backing this out.

https://github.com/mozilla-b2g/gaia/commit/447cdaa4656d6a830f5bd129be0ac0c755fa6153
REOPEN as this has been backout'd.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Fabrice Desré [:fabrice] from comment #11)
> Is it expected that the keyboard only displays UPPERCASE letters even when
> entering lowercase ones? That's pretty broken imho, so I'm backing this out.
> 
> https://github.com/mozilla-b2g/gaia/commit/
> 447cdaa4656d6a830f5bd129be0ac0c755fa6153

I second Fabrice on backing this out, more broken elements I noticed:
1. the alternative chars dialog displays all characters highlighted blue, as if they are all selected
2. the enabled keyboards list (i.e. small globe button if multiple keyboards are enabled) shows all entries in UPPERCASE and selected (i.e. no way to figure out which keyboard is active)
Hi, 

I haven't had the chance to try the new patch, but looking at the layouts I have a few comments. 

1) The keys on the bottom row seem to be 2 pixels out of alignment with the row above. Please find the corrected layouts in the attachment. 

2) NewKeyboardLayout-SymbolPage - the comma key should be between the alpha and space key. Please find the suggested layout in the attachment.
Flags: needinfo?(fdjabri)
(In reply to Fabrice Desré [:fabrice] from comment #11)
> Is it expected that the keyboard only displays UPPERCASE letters even when
> entering lowercase ones? That's pretty broken imho, so I'm backing this out.
> 
> https://github.com/mozilla-b2g/gaia/commit/
> 447cdaa4656d6a830f5bd129be0ac0c755fa6153

This is intentional. For prior art comparison, try iOS vs Android.
(In reply to Mihai Cirlanaru [:mcirlanaru] from comment #13)
> (In reply to Fabrice Desré [:fabrice] from comment #11)
> > Is it expected that the keyboard only displays UPPERCASE letters even when
> > entering lowercase ones? That's pretty broken imho, so I'm backing this out.
> > 
> > https://github.com/mozilla-b2g/gaia/commit/
> > 447cdaa4656d6a830f5bd129be0ac0c755fa6153
> 
> I second Fabrice on backing this out, more broken elements I noticed:
> 1. the alternative chars dialog displays all characters highlighted blue, as
> if they are all selected

Selections still work; styling is just a bit different. It's important that we land this patch for the much-needed performance benefits. We can then revisit visual tweaks where needed in a separate bug. 

> 2. the enabled keyboards list (i.e. small globe button if multiple keyboards
> are enabled) shows all entries in UPPERCASE and selected (i.e. no way to
> figure out which keyboard is active)

Pavel or Fabrice, can you attach a screenshot of this?
Flags: needinfo?(pivanov)
(In reply to Francis Djabri [:djabber] from comment #14)
> Created attachment 743167 [details]
> Comments on new keyboard layout
> 2) NewKeyboardLayout-SymbolPage - the comma key should be between the alpha
> and space key. Please find the suggested layout in the attachment.

Pavel, can you make this quick layout change, or should Sam? Time is of the essence here for 1.1.
(In reply to Josh Carpenter [:jcarpenter] from comment #15)
> (In reply to Fabrice Desré [:fabrice] from comment #11)
> > Is it expected that the keyboard only displays UPPERCASE letters even when
> > entering lowercase ones? That's pretty broken imho, so I'm backing this out.
> > 
> > https://github.com/mozilla-b2g/gaia/commit/
> > 447cdaa4656d6a830f5bd129be0ac0c755fa6153
> 
> This is intentional. For prior art comparison, try iOS vs Android.

That's really annoying when entering passwords. Our "old" way was much better.
Meh, iOS users don't have any problems. You habituate, I guarantee :)
(In reply to Josh Carpenter [:jcarpenter] from comment #16)

for layout changes I think Rudy can help us because he is more familiar than me
Flags: needinfo?(pivanov) → needinfo?(rlu)
Hi Pavel,

Yeah, sure, I'll take a look.
I was wondering how we cowork on this, I'll send a pull request to your branch?
Or just incorporate your changes and send a pull request to Gaia master?

Let me know which way you think is better.
Thank you.
Flags: needinfo?(rlu)
(In reply to Rudy Lu [:rudyl] from comment #21)
I think there is another bug for the keyboard layout? Josh?
Flags: needinfo?(jcarpenter)
(In reply to Pavel Ivanov [:ivanovpavel] from comment #22)
> (In reply to Rudy Lu [:rudyl] from comment #21)
> I think there is another bug for the keyboard layout? Josh?

Hi Pavel, I think you might be referring to the keyboard layouts that I posted in comment 14. Is that correct?
Flags: needinfo?(jcarpenter)
(In reply to Josh Carpenter [:jcarpenter] from comment #16)
> Pavel or Fabrice, can you attach a screenshot of this?

Pavel or Fabrice please attach a screenshot so we can better understand this problem. Better yet, can you get a build to Francis?
Whiteboard: visual design, u=user c=keyboard s=ux-most-wanted → visual design, u=user c=
(In reply to Pavel Ivanov [:ivanovpavel] from comment #22)
> (In reply to Rudy Lu [:rudyl] from comment #21)
> I think there is another bug for the keyboard layout? Josh?

Pavel, 

We established that there was no separate bug associated with the keyboard layout changes. I've created bug #869267 to cover this. 

Francis
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
2 issues have been addressed.
 1. Keyboard layout name will be all capitalized.
 2. The key order in alternative char menu (right-hand side) is not reversed.

WIP here,
https://github.com/RudyLu/gaia/tree/keyboard/Bug860318-layout_improvement_test 

There are still some issues to be addressed,

1. (Comment 13, point 1)
   The alternative chars dialog displays all characters highlighted blue, as if they are all selected.
   I hope the new visual we want could be defined clearly, see below point 2.
   See the attachments for the current changes.

2. The visual highlighting for CapLock key  is now gone.
   (After you press 2 times on Caps lock, it will turn into always Cap status)   
   I cannot change this since the highlighted effect has been changed as stated in #1.
Hi Pavel, Francis,

Could you please give some guidance on the hightlighted effect, as described in  Comment 26?
Thanks.
Flags: needinfo?(pivanov)
Flags: needinfo?(fdjabri)
Eric, can you please attach a minimal highlight design for implementation. Thanks.
Flags: needinfo?(pivanov) → needinfo?(epang)
Pavel, 
Here are PSDs and JPEGs of the updated highlighting styles for
1. Alternates
2. Language Selects
3. Caps locks (1 & 2 press)

Let me know if you have any questions of need detailed specs!
We'll try to push this into v1.1 :) Thanks!
Flags: needinfo?(epang) → needinfo?(pivanov)
tracking-b2g18: --- → ?
Whiteboard: visual design, u=user c= → visual design, yedo, u=user c=
Rudy, Removing my needinfo request as Eric has provided a design for the highlights but please let me know if you need anything else.
Flags: needinfo?(fdjabri)
Whiteboard: visual design, yedo, u=user c= → visual design, yedo, ux-tracking
Whiteboard: visual design, yedo, ux-tracking → visual design, yedo, ux-tracking, [TAIPEI_FND_TRACKING]
Taking this to implement as the finalized design.
Thanks.
Assignee: pivanov → rlu
Comment on attachment 750991 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9845

This patch:
     1. Fix the issue that Keyboard layout name will be all capitalized.
     2. Fix the isuse that the key order in alternative char menu (right-hand side) is not reversed.
     3. Update the visual design

Dear Vivien,

Could you please help review this update?
Thank you.
Attachment #750991 - Flags: review?(21)
Comment on attachment 750991 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9845

Redirect the review request to get this through.

Dear Tim,

Could you please help review this patch?
Thank you.
Attachment #750991 - Flags: review?(21) → review?(timdream)
Attachment #750991 - Flags: review?(timdream) → review+
Tim,

Thanks for the review.

Gaia master:
https://github.com/mozilla-b2g/gaia/commit/2f0704e6c173e6014c36d524fcb43828d9a80c2c
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
I just backed out the fix, since it broke WVGA support.
https://github.com/mozilla-b2g/gaia/commit/3b23dbbac103fa7bcf0d83e295ea6ce101f51dcc

Thanks to Tim for his reminding on this.

The update is on the way, will send the pull request soon.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 752018 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9895

Hi Tim,

This change removes the responsive.css in keyboard app.
Please help review it.

Thank you.
Attachment #752018 - Flags: review?(timdream)
Comment on attachment 752018 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9895

Keyboard uses it's own Javascript logic to size itself, |responsive.css| shouldn't be included in the app.

Rudy and I failed to spot the bug because the rules in responsive.css on makes it work differently between B2G/Desktop and actual device. We should probably file a bug on B2G/Desktop if we eventually want to make it a simulator.

Anyhow, responsive.css will be removed in bug 870318.
Attachment #752018 - Flags: review?(timdream) → review+
Landed to Gaia master:
https://github.com/mozilla-b2g/gaia/commit/e6579f1a2a7d980ae93bfa4ffdcbdf47eac8ae97

Thanks to Tim for the review.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Flags: needinfo?(pivanov)
(In reply to Josh Carpenter [:jcarpenter] from comment #15)
> This is intentional. For prior art comparison, try iOS vs Android.

I don't know what you iPhone users are smoking, but this feels very broken to me. The shift key appears to have no effect. What possible benefit can this have that justifies the mismatch between what the button says and what the button does?

Is there actually any evidence to demonstrate this change to our UI design improves performance or usability other than "that's what the iPhone does, praise the iPhone"?
(In reply to Ben Francis [:benfrancis] from comment #42)
> I don't know what you iPhone users are smoking, but this feels very broken
> to me. The shift key appears to have no effect. What possible benefit can
> this have that justifies the mismatch between what the button says and what
> the button does?

It seems to me that the goal here is something like: "As a user, I want clear visual confirmation of whether the keyboard is shifted, so that I can spell my passwords correctly, and use my language's standard capitalization when communicating with people."

Which might not be shifting all the key caps; I'm waiting on a build so I don't know what the latest version of the keyboard change does, but there may be room for improvement in how the locked shift key is styled.

> Is there actually any evidence to demonstrate this change to our UI design
> improves performance or usability other than "that's what the iPhone does,
> praise the iPhone"?

If I understand correctly, the capitalization change is just a couple of text-transform directives, and independent of the rest of the change.  My understanding/expectation is that changing case like that won't affect performance because we're still using different DOM nodes for the lower-case and upper-case keys (other parts of the change, however, are a clear win), but removing the visual distinction will allow optimizing how it's implemented.
(In reply to Jed Davis [:jld] from comment #43)
> Which might not be shifting all the key caps; I'm waiting on a build so I
> don't know what the latest version of the keyboard change does, but there
> may be room for improvement in how the locked shift key is styled.

In a word, ugh.  I think how this is supposed to work is that the shift key shows the arrow in cyan instead of white (against the normal dark gray background) when it's just latched for the next key, and changes it to a cyan background with dark gray arrow when it's locked (press the key twice).

Except that I can sometimes press the shift key once, and get the subtle arrow color change, but the shift stays on until I press it again.  This seems to depend on whether the last character typed was upper-case.

Also, changing the color of the arrow like that might be too subtle even for its intended purpose, and might be especially bad for people with red/green colorblindness.
Note that bug 810306 covers the case change.
(In reply to Jed Davis [:jld] from comment #44)
> I think how this is supposed to work is that the shift key
> shows the arrow in cyan instead of white (against the normal dark gray
> background) when it's just latched for the next key, and changes it to a
> cyan background with dark gray arrow when it's locked (press the key twice).
> 
> Except that I can sometimes press the shift key once, and get the subtle
> arrow color change, but the shift stays on until I press it again.  This
> seems to depend on whether the last character typed was upper-case.

By design, apparently.  According to gaia/apps/keyboard/js/imes/latin/latin.js, in updateCapitalization:

    // Set the keyboard to uppercase or lowercase depending
    // on the text around the cursor:
    //
    // 1) If the cursor is at the start of the field: uppercase
    //
    // 2) If there are two uppercase chars before the cursor: uppercase
    //
    // 3) If there is a non space character immediately before the cursor:
    //    lowercase
    //
    // 4) If the first non-space character before the cursor is . ? or !:
    //    uppercase
    //
    // 5) Otherwise: lowercase
(In reply to Jed Davis [:jld] from comment #43)

> If I understand correctly, the capitalization change is just a couple of
> text-transform directives, and independent of the rest of the change.  My
> understanding/expectation is that changing case like that won't affect
> performance because we're still using different DOM nodes for the lower-case
> and upper-case keys (other parts of the change, however, are a clear win),
> but removing the visual distinction will allow optimizing how it's
> implemented.

Jed is correct: keyboard.js still calls IMERender.draw() on every case change. (See the setUpperCase() function.) It recreates the entire keyboard, even though there is now no visual change. We should make render.js able to switch case (perhaps using text-transform on the key caps as Jed suggests) without rebuilding the whole thing. That would have a significant impact on performance.
Depends on: 875963
I've filed bug 875963 as a follow up to this one.
David, how do we know which release this is going to make it in to? I'm having trouble discerning that. Thanks for any clarity you can provide.
blocking-b2g: --- → leo?
Triage - do not want this change to take place on leo as the keyboard is fine as is on 1.1.
blocking-b2g: leo? → ---
Stephany,
This was landed in gaia master but hasn't been uplifted for any 1.x release. Wayne's comment on bug 875963 indicates that partners don't want the ALL-CAPS keyboard change: https://bugzilla.mozilla.org/show_bug.cgi?id=875963#c14

Rudy,
The patches submitted for this bug appear to cover more than just switching to all CAPS, can you confirm? The non-CAPS changes may be beneficial for a future release (i.e. 1.2).
Flags: needinfo?(rlu)
Keywords: perf
Priority: -- → P2
Whiteboard: visual design, yedo, ux-tracking, [TAIPEI_FND_TRACKING] → visual design, yedo, ux-tracking, [TAIPEI_FND_TRACKING] [c= s=2013.05.31]
> 
> Rudy,
> The patches submitted for this bug appear to cover more than just switching
> to all CAPS, can you confirm?

Yes, this patch includes 2 parts,
 1. Simplify the CSS styles to improvement keyboard performance.
 2. Make the keyboard all-caps - which affects the (perceived) performance when you switch the case.

> The non-CAPS changes may be beneficial for a
> future release (i.e. 1.2).

Yes, agree. 
Maybe we could open another bug to uplift part. 1 to v1-train?
Flags: needinfo?(rlu)
(In reply to Rudy Lu [:rudyl] from comment #53)
> 
> Yes, this patch includes 2 parts,
>  1. Simplify the CSS styles to improvement keyboard performance.
> 
> ...
>
> Maybe we could open another bug to uplift part. 1 to v1-train?

That makes sense to me. Please go ahead and create that bug and nominate. Is the 60% performance gain Josh mentioned in comment #2 confirmed?

(In reply to Josh Carpenter [:jcarpenter] from comment #2)
> We're seeing 60% keyboard perf improvements with new markup, thanks to
> Pavel's efforts. Will apply for review and merge to master tomorrow (Friday).
No longer depends on: 810306
Duplicate of this bug: 810306
as bug 810306 has been marked as a duplicate of this one, let me start again the debate on the all uppercase keyboard here.

From what I understand, this is a regression due to performance impact. it is *not* a desired feature as some people are saying...

I've had my first smartphone few weeks ago, a GeeksPhone Revolution with FFOS 1.3. and this all uppercase keyboard is really disturbing.
at geeksphone's forum, people thought this was a bug which was going to be solved. but now, we worry... (https://forum.geeksphone.com/index.php?topic=5935.msg63472#msg63472 this is just an example)

to those who say that on computer, the case of the keyboard doesn't change... Yes, there is a little LED which tells you when you are in uppercase or not. but, please! accept that this is not the best indicator... Moreover, on a smartphone, you hide the shift button with your thumb when you are writing. So, the indicator is not so visible, it is really a bad bad choice...

if nobody can agree on this, maybe adding an option could be the best feature :)
> From what I understand, this is a regression due to performance impact. it is *not* a desired feature as some people are saying...

Yes, it is a desired feature. See bug 883340.
in your given discussion:

(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #7)
> This behavior was added as a performance improvement by the patch for bug
> 860318 and bug 810306 details the design decision behind it.

(In reply to Rudy Lu [:rudyl] from comment #8)
> As far as I know, the all-caps change is not only for performance, but also
> a UX design decision.

(In reply to Henrik Skupin (:whimboo) from comment #14)
> Both dependencies have been fixed. So that should indeed be re-evaluated.
> Which performance loss do we still experience?

(In reply to Rudy Lu [:rudyl] from comment #18)
> We have gone through a discussion on this topic during a keyboard workshop,
> and this (keep all keys shown as uppercase) is still the final decision for
> keyboard UX.

(In reply to GoodMike from comment #21)
> Currently most Android device use lower case as default while Apple and
> BlackBerry use uppercase default.

So... even you guys don't really know if it has been from a performance issue or a real (and questionable?) decision.

The multiple bug reports on this, that are scattered on bugzilla, *should* let you think that everybody does not love what Apple does, and *maybe* change the case of the keyboard is more user-friendly.
Moreover, as lots of people have reported before me, this is particularly painful when typing a password. And I let you read again my comment #56

I can understand that some guys don't want to change the case, but you have to understand that some others do want. And I don't understand why this have to be imposed, while a simple option could resolve everything ;)
(In reply to remi.ducceschi from comment #59)

Hi Rémi,

> So... even you guys don't really know if it has been from a performance
> issue or a real (and questionable?) decision.

It was a performance issue, and UX people were very happy to solve it with an "all caps" solution. There has been disagreement on that since day 1, so there's no point in trying to paint the team as a single entity where everyone agrees.

> The multiple bug reports on this, that are scattered on bugzilla, *should*
> let you think that everybody does not love what Apple does, and *maybe*
> change the case of the keyboard is more user-friendly.
> Moreover, as lots of people have reported before me, this is particularly
> painful when typing a password. And I let you read again my comment #56
> 
> I can understand that some guys don't want to change the case, but you have
> to understand that some others do want. And I don't understand why this have
> to be imposed, while a simple option could resolve everything ;)

For the record, I'd like very much the upper/lower case keyboard to come back (I even backed out the upper-only version when it landed initially). But I don't feel adding a "simple" option is a good solution either. That will add code complexity in the keyboard and maintenance burden. With 3rd party keyboards coming in 1.5, you'll have the opportunity to create your own keyboard that does just that.
All caps was also proposed as a temporary work-around for poor performance. Casey, per our talk earlier this week, let's see if we can't get some Review time from you and/or from the Frameworks team on the state of keyboard.
Flags: needinfo?(kyee)
Duplicate of this bug: 1130531
Flags: needinfo?(caseyyee.ca)
You need to log in before you can comment on or make changes to this bug.