Add 5 experimental accessibility features on Chrome OS.

Add 5 experimental accessibility features on Chrome OS.

This just adds the boilerplate for five new accessibility features and hides
them behind a flag. All five have been prototyped and are ready for
implementation.

The first three are related, they just provide visual highlighting to
help low-vision users keep track of things on the screen:
* Caret highlight
* Cursor highlight
* Focus highlight

Hold down a key and click or drag to speak any visible text on screen
* Select to speak

Control the whole computer with a single switch (for motor impaired users)
* Switch access

For now, there's no need to put these in the tray. They can graduate to the
tray when they launch.

BUG=314889, 593887, 593885
Committed: https://crrev.com/756089a218696542cadcf96a1a7ed00025c1f255
Cr-Commit-Position: refs/heads/master@{#381353}

[dmazzoni, 2016-03-10 22:49:25]

dmazzoni@chromium.org changed reviewers:
+ dbeam@chromium.org, isherman@chromium.org, oshima@chromium.org,
rockot@chromium.org

[dmazzoni, 2016-03-10 22:49:26]

oshima: ash/,
        chrome/browser/chromeos/
        chrome/browser/ui/ash/
dbeam: chrome/browser/resources/options/,
       chrome/browser/ui/webui/options/
rockot: chrome/common/extensions/api/
isherman: tools/metrics/

[Ilya Sherman, 2016-03-10 23:41:17]

https://codereview.chromium.org/1785833002/diff/20001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1785833002/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:9334: <action
name="Options_AccessibilityCaretHighlight">
It seems redundant to have both this, and disable/enable states.  Do you really
need both?

[dmazzoni, 2016-03-10 23:56:58]

https://codereview.chromium.org/1785833002/diff/20001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1785833002/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:9334: <action
name="Options_AccessibilityCaretHighlight">
On 2016/03/10 23:41:16, Ilya Sherman wrote:
> It seems redundant to have both this, and disable/enable states.  Do you
really
> need both?

Nope, was trying to copy other Options metrics, but looking closer
it looks like "_Enable" and "_Disable" are called automatically so
that's all I need.

[Ilya Sherman, 2016-03-10 23:58:22]

metrics lgtm, thanks

[oshima, 2016-03-11 02:20:56]

https://codereview.chromium.org/1785833002/diff/40001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/1785833002/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.h:222: extern const char
kAccessibilitySwitchAccessEnabled[];
does these pref key have to be in chrome/ ? 

pref service is componentized, so they can be in ash unless it has to use
class/API in chrome/.

[Dan Beam, 2016-03-11 05:35:40]

lgtm w/nits

https://codereview.chromium.org/1785833002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/1785833002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:969: <div
id="experimental_accessibility_features" hidden>
ids-like-this

https://codereview.chromium.org/1785833002/diff/20001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/1785833002/diff/20001/chromeos/chromeos_switc...
chromeos/chromeos_switches.cc:150: 
though i feel your name is pretty self-documenting, consider adding a comment
here

https://codereview.chromium.org/1785833002/diff/20001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.h (right):

https://codereview.chromium.org/1785833002/diff/20001/chromeos/chromeos_switc...
chromeos/chromeos_switches.h:64: CHROMEOS_EXPORT extern const char
kEnableExperimentalAccessibilityFeatures[];
nit: alphabetize

https://codereview.chromium.org/1785833002/diff/40001/ash/default_accessibili...
File ash/default_accessibility_delegate.h (right):

https://codereview.chromium.org/1785833002/diff/40001/ash/default_accessibili...
ash/default_accessibility_delegate.h:67: bool switch_access_enabled_;
nit: you know you can do inline initialization now right :)?

https://codereview.chromium.org/1785833002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/1785833002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:957: <div class="checkbox
controlled-setting-with-label">
bummer, this should probably option-name (rather than option_name)

not a big deal, just copying a few times in this CL :(

https://codereview.chromium.org/1785833002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/1785833002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/browser_options_handler.cc:725:
chromeos::switches::kEnableExperimentalAccessibilityFeatures));
nit: if /options/ wasn't on its way out (see: MD settings) I'd recommend you
test this

