Make views Slider accessible on Chrome OS

Make views Slider accessible on Chrome OS

BUG=117616
TEST=manual testing


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

[dmazzoni, 2012-03-12 08:34:43]

sadrul: slider code
hashimoto: everything else

I'll add OWNERS after you've taken a first look, thanks.

[sadrul, 2012-03-12 08:40:46]

LGTM. Some nits:

http://codereview.chromium.org/9665026/diff/3013/chrome/browser/accessibility...
File chrome/browser/accessibility/accessibility_extension_api.cc (right):

http://codereview.chromium.org/9665026/diff/3013/chrome/browser/accessibility...
chrome/browser/accessibility/accessibility_extension_api.cc:134: printf("Focus
%s\n", json_args.c_str());
- this line?

http://codereview.chromium.org/9665026/diff/3013/chrome/browser/accessibility...
chrome/browser/accessibility/accessibility_extension_api.cc:147: printf("Text
Changed %s\n", json_args.c_str());
ditto

http://codereview.chromium.org/9665026/diff/3013/ui/views/controls/slider.cc
File ui/views/controls/slider.cc (right):

http://codereview.chromium.org/9665026/diff/3013/ui/views/controls/slider.cc#...
ui/views/controls/slider.cc:69: GetWidget()->NotifyAccessibilityEvent(
A NULL check here for GetWidget(), in case the value of the slider is set before
it is attached to a widget.

[dmazzoni, 2012-03-12 08:44:11]

http://codereview.chromium.org/9665026/diff/3013/chrome/browser/accessibility...
File chrome/browser/accessibility/accessibility_extension_api.cc (right):

http://codereview.chromium.org/9665026/diff/3013/chrome/browser/accessibility...
chrome/browser/accessibility/accessibility_extension_api.cc:134: printf("Focus
%s\n", json_args.c_str());
On 2012/03/12 08:40:47, sadrul wrote:
> - this line?

Oops, thanks

http://codereview.chromium.org/9665026/diff/3013/ui/views/controls/slider.cc
File ui/views/controls/slider.cc (right):

http://codereview.chromium.org/9665026/diff/3013/ui/views/controls/slider.cc#...
ui/views/controls/slider.cc:69: GetWidget()->NotifyAccessibilityEvent(
On 2012/03/12 08:40:47, sadrul wrote:
> A NULL check here for GetWidget(), in case the value of the slider is set
before
> it is attached to a widget.

Done.

[hashimoto, 2012-03-12 17:26:18]

http://codereview.chromium.org/9665026/diff/6005/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental.accessibility.json (right):

http://codereview.chromium.org/9665026/diff/6005/chrome/common/extensions/api...
chrome/common/extensions/api/experimental.accessibility.json:105: "value":
{"type": "string", "description": "The value of the slider as a string."}
Cannot this be a number instead of string?

[dmazzoni, 2012-03-12 17:51:06]

On 2012/03/12 17:26:18, hashimoto wrote:
>
http://codereview.chromium.org/9665026/diff/6005/chrome/common/extensions/api...
> File chrome/common/extensions/api/experimental.accessibility.json (right):
> 
>
http://codereview.chromium.org/9665026/diff/6005/chrome/common/extensions/api...
> chrome/common/extensions/api/experimental.accessibility.json:105: "value":
> {"type": "string", "description": "The value of the slider as a string."}
> Cannot this be a number instead of string?

I was thinking of making this a string now (so it might include units or a
descriptive label) and adding additional fields for the numeric value later as
needed, what do you think?

Windows / Mac accessibility APIs expose these for a slider:
* value text
* numeric Value
* min value
* max value

I thought exposing just value text would be the most flexible for now, since min
and max aren't available. Would it make sense to you if I renamed it valueText
or stringValue?

[hashimoto, 2012-03-12 18:15:22]

On 2012/03/12 17:51:06, Dominic Mazzoni wrote:
> On 2012/03/12 17:26:18, hashimoto wrote:
> >
>
http://codereview.chromium.org/9665026/diff/6005/chrome/common/extensions/api...
> > File chrome/common/extensions/api/experimental.accessibility.json (right):
> > 
> >
>
http://codereview.chromium.org/9665026/diff/6005/chrome/common/extensions/api...
> > chrome/common/extensions/api/experimental.accessibility.json:105: "value":
> > {"type": "string", "description": "The value of the slider as a string."}
> > Cannot this be a number instead of string?
> 
> I was thinking of making this a string now (so it might include units or a
> descriptive label) and adding additional fields for the numeric value later as
> needed, what do you think?
> 
> Windows / Mac accessibility APIs expose these for a slider:
> * value text
> * numeric Value
> * min value
> * max value
> 
> I thought exposing just value text would be the most flexible for now, since
min
> and max aren't available. Would it make sense to you if I renamed it valueText
> or stringValue?

Let's keep the name |value| for the numeric value.
|stringValue| looks good.

[hashimoto, 2012-03-12 18:15:22]

On 2012/03/12 17:51:06, Dominic Mazzoni wrote:
> On 2012/03/12 17:26:18, hashimoto wrote:
> >
>
http://codereview.chromium.org/9665026/diff/6005/chrome/common/extensions/api...
> > File chrome/common/extensions/api/experimental.accessibility.json (right):
> > 
> >
>
http://codereview.chromium.org/9665026/diff/6005/chrome/common/extensions/api...
> > chrome/common/extensions/api/experimental.accessibility.json:105: "value":
> > {"type": "string", "description": "The value of the slider as a string."}
> > Cannot this be a number instead of string?
> 
> I was thinking of making this a string now (so it might include units or a
> descriptive label) and adding additional fields for the numeric value later as
> needed, what do you think?
> 
> Windows / Mac accessibility APIs expose these for a slider:
> * value text
> * numeric Value
> * min value
> * max value
> 
> I thought exposing just value text would be the most flexible for now, since
min
> and max aren't available. Would it make sense to you if I renamed it valueText
> or stringValue?

Let's keep the name |value| for the numeric value.
|stringValue| looks good.

[dmazzoni, 2012-03-12 18:27:09]

On Mon, Mar 12, 2012 at 11:15 AM, <hashimoto@chromium.org> wrote:

> Let's keep the name |value| for the numeric value.
> |stringValue| looks good.
>

Done.

[hashimoto, 2012-03-12 18:35:31]

lgtm

[commit-bot: I haz the power, 2012-03-12 19:01:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/9665026/11001

[commit-bot: I haz the power, 2012-03-12 19:01:09]

Presubmit check for 9665026-11001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui
    chrome/common/extensions/api
    ui

Presubmit checks took 1.9s to calculate.

[dmazzoni, 2012-03-12 19:05:56]

+ben for ui/ and chrome/browser/ui/
+aa for chrome/common/extensions/api/

[Aaron Boodman, 2012-03-12 20:06:42]

extensions lgtm

[Ben Goodger (Google), 2012-03-12 20:25:25]

lgtm

[commit-bot: I haz the power, 2012-03-12 20:28:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/9665026/11001

[commit-bot: I haz the power, 2012-03-12 23:23:18]

Change committed as 126262