Add a button on status bar to indicate that accessibility is enabled.

Add a button on status bar to indicate that accessibility is enabled.

BUG=chromium-os:18937
TEST=Press Ctrl+Alt+Z on login screen to enable accessibility, ensure that a hand icon with an orange background appears.


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

[hashimoto, 2011-08-26 09:30:51]

Could you review this?

[dmazzoni, 2011-08-26 15:51:06]

Code looks great! The only additional thing we need is a way for people who turn
this on accidentally to turn it off.

Two options:

1. Add Ctrl+Alt+Z to the tooltip so that the user knows what key to press to
toggle it.

2. Make the indicator into an actual menu, with an option "Turn off
accessibility (Ctrl + Alt + Z).

What do you think?

Something to keep in mind is that pretty soon we will have more than one type of
accessibility, if you'll be adding magnifier support. How will you want it to
act then? Perhaps it would make sense for the accessibility status menu to
always be present on the login screen, with a menu containing different options:

  - Turn on spoken feedback (Ctrl+Alt+Z)
  - Turn on spoken magnifier (Ctrl+Alt+M)  

I'm not suggesting that you implement all of this now, just think about creating
a design that will still make sense when new accessibility features are added.

[hashimoto, 2011-08-26 16:39:51]

On 2011/08/26 15:51:06, Dominic Mazzoni wrote:
> Code looks great! The only additional thing we need is a way for people who
turn
> this on accidentally to turn it off.
> 
> Two options:
> 
> 1. Add Ctrl+Alt+Z to the tooltip so that the user knows what key to press to
> toggle it.
> 
> 2. Make the indicator into an actual menu, with an option "Turn off
> accessibility (Ctrl + Alt + Z).
> 
> What do you think?
I think the option 2 is better because a user can turn the feature off with
mouse only.
With the option 1, after s/he moves mouse cursor over the button to show
tooltip, then s/he knows how to turn the feature off, s/he have to switch to
keyboard to press Ctrl+Alt+Z.

There is a small problem that this button appears even after the login screen.
Currently Ctrl+Alt+Z does not work after logging in so that the caption of the
menu should be just "Turn off
accessibility" without "(Ctrl + Alt + Z)" .

> Something to keep in mind is that pretty soon we will have more than one type
of
> accessibility, if you'll be adding magnifier support. How will you want it to
> act then? Perhaps it would make sense for the accessibility status menu to
> always be present on the login screen, with a menu containing different
options:
> 
>   - Turn on spoken feedback (Ctrl+Alt+Z)
>   - Turn on spoken magnifier (Ctrl+Alt+M)  
> 
> I'm not suggesting that you implement all of this now, just think about
creating
> a design that will still make sense when new accessibility features are added.
Good point, adding a menu option for magnifier will be easy task even with the
current code.
Besides this button, It also seems good to do some preparation before new
features are added. (e.g. renaming prefs::kAccessibilityEnabled to
prefs::kSpokenFeedbackEnabled)

[dmazzoni, 2011-08-26 16:46:13]

On Fri, Aug 26, 2011 at 9:39 AM, <hashimoto@chromium.org> wrote:

> There is a small problem that this button appears even after the login
>> screen.
>
> Currently Ctrl+Alt+Z does not work after logging in so that the caption of
> the
> menu should be just "Turn off
> accessibility" without "(Ctrl + Alt + Z)" .


I think it might be good for the menu to always appear, if accessibility is
on. That way a user can turn it off at any time.

Besides this button, It also seems good to do some preparation before new
> features are added. (e.g. renaming prefs::kAccessibilityEnabled to
> prefs::kSpokenFeedbackEnabled)


That sounds like a good idea.

Keep in mind that there may be some functionality in common to both types of
accessibility, so not everything will need to be renamed. For example, the
accessibility event router and accessibility extension api are probably
useful for both - but I agree, we should have two separate prefs.

A third pref we may want to add sometime is "high contrast" mode, which
switches to a white-on-black color scheme. This is already partially
implemented on Windows so it shouldn't be too hard to make it work on Chrome
OS too.

- Dominic

[Zachary Kuznia, 2011-08-29 03:59:20]

I like the idea of the icon having a real menu, with options to turn off
each type of accessibility.  It removes an extra step for someone who may
already be in a frustrating position.

-Zach

On Sat, Aug 27, 2011 at 1:46 AM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:

> On Fri, Aug 26, 2011 at 9:39 AM, <hashimoto@chromium.org> wrote:
>
>> There is a small problem that this button appears even after the login
>>> screen.
>>
>> Currently Ctrl+Alt+Z does not work after logging in so that the caption of
>> the
>> menu should be just "Turn off
>> accessibility" without "(Ctrl + Alt + Z)" .
>
>
> I think it might be good for the menu to always appear, if accessibility is
> on. That way a user can turn it off at any time.
>
> Besides this button, It also seems good to do some preparation before new
>> features are added. (e.g. renaming prefs::kAccessibilityEnabled to
>> prefs::kSpokenFeedbackEnabled)
>
>
> That sounds like a good idea.
>
> Keep in mind that there may be some functionality in common to both types
> of accessibility, so not everything will need to be renamed. For example,
> the accessibility event router and accessibility extension api are probably
> useful for both - but I agree, we should have two separate prefs.
>
> A third pref we may want to add sometime is "high contrast" mode, which
> switches to a white-on-black color scheme. This is already partially
> implemented on Windows so it shouldn't be too hard to make it work on Chrome
> OS too.
>
> - Dominic
>
>

