Make Braille IME send space bar as an empty cell instead of letting propagate.

Make Braille IME send space bar as an empty cell instead of letting propagate.

This more realistically immitates what a braille display would provide and
makes the space bar behave like the other 'braille keys' (submit on key up,
no autorepeat).  This CL also makes the IME propagate all keys when
any modifier (or caps lock) is active so tha browser keyboard shortcuts
that would conflict with a braille key will still work.

BUG=310285
R=dtseng@chromium.org

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

[David Tseng, 2014-05-07 18:05:54]

https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/braille_ime/braille_ime.js (left):

https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/braille_ime/braille_ime.js:255: * Handles a
querty key on the home row as a braille key.
nit: I think it's "qwerty"

https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/braille_ime/braille_ime.js (right):

https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/braille_ime/braille_ime.js:100: 'KeyS': 0x04,
'KeyL': 0x20 ,
nit: extra space before comma

https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/braille_ime/braille_ime.js:101: 'Space': 0},
nit: space before }

https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/braille_ime/braille_ime.js:277: // Space
cancels other dots.
Just wondering about the behavior here. If I hold down, say dots 1 and 2, and
tap the space key by mistake (with dots 1 and 2 still held down), this would
cancel dots 1 and 2? Is that something we want?

https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/braille_ime/braille_ime_unittest.gtestjs
(left):

https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/braille_ime/braille_ime_unittest.gtestjs:167:
assertThat(this.port.messages,
I was expecting a test for the space key behavior introduced by this cl.

[Peter Lundblad, 2014-05-07 19:17:07]

PTAL

dtseng@chromium.org writes:
> 
>
https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
> File chrome/browser/resources/chromeos/braille_ime/braille_ime.js
> (left):
> 
>
https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
> chrome/browser/resources/chromeos/braille_ime/braille_ime.js:255: *
> Handles a querty key on the home row as a braille key.
> nit: I think it's "qwerty"

Of coruse it is, I wonder how I managed to get that wrong.  Fixed in two
places.


>
https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
> File chrome/browser/resources/chromeos/braille_ime/braille_ime.js
> (right):
> 
>
https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
> chrome/browser/resources/chromeos/braille_ime/braille_ime.js:100:
> 'KeyS': 0x04, 'KeyL': 0x20 ,
> nit: extra space before comma

Fixed.

>
https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
> chrome/browser/resources/chromeos/braille_ime/braille_ime.js:101:
> 'Space': 0},
> nit: space before }

Done.

>
https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
> chrome/browser/resources/chromeos/braille_ime/braille_ime.js:277: //
> Space cancels other dots.
> Just wondering about the behavior here. If I hold down, say dots 1 and
> 2, and tap the space key by mistake (with dots 1 and 2 still held down),
> this would cancel dots 1 and 2? Is that something we want?

Alright, changed it to track space separately and cancel if it is combined
with dot keys (until we support chords).

>
https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
> File
> chrome/browser/resources/chromeos/braille_ime/braille_ime_unittest.gtestjs
> (left):
> 
>
https://codereview.chromium.org/271723002/diff/1/chrome/browser/resources/chr...
>
chrome/browser/resources/chromeos/braille_ime/braille_ime_unittest.gtestjs:167:
> assertThat(this.port.messages,
> I was expecting a test for the space key behavior introduced by this cl.
> 

I had it, must have git --amended it away somehow.  Rewrote.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-05-07 23:38:00]

https://codereview.chromium.org/271723002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/braille_ime/braille_ime.js (right):

https://codereview.chromium.org/271723002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/braille_ime/braille_ime.js:290: dotsToSend &=
~this.SPACE;
Would it be clearer to just say
dotsToSend != this.SPACE?

[David Tseng, 2014-05-07 23:38:12]

lgtm

[Peter Lundblad, 2014-05-13 20:52:11]

dtseng@chromium.org writes:
> 
>
https://codereview.chromium.org/271723002/diff/20001/chrome/browser/resources...
> File chrome/browser/resources/chromeos/braille_ime/braille_ime.js
> (right):
> 
>
https://codereview.chromium.org/271723002/diff/20001/chrome/browser/resources...
> chrome/browser/resources/chromeos/braille_ime/braille_ime.js:290:
> dotsToSend &= ~this.SPACE;
> Would it be clearer to just say
> dotsToSend != this.SPACE?

Probably, changed.

Thank you,
//Peter

> https://codereview.chromium.org/271723002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Lundblad, 2014-05-13 21:00:37]

The CQ bit was checked by plundblad@chromium.org

[commit-bot: I haz the power, 2014-05-13 21:00:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/plundblad@chromium.org/271723002/60001

[commit-bot: I haz the power, 2014-05-13 23:58:55]

Change committed as 270255