Web Component Virtual Keyboard

Web Component Virtual Keyboard

Convert existing keyboard to web component keyboard. Also fix touch down state of keys.

BUG=242539, 240346
R=bryeung@chromium.org, miket@chromium.org, stevet@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207448

[bshe, 2013-05-23 20:28:26]

Hi Bryan.

Could you please take a look at this?
The web component version should behave exactly like what we have right now.
SVGs in the webui version is somehow not displayed and I dont know why. But
since we are going to remove these svgs according to the new mock. So I dont
worry about it for now.

Thanks!

[bryeung, 2013-05-23 21:39:28]

https://codereview.chromium.org/15176004/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/image_loader.cc (right):

https://codereview.chromium.org/15176004/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/image_loader.cc:179: {"keyboard/voice_input.js",
IDR_KEYBOARD_VOICE_INPUT_JS},
alphabetical order please...

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/keyboard_reso...
File ui/keyboard/keyboard_resources.grd (right):

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/keyboard_reso...
ui/keyboard/keyboard_resources.grd:17: <include
name="IDR_KEYBOARD_VOICE_INPUT_JS" file="resources/voice_input.js"
type="BINDATA" />
alphabetical order please...

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/keyboard_ui_c...
File ui/keyboard/keyboard_ui_controller.cc (right):

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/keyboard_ui_c...
ui/keyboard/keyboard_ui_controller.cc:34:
source->AddResourcePath("polymer.min.js", IDR_KEYBOARD_POLYMER);
alphabetical order please

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
File ui/keyboard/resources/elements/kb-keyboard.html (right):

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:97: if (detail.char == "Mic") {
is there any way we can push this out of the general keyDown function?

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:102: if (to) {
can we rename "to" to something a bit more descriptive?

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:138: this.keyset = to;
why are we doing this on up and down?  I must not understand what this does...

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/mai...
File ui/keyboard/resources/main.css (right):

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/mai...
ui/keyboard/resources/main.css:91: background-image: url('images/del.svg');
if the SVGs aren't working, let's just remove them in this CL

[bshe, 2013-05-23 21:46:11]

+sjmiles I could find your chromium account.
Anyway, I converted your prototype to this CL. Could you please take a look to
make sure that I am on the right path? Thanks!

[bshe, 2013-05-24 14:43:49]

PTAL. Thanks!

https://codereview.chromium.org/15176004/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/image_loader.cc (right):

https://codereview.chromium.org/15176004/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/image_loader.cc:179: {"keyboard/voice_input.js",
IDR_KEYBOARD_VOICE_INPUT_JS},
On 2013/05/23 21:39:28, bryeung wrote:
> alphabetical order please...

Done.

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/keyboard_reso...
File ui/keyboard/keyboard_resources.grd (right):

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/keyboard_reso...
ui/keyboard/keyboard_resources.grd:17: <include
name="IDR_KEYBOARD_VOICE_INPUT_JS" file="resources/voice_input.js"
type="BINDATA" />
On 2013/05/23 21:39:28, bryeung wrote:
> alphabetical order please...

Done.

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/keyboard_ui_c...
File ui/keyboard/keyboard_ui_controller.cc (right):

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/keyboard_ui_c...
ui/keyboard/keyboard_ui_controller.cc:34:
source->AddResourcePath("polymer.min.js", IDR_KEYBOARD_POLYMER);
On 2013/05/23 21:39:28, bryeung wrote:
> alphabetical order please

Done.

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
File ui/keyboard/resources/elements/kb-keyboard.html (right):

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:97: if (detail.char == "Mic") {
I imagine this would merge to the "to" below in the future. When mic key is
pressed, we will transit to a voice input keyset. So this is a temporary
solution for now as we dont have voice input keyset yet.
I could be wrong. In that case, I agree we should move it out. Maybe just move
it to the switch statement in key up handler? Since we dont have keyset
transition, we can just use key up handler like other keys.

On 2013/05/23 21:39:28, bryeung wrote:
> is there any way we can push this out of the general keyDown function?

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:102: if (to) {
renamed to "toKeyset", but I am not sure if this is a good one either. Do you
have suggestion?

On 2013/05/23 21:39:28, bryeung wrote:
> can we rename "to" to something a bit more descriptive?

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:138: this.keyset = to;
This is mainly for upper case keys. When an upper case key is pressed, keyboard
should transit back to lower case. The same code in keyDown is for transit to
"symbol" or "more" mode.

I tried android keyboard, and it works the same way as ours for upper case mode.
Although, when symbol key is down, android keyboard do not immediately transit
to symbol keyboard. It waits for key up event to transit.

I dont know if there is any specific reason that we use keydown for transition.
But it seems we can make the code easier/less confusing if we only transit on
keyUp like android? what do you think?
 
On 2013/05/23 21:39:28, bryeung wrote:
> why are we doing this on up and down?  I must not understand what this does...

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/mai...
File ui/keyboard/resources/main.css (right):

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/mai...
ui/keyboard/resources/main.css:91: background-image: url('images/del.svg');
On 2013/05/23 21:39:28, bryeung wrote:
> if the SVGs aren't working, let's just remove them in this CL

Done.

https://codereview.chromium.org/15176004/diff/36001/ui/keyboard/resources/web...
File ui/keyboard/resources/webui/main.css (right):

https://codereview.chromium.org/15176004/diff/36001/ui/keyboard/resources/web...
ui/keyboard/resources/webui/main.css:100: .mic {
Looks like the mic key still exists in the mock. I will fix the svg issue in a
separate CL.

[bryeung, 2013-05-24 19:33:46]

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
File ui/keyboard/resources/elements/kb-keyboard.html (right):

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:138: this.keyset = to;
On 2013/05/24 14:43:50, bshe wrote:
> This is mainly for upper case keys. When an upper case key is pressed,
keyboard
> should transit back to lower case. The same code in keyDown is for transit to
> "symbol" or "more" mode.
> 
> I tried android keyboard, and it works the same way as ours for upper case
mode.
> Although, when symbol key is down, android keyboard do not immediately transit
> to symbol keyboard. It waits for key up event to transit.
> 
> I dont know if there is any specific reason that we use keydown for
transition.
> But it seems we can make the code easier/less confusing if we only transit on
> keyUp like android? what do you think?

I'm fine with transitioning only on keyUp.  Let's implement it that way for now,
and UX can ask us to change it later if they feel strongly about it :-)



>  
> On 2013/05/23 21:39:28, bryeung wrote:
> > why are we doing this on up and down?  I must not understand what this
does...
>

[bshe, 2013-05-29 16:35:01]

Hi Bryan.

Could you please take another look? Thanks.

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
File ui/keyboard/resources/elements/kb-keyboard.html (right):

https://codereview.chromium.org/15176004/diff/27001/ui/keyboard/resources/ele...
ui/keyboard/resources/elements/kb-keyboard.html:138: this.keyset = to;
Done. Please see the latest patchset, the main files that I change are:
ui/keyboard/resources/elements/kb-keyboard.html
ui/keyboard/resources/api_adapter.js
ui/keyboard/resources/voice_input.js

Other files are result from rebase.

On 2013/05/24 19:33:47, bryeung wrote:
> On 2013/05/24 14:43:50, bshe wrote:
> > This is mainly for upper case keys. When an upper case key is pressed,
> keyboard
> > should transit back to lower case. The same code in keyDown is for transit
to
> > "symbol" or "more" mode.
> > 
> > I tried android keyboard, and it works the same way as ours for upper case
> mode.
> > Although, when symbol key is down, android keyboard do not immediately
transit
> > to symbol keyboard. It waits for key up event to transit.
> > 
> > I dont know if there is any specific reason that we use keydown for
> transition.
> > But it seems we can make the code easier/less confusing if we only transit
on
> > keyUp like android? what do you think?
> 
> I'm fine with transitioning only on keyUp.  Let's implement it that way for
now,
> and UX can ask us to change it later if they feel strongly about it :-)
> 
> 
> 
> >  
> > On 2013/05/23 21:39:28, bryeung wrote:
> > > why are we doing this on up and down?  I must not understand what this
> does...
> > 
>

[bryeung, 2013-05-29 17:16:06]

just one little fix, and then I think this is good to go

https://codereview.chromium.org/15176004/diff/54001/ui/keyboard/resources/api...
File ui/keyboard/resources/api_adapter.js (right):

https://codereview.chromium.org/15176004/diff/54001/ui/keyboard/resources/api...
ui/keyboard/resources/api_adapter.js:16: keyIdentifier = 'Spacebar';
this kind of logic doesn't belong in the adapter, which should just have the
platform specific API call in it

[bshe, 2013-05-29 18:34:47]

Thanks for review. Could you please take another look?

https://codereview.chromium.org/15176004/diff/54001/ui/keyboard/resources/api...
File ui/keyboard/resources/api_adapter.js (right):

https://codereview.chromium.org/15176004/diff/54001/ui/keyboard/resources/api...
ui/keyboard/resources/api_adapter.js:16: keyIdentifier = 'Spacebar';
doh. You are right. I forget about webui version. Moved the function to main.js
which shared by both component and webui keyboard.


On 2013/05/29 17:16:06, bryeung wrote:
> this kind of logic doesn't belong in the adapter, which should just have the
> platform specific API call in it

[bryeung, 2013-05-29 18:36:30]

lgtm

Let's go ahead and lands this: we can iterate with suggestions from the polymer
team later if they have comments.

[bshe, 2013-05-29 18:38:43]

sounds good. thanks

+miket for owners:
chrome/browser/extensions/image_loader.cc

[miket_OOO, 2013-05-29 18:40:10]

> +miket for owners:
> chrome/browser/extensions/image_loader.cc

LGTM

[commit-bot: I haz the power, 2013-05-29 19:00:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/15176004/63001

[commit-bot: I haz the power, 2013-05-29 19:17:25]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[SteveT, 2013-05-30 14:31:30]

Random drive-by nit, while this is stalled.

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/web...
File ui/keyboard/resources/webui/keysets.html (right):

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/web...
ui/keyboard/resources/webui/keysets.html:2: Copyright 2013 The Toolkitchen
Authors. All rights reserved.
Toolkitchen -> Chromium ?

[SteveT, 2013-05-30 14:46:19]

Another drive by.

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/key...
File ui/keyboard/resources/keysets.html (right):

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/key...
ui/keyboard/resources/keysets.html:32: <kb-key class="dark"
char=".com">com</kb-key>
com -> .com

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/web...
File ui/keyboard/resources/webui/keysets.html (right):

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/web...
ui/keyboard/resources/webui/keysets.html:29: <kb-key class="dark"
char=".com">com</kb-key>
com -> .com

[SteveT, 2013-05-30 15:27:38]

One more thing worth mentioning: I noticed that pressing on the symbol key when
it says #123 actually injects #123 into text fields. Same goes for when it says
abc.

Seems like we're missing a case here. Feel free to fix it in a quick follow up
if you think it convolutes what's already reviewed here.

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/web...
File ui/keyboard/resources/webui/keysets.html (right):

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/web...
ui/keyboard/resources/webui/keysets.html:88: <kb-key class='return dark'
char="Enter"></kb-key>
Missing "enter" here between the tags.

[bshe, 2013-05-30 16:29:10]

Thanks for the drive-ups. Steve. I have fixed them. Once the test framework is
ready, I will wrote test for the virtual keyboard.

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/key...
File ui/keyboard/resources/keysets.html (right):

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/key...
ui/keyboard/resources/keysets.html:32: <kb-key class="dark"
char=".com">com</kb-key>
On 2013/05/30 14:46:19, SteveT wrote:
> com -> .com

Done.

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/web...
File ui/keyboard/resources/webui/keysets.html (right):

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/web...
ui/keyboard/resources/webui/keysets.html:2: Copyright 2013 The Toolkitchen
Authors. All rights reserved.
On 2013/05/30 14:31:30, SteveT wrote:
> Toolkitchen -> Chromium ?

Done.

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/web...
ui/keyboard/resources/webui/keysets.html:29: <kb-key class="dark"
char=".com">com</kb-key>
I remember we just want to show com for the keyboard. But maybe I am wrong.
Thanks for pointing it out.

On 2013/05/30 14:46:19, SteveT wrote:
> com -> .com

https://codereview.chromium.org/15176004/diff/63001/ui/keyboard/resources/web...
ui/keyboard/resources/webui/keysets.html:88: <kb-key class='return dark'
char="Enter"></kb-key>
On 2013/05/30 15:27:38, SteveT wrote:
> Missing "enter" here between the tags.

Done.

[SteveT, 2013-05-30 17:00:25]

Cool - and you're planning on fixing the #123/abc injection issue in a new
patch?

[bshe, 2013-05-30 17:08:53]

On 2013/05/30 17:00:25, SteveT wrote:
> Cool - and you're planning on fixing the #123/abc injection issue in a new
> patch?

It should fixed in this patch. Sorry, I forgot to mention.

[SteveT, 2013-05-30 17:42:31]

Cool. LGTM.

[bshe, 2013-06-20 16:33:45]

Committed patchset #8 manually as r207448 (presubmit successful).