[Zachary Kuznia, 2011-08-29 04:09:48]

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/accessibility_menu_button.cc (right):

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/accessibility_menu_button.cc:45: const
NotificationSource& source,
nit:  Indent should line up with int on previous line.

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/accessibility_menu_button.h (right):

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/accessibility_menu_button.h:28: virtual void
RunMenu(views::View* unused_source, const gfx::Point& pt);
Add the OVERRIDE marker here.

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/accessibility_menu_button.h:31: virtual void
Observe(int type,
Add the OVERRIDE marker here.

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/accessibility_menu_button.h:36: void
UpdateTooltip();
This should probably be private.

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/accessibility_menu_button.h:39: BooleanPrefMember
accessibility_enabled_;
nit:  I think there should be a blank line here.

[hashimoto, 2011-08-29 08:22:48]

New patch set, added browser_test and menu to disable accessibility.

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/accessibility_menu_button.cc (right):

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/accessibility_menu_button.cc:45: const
NotificationSource& source,
On 2011/08/29 04:09:48, Zachary Kuznia wrote:
> nit:  Indent should line up with int on previous line.

Fixed.

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/accessibility_menu_button.h (right):

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/accessibility_menu_button.h:28: virtual void
RunMenu(views::View* unused_source, const gfx::Point& pt);
On 2011/08/29 04:09:48, Zachary Kuznia wrote:
> Add the OVERRIDE marker here.

Fixed.

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/accessibility_menu_button.h:31: virtual void
Observe(int type,
On 2011/08/29 04:09:48, Zachary Kuznia wrote:
> Add the OVERRIDE marker here.

Fixed.

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/accessibility_menu_button.h:36: void
UpdateTooltip();
On 2011/08/29 04:09:48, Zachary Kuznia wrote:
> This should probably be private.

Good point, thanks.

http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/accessibility_menu_button.h:39: BooleanPrefMember
accessibility_enabled_;
On 2011/08/29 04:09:48, Zachary Kuznia wrote:
> nit:  I think there should be a blank line here.

Fixed.

[Zachary Kuznia, 2011-08-29 08:28:42]

On 2011/08/29 08:22:48, hashimoto wrote:
> New patch set, added browser_test and menu to disable accessibility.
> 
>
http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
> File chrome/browser/chromeos/status/accessibility_menu_button.cc (right):
> 
>
http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
> chrome/browser/chromeos/status/accessibility_menu_button.cc:45: const
> NotificationSource& source,
> On 2011/08/29 04:09:48, Zachary Kuznia wrote:
> > nit:  Indent should line up with int on previous line.
> 
> Fixed.
> 
>
http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
> File chrome/browser/chromeos/status/accessibility_menu_button.h (right):
> 
>
http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
> chrome/browser/chromeos/status/accessibility_menu_button.h:28: virtual void
> RunMenu(views::View* unused_source, const gfx::Point& pt);
> On 2011/08/29 04:09:48, Zachary Kuznia wrote:
> > Add the OVERRIDE marker here.
> 
> Fixed.
> 
>
http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
> chrome/browser/chromeos/status/accessibility_menu_button.h:31: virtual void
> Observe(int type,
> On 2011/08/29 04:09:48, Zachary Kuznia wrote:
> > Add the OVERRIDE marker here.
> 
> Fixed.
> 
>
http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
> chrome/browser/chromeos/status/accessibility_menu_button.h:36: void
> UpdateTooltip();
> On 2011/08/29 04:09:48, Zachary Kuznia wrote:
> > This should probably be private.
> 
> Good point, thanks.
> 
>
http://codereview.chromium.org/7753029/diff/1/chrome/browser/chromeos/status/...
> chrome/browser/chromeos/status/accessibility_menu_button.h:39:
BooleanPrefMember
> accessibility_enabled_;
> On 2011/08/29 04:09:48, Zachary Kuznia wrote:
> > nit:  I think there should be a blank line here.
> 
> Fixed.

LGTM

[hashimoto, 2011-08-29 09:52:57]

Some files are separated into http://codereview.chromium.org/7736012/ because
trybots do not support patches with binary files.

[dmazzoni, 2011-08-29 22:27:57]

LGTM

[commit-bot: I haz the power, 2011-08-30 04:11:54]

Can't apply patch for file chrome/app/theme/theme_resources.grd.
While running patch -p1 --forward --force;
patching file chrome/app/theme/theme_resources.grd
Hunk #1 FAILED at 503.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/app/theme/theme_resources.grd.rej

[commit-bot: I haz the power, 2011-08-30 06:46:22]

Change committed as 98788