instead, I recommend you port it ;) (to MD settings or at least make sure we
don't forget it)

[dmazzoni, 2016-03-11 15:11:28]

https://codereview.chromium.org/1785833002/diff/40001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/1785833002/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.h:222: extern const char
kAccessibilitySwitchAccessEnabled[];
On 2016/03/11 02:20:55, oshima wrote:
> does these pref key have to be in chrome/ ? 
> 
> pref service is componentized, so they can be in ash unless it has to use
> class/API in chrome/.

All of these have at least some of their implementation in
chrome/browser/*chromeos* - for example the magnifier does focus tracking,
spoken feedback starts and stops the extension, and so on.

Would it make more sense to move them to /chromeos/chromeos_pref_names? That
makes more sense to me than ash/ - don't we still have a bot that compiles ash
on Windows?

[dmazzoni, 2016-03-11 15:12:54]

On 2016/03/11 05:35:40, Dan Beam wrote:
> instead, I recommend you port it ;) (to MD settings or at least make sure we
> don't forget it)

There's no accessibility settings in MD settings right now as far as I can tell,
and a lot of it seems broken. Should I try to port it anyway now, or wait until
it's closer?

[Dan Beam, 2016-03-11 17:23:04]

On 2016/03/11 15:12:54, dmazzoni wrote:
> On 2016/03/11 05:35:40, Dan Beam wrote:
> > instead, I recommend you port it ;) (to MD settings or at least make sure we
> > don't forget it)
> 
> There's no accessibility settings in MD settings right now as far as I can
tell,
> and a lot of it seems broken. Should I try to port it anyway now, or wait
until
> it's closer?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

[dmazzoni, 2016-03-11 20:29:35]

OK, ported to md-settings, too. It works, but
the strings don't show up until after I reload.
Any ideas?

https://codereview.chromium.org/1785833002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/1785833002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:969: <div
id="experimental_accessibility_features" hidden>
On 2016/03/11 05:35:40, Dan Beam wrote:
> ids-like-this

Done.

https://codereview.chromium.org/1785833002/diff/20001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/1785833002/diff/20001/chromeos/chromeos_switc...
chromeos/chromeos_switches.cc:150: 
On 2016/03/11 05:35:40, Dan Beam wrote:
> though i feel your name is pretty self-documenting, consider adding a comment
> here

Done.

https://codereview.chromium.org/1785833002/diff/20001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.h (right):

https://codereview.chromium.org/1785833002/diff/20001/chromeos/chromeos_switc...
chromeos/chromeos_switches.h:64: CHROMEOS_EXPORT extern const char
kEnableExperimentalAccessibilityFeatures[];
On 2016/03/11 05:35:40, Dan Beam wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/1785833002/diff/40001/ash/default_accessibili...
File ash/default_accessibility_delegate.h (right):

https://codereview.chromium.org/1785833002/diff/40001/ash/default_accessibili...
ash/default_accessibility_delegate.h:67: bool switch_access_enabled_;
On 2016/03/11 05:35:40, Dan Beam wrote:
> nit: you know you can do inline initialization now right :)?

Totally not used to that. :)

Done for this file. I'll try to migrate other files when I can do them
completely.

[Dan Beam, 2016-03-11 22:07:20]

i would consider splitting the MD impl off if this CL is time sensitive

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:970: <div
class="option_name" id="accessibility_caret_highlight">
ids-like-this-in-all-your-new-code

or just remove these IDs because I don't see where you actually use them

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/a11y_page/a11y_page.html (right):

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/a11y_page.html:61: <template
is="dom-if" if="[[showExperimentalFeatures_]]" restamp>
I don't think you need restamp

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/a11y_page/a11y_page.js (right):

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/a11y_page.js:30:
showExperimentalFeatures_: Boolean,
I think this is all you need:

/**
 * Whether to show experimental accessibility features.
 * @private {boolean}
 */
showExperimentalFeatures_: {
  type: Boolean,
  value: function() {
    return loadTimeData.getBoolean('enableExperimentalA11yFeatures');
  },
},

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/a11y_page.js:48: * should be shown.
@param {boolean} show ...

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/a11y_handler.cc (right):

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/a11y_handler.cc:34:
chromeos::switches::kEnableExperimentalAccessibilityFeatures));
can you just push this value into loadTimeData instead?

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/a11y_handler.h (right):

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/a11y_handler.h:9: #include
"chrome/browser/ui/webui/settings/md_settings_ui.h"
this is now settings_page_ui_handler.h

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/a11y_handler.h:34: Profile* profile_; 
// Weak pointer.
unused

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:109:
chrome::kChromeAccessibilityHelpURL);
I think rather than making an A11yHandler, you should do:

  html_source->AddBoolean("experimentalA11yFeaturesEnabled", ...);

here, and then from javascript you can do:

  if (loadTimeData.getBoolean('experimentalA11yFeaturesEnabled'))
    ...

i didn't name this file or all the methods in it "Strings", so it's not my fault
you're adding ... a boolean ;)

[dmazzoni, 2016-03-14 04:24:53]

Material Design settings changes moved to a different
changelist:

https://codereview.chromium.org/1798733003/

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/1785833002/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:970: <div
class="option_name" id="accessibility_caret_highlight">
On 2016/03/11 22:07:19, Dan Beam wrote:
> ids-like-this-in-all-your-new-code
> 
> or just remove these IDs because I don't see where you actually use them

Removed

