Make keyboard overlay speak when ChromeVox is active

Make keyboard overlay speak when ChromeVox is active

Pressing modifier keys will speak all the shortcuts with the modifier

BUG=chromium-os:16603
TEST=Enable accessibility.  Press Ctrl+Alt+/ to show keyboard overlay.  Press modifier keys and ensure it speaks.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96909

[dmazzoni, 2011-08-12 21:26:09]

LGTM with just a few minor issues.

http://codereview.chromium.org/7628018/diff/3001/chrome/browser/resources/key...
File chrome/browser/resources/keyboard_overlay_accessibility_helper.js (right):

http://codereview.chromium.org/7628018/diff/3001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:10:
if(cvox.Api.isChromeVoxActive())
Nit: Space after if

http://codereview.chromium.org/7628018/diff/3001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:35:
maybeSpeakAllshortcuts: function(modifiers) {
Nit: Capital S: maybeSpeakAllshortcuts -> maybeSpeakAllShortcuts

http://codereview.chromium.org/7628018/diff/3001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:47: var
keysText = modifiers.concat(keyLabel).join(' plus ');
I think you can use '+' here - I just tested and it will get spoken.

Otherwise you'd have to mark this string for translation.

[hashimoto, 2011-08-15 05:00:33]

A new patch set.
Thank you for your kind review.

http://codereview.chromium.org/7628018/diff/3001/chrome/browser/resources/key...
File chrome/browser/resources/keyboard_overlay_accessibility_helper.js (right):

http://codereview.chromium.org/7628018/diff/3001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:10:
if(cvox.Api.isChromeVoxActive())
Fixed

On 2011/08/12 21:26:09, Dominic Mazzoni wrote:
> Nit: Space after if

http://codereview.chromium.org/7628018/diff/3001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:31:
this.maybeSpeak(keysText, 1, {rate: 0.5});
Removed 'rate' parameter here because it does nothing on real ChromeOS devices.

http://codereview.chromium.org/7628018/diff/3001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:35:
maybeSpeakAllshortcuts: function(modifiers) {
Fixed.

On 2011/08/12 21:26:09, Dominic Mazzoni wrote:
> Nit: Capital S: maybeSpeakAllshortcuts -> maybeSpeakAllShortcuts

http://codereview.chromium.org/7628018/diff/3001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:47: var
keysText = modifiers.concat(keyLabel).join(' plus ');
Very good point, thanks.

On 2011/08/12 21:26:09, Dominic Mazzoni wrote:
> I think you can use '+' here - I just tested and it will get spoken.
> 
> Otherwise you'd have to mark this string for translation.

[mazda, 2011-08-16 05:15:46]

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
File chrome/browser/resources/keyboard_overlay_accessibility_helper.js (right):

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:13: if (!(e
instanceof ReferenceError))
The following check may work though I haven't tested.

if (window.cvox)
  ...

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:19:
maybeSpeak: function(textString, queueMode, properties) {
This function can be private.

maybeSpeak_

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:24: maybeStop:
function() {
Ditto

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:29:
maybeSpeakShortcut: function(keysText, shortcutText) {
Ditto

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:35:
maybeSpeakAllShortcuts: function(modifiers) {
How about inserting early exit if cvoxIsActive returns false?

[hashimoto, 2011-08-16 07:54:56]

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
File chrome/browser/resources/keyboard_overlay_accessibility_helper.js (right):

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:13: if (!(e
instanceof ReferenceError))
Thank you for the suggestion, fixed.

On 2011/08/16 05:15:46, mazda wrote:
> The following check may work though I haven't tested.
> 
> if (window.cvox)
>   ...

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:19:
maybeSpeak: function(textString, queueMode, properties) {
On 2011/08/16 05:15:46, mazda wrote:
> This function can be private.
> 
> maybeSpeak_

This method is removed in the new patch set

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:24: maybeStop:
function() {
On 2011/08/16 05:15:46, mazda wrote:
> Ditto

This method is removed in the new patch set

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:29:
maybeSpeakShortcut: function(keysText, shortcutText) {
On 2011/08/16 05:15:46, mazda wrote:
> Ditto

Made private

http://codereview.chromium.org/7628018/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:35:
maybeSpeakAllShortcuts: function(modifiers) {
On 2011/08/16 05:15:46, mazda wrote:
> How about inserting early exit if cvoxIsActive returns false?

Fixed.

http://codereview.chromium.org/7628018/diff/11002/chrome/browser/resources/ke...
chrome/browser/resources/keyboard_overlay_accessibility_helper.js:39: var layout
= getLayouts()[keyboardGlyphData.layoutName];
Moved speakShortcut to the end part of the class because it's now private

[mazda, 2011-08-16 08:11:27]

LGTM