[Ken Rockot(use gerrit already), 2016-03-14 18:59:00]

extensions lgtm

[Dan Beam, 2016-03-14 21:11:50]

still lgtm

[oshima, 2016-03-14 23:19:52]

https://codereview.chromium.org/1785833002/diff/40001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/1785833002/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.h:222: extern const char
kAccessibilitySwitchAccessEnabled[];
On 2016/03/11 15:11:28, dmazzoni wrote:
> On 2016/03/11 02:20:55, oshima wrote:
> > does these pref key have to be in chrome/ ? 
> > 
> > pref service is componentized, so they can be in ash unless it has to use
> > class/API in chrome/.
> 
> All of these have at least some of their implementation in
> chrome/browser/*chromeos* - for example the magnifier does focus tracking,
> spoken feedback starts and stops the extension, and so on.

Focus tracking seems to depend on content, not chrome, and extension api do not
have to be in chrome/browser. see extensions/browser/api.

I understand that the some of accessibility implementations may need access
chrome/browser, and that's where we need delegate.
I also strongly recommend to stop using chrome/browser interface directly, such
as Profile (use BrowserContext instead), BrowserProcess (pass objects).

> Would it make more sense to move them to /chromeos/chromeos_pref_names? That
> makes more sense to me than ash/ - don't we still have a bot that compiles ash
> on Windows?

Or ui/chromeos/accessibility or components/accessibility, if we can move more
code out from chrome/browser, as chromeos for non UI stuff.

For win_ash, you can just exclude it on Windows. It's does not have
accessibility stuff now, so it's not a regression.

It doesn't have to be in this CL, but this is getting more important for
mustash, and it's better to do this sooner than later. Can you at least file a
bug and add TODO here?

[dmazzoni, 2016-03-15 05:30:33]

On 2016/03/14 23:19:52, oshima wrote:
> It doesn't have to be in this CL, but this is getting more important for
> mustash, and it's better to do this sooner than later. Can you at least file a
> bug and add TODO here?

I filed http://crbug.com/594887 to start the conversation on moving
accessibility code out of chrome/ and added a TODO to this change.

[oshima, 2016-03-15 07:27:57]

lgtm thanks!

[dmazzoni, 2016-03-15 17:33:55]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-03-15 17:33:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1785833002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-15 17:34:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785833002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785833002/120001

[commit-bot: I haz the power, 2016-03-15 17:48:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-15 17:48:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmazzoni, 2016-03-15 21:01:52]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-03-15 21:01:52]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
isherman@chromium.org, oshima@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1785833002/#ps160001
(title: "Add stub for accessibilityFeatures docs")

[commit-bot: I haz the power, 2016-03-15 21:02:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785833002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785833002/160001

[commit-bot: I haz the power, 2016-03-15 21:16:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-15 21:16:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmazzoni, 2016-03-15 21:32:56]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-03-15 21:32:57]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
isherman@chromium.org, oshima@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1785833002/#ps180001
(title: "Change option_name to option-name")

[commit-bot: I haz the power, 2016-03-15 21:33:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785833002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785833002/180001

[commit-bot: I haz the power, 2016-03-15 23:55:31]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-03-15 23:56:22]

Description was changed from

==========
Add 5 experimental accessibility features on Chrome OS.

This just adds the boilerplate for five new accessibility features and hides
them behind a flag. All five have been prototyped and are ready for
implementation.

The first three are related, they just provide visual highlighting to
help low-vision users keep track of things on the screen:
* Caret highlight
* Cursor highlight
* Focus highlight

Hold down a key and click or drag to speak any visible text on screen
* Select to speak

Control the whole computer with a single switch (for motor impaired users)
* Switch access

For now, there's no need to put these in the tray. They can graduate to the
tray when they launch.

BUG=314889,593887,593885
==========

to

==========
Add 5 experimental accessibility features on Chrome OS.

This just adds the boilerplate for five new accessibility features and hides
them behind a flag. All five have been prototyped and are ready for
implementation.

The first three are related, they just provide visual highlighting to
help low-vision users keep track of things on the screen:
* Caret highlight
* Cursor highlight
* Focus highlight

Hold down a key and click or drag to speak any visible text on screen
* Select to speak

Control the whole computer with a single switch (for motor impaired users)
* Switch access

For now, there's no need to put these in the tray. They can graduate to the
tray when they launch.

BUG=314889,593887,593885

Committed: https://crrev.com/756089a218696542cadcf96a1a7ed00025c1f255
Cr-Commit-Position: refs/heads/master@{#381353}
==========

[commit-bot: I haz the power, 2016-03-15 23:56:25]

Patchset 10 (id:??) landed as
https://crrev.com/756089a218696542cadcf96a1a7ed00025c1f255
Cr-Commit-Position: refs/heads/master@{#381353}