MacViews a11y: Allow accessibility clients to set the AXValue on some controls.

MacViews a11y: Allow accessibility clients to set the AXValue on some controls.

Cocoa controls allow accessibility clients to set specific values on user input
controls via accessibility clients. This doesn't yet work for MacViews controls.
This CL adds support for writable ui::AX_VALUE attributes on MacViews.

BUG=610591
TEST=Open the XCode 'Accessibility Inspector' and hover the mouse over the
omnibox (for example). Press Cmd+F7 to lock onto the omnibox. Find the
'accessibilityValue' entry in the inspector (it should have a '(W)' next to
it to indicate it's writable). Click it and at the bottom of the inspector has a
textbox to edit the value. Edit and confirm, the text in the omnibox should be
updated.

Committed: https://crrev.com/0fa98f7891c47f1f942e3e584fc64227ebf65500
Cr-Commit-Position: refs/heads/master@{#412120}

[Patti Lor, 2016-08-10 03:46:10]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 03:46:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 04:49:36]

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

[commit-bot: I haz the power, 2016-08-10 04:49:37]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Patti Lor, 2016-08-11 00:42:07]

patricialor@chromium.org changed reviewers:
+ tapted@chromium.org

[Patti Lor, 2016-08-11 00:42:07]

Hi Trent, PTAL! Thanks.

[tapted, 2016-08-11 04:23:26]

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:356:
base::scoped_nsobject<NSMutableArray> axWritableAttributes(
I don't think we need this array unless it's used in a follow-up (in which case
it should be moved to a helper method that returns an NSArray*). But it's a bit
weird to build the array only to check if we put something in it.

I think "switching" on the attributeName  with [attributeName
isEqualToString:..] is the nicer approach.. but I'm a bit out of my depth wrt
how "setting" some of these should actually work :/ - see comments below.

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:362:
NSAccessibilitySelectedTextRangeAttribute,
I think the TextRange should be settable on ReadOnly text fields as well, same
with the VisibileCharacterRange

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:368: if
(node_->GetDelegate()->CanSetStringValue())
We already call node_->GetData().state a bunch here - is that where the callback
lives? Can we just check for its presence here, or is there a use-case for
putting it on the delegate, which wouldn't be satisfied that way?

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:369: [axWritableAttributes
addObject:NSAccessibilityValueAttribute];
I think NSAccessibilityValueAttribute and NSAccessibilitySelectedTextAttribute
should always be paired together (e.g. if all the text is selected,
NSAccessibilitySelectedTextAttribute is equivalent to
NSAccessibilityValueAttribute so they should be writable under the same set of
conditions.)

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:378:
axWritableAttributes.autorelease();
(usually we only do scoped_nsobject::autorelease() to return from a function
(i.e. with the value of the autorelease) - otherwise it's usually fine for the
scoped_nsobject just to go out of scope and do a "proper" release)

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:383: if ([attribute
isEqualToString:NSAccessibilityValueAttribute] &&
Does NSAccessibilitySelectedTextAttribute already work the same as NSTextField
when it's plumbed through here as well?

https://codereview.chromium.org/2230093002/diff/20001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/2230093002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:218: bool
NativeViewAccessibility::CanSetStringValue() {
Can/should this be enforced by ui::AX_STATE_READ_ONLY? (i.e. _in theory_ the
ui::AX_STATE_READ_ONLY flag should be set for all cases *unless* there is a
set_value_callback available to change it, but in reality that might be hard

https://codereview.chromium.org/2230093002/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right):

https://codereview.chromium.org/2230093002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:328: }
We should also check that the value of the views::TextField has been updated

[Patti Lor, 2016-08-12 01:56:39]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 01:57:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patti Lor, 2016-08-12 01:57:42]

Thanks Trent, PTAL (tests haven't run yet but I thought I should give you more
time to take a look).

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:356:
base::scoped_nsobject<NSMutableArray> axWritableAttributes(
On 2016/08/11 04:23:25, tapted wrote:
> I don't think we need this array unless it's used in a follow-up (in which
case
> it should be moved to a helper method that returns an NSArray*). But it's a
bit
> weird to build the array only to check if we put something in it.
> 
> I think "switching" on the attributeName  with [attributeName
> isEqualToString:..] is the nicer approach.. but I'm a bit out of my depth wrt
> how "setting" some of these should actually work :/ - see comments below.

Done.

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:362:
NSAccessibilitySelectedTextRangeAttribute,
On 2016/08/11 04:23:25, tapted wrote:
> I think the TextRange should be settable on ReadOnly text fields as well, same
> with the VisibileCharacterRange

Done.

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:368: if
(node_->GetDelegate()->CanSetStringValue())
On 2016/08/11 04:23:25, tapted wrote:
> We already call node_->GetData().state a bunch here - is that where the
callback
> lives? Can we just check for its presence here, or is there a use-case for
> putting it on the delegate, which wouldn't be satisfied that way?

The naming is a bit confusing - it's not actually node_->GetData().state that's
called (that is a unsigned int), but actually view_->GetAccessibleState(&state),
which gets us a ui::AXViewState. The latter is where the callback lives.
AXPlatformNodeCocoa shouldn't have access to the View it's representing, so I
have opted for putting a method on the delegate here. I agree it's a bit gross
though - I considered adding a optional parameter |dry_run| to the existing
SetStringValue() method, but that also seemed weird. Do you have a better idea?

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:369: [axWritableAttributes
addObject:NSAccessibilityValueAttribute];
On 2016/08/11 04:23:25, tapted wrote:
> I think NSAccessibilityValueAttribute and NSAccessibilitySelectedTextAttribute
> should always be paired together (e.g. if all the text is selected,
> NSAccessibilitySelectedTextAttribute is equivalent to
> NSAccessibilityValueAttribute so they should be writable under the same set of
> conditions.)

Done.

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:378:
axWritableAttributes.autorelease();
On 2016/08/11 04:23:25, tapted wrote:
> (usually we only do scoped_nsobject::autorelease() to return from a function
> (i.e. with the value of the autorelease) - otherwise it's usually fine for the
> scoped_nsobject just to go out of scope and do a "proper" release)

Done, thanks for the FYI!

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:383: if ([attribute
isEqualToString:NSAccessibilityValueAttribute] &&
On 2016/08/11 04:23:25, tapted wrote:
> Does NSAccessibilitySelectedTextAttribute already work the same as NSTextField
> when it's plumbed through here as well?

Yep, writing NSAccessibilitySelectedTextAttribute on a NSTextfield will clear
the existing selection and replace it with new text. To make it work with
MacViews though I think there needs to be a new method on the delegate that will
figure out which substring to replace and replace the whole text with the old
string + substitution. (Actually, this applies to all the writable attributes -
was going to add callbacks and their own methods to the delegate to make them
work.) Is that what you were asking?

https://codereview.chromium.org/2230093002/diff/20001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/2230093002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:218: bool
NativeViewAccessibility::CanSetStringValue() {
On 2016/08/11 04:23:25, tapted wrote:
> Can/should this be enforced by ui::AX_STATE_READ_ONLY? (i.e. _in theory_ the
> ui::AX_STATE_READ_ONLY flag should be set for all cases *unless* there is a
> set_value_callback available to change it, but in reality that might be hard

I think that's a good idea - AXViewState is meant to be as much like a struct as
possible, so it doesn't enforce this. I've moved the READ_ONLY checks to here
from accessibilityIsAttributeSettable since this class is cross-platform.

https://codereview.chromium.org/2230093002/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right):

https://codereview.chromium.org/2230093002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:328: }
On 2016/08/11 04:23:26, tapted wrote:
> We should also check that the value of the views::TextField has been updated

Done. Also added tests for a read-only textfield.

[tapted, 2016-08-12 03:50:52]

lgtm % nits. Although I'm wary of returning `YES` for the ones not implemented
yet -- see what the a11y folks say

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:368: if
(node_->GetDelegate()->CanSetStringValue())
On 2016/08/12 01:57:42, Patti Lor wrote:
> On 2016/08/11 04:23:25, tapted wrote:
> > We already call node_->GetData().state a bunch here - is that where the
> callback
> > lives? Can we just check for its presence here, or is there a use-case for
> > putting it on the delegate, which wouldn't be satisfied that way?
> 
> The naming is a bit confusing - it's not actually node_->GetData().state
that's
> called (that is a unsigned int), but actually
view_->GetAccessibleState(&state),
> which gets us a ui::AXViewState. The latter is where the callback lives.
> AXPlatformNodeCocoa shouldn't have access to the View it's representing, so I
> have opted for putting a method on the delegate here. I agree it's a bit gross
> though - I considered adding a optional parameter |dry_run| to the existing
> SetStringValue() method, but that also seemed weird. Do you have a better
idea?

Ah I see. I guess it would be necessary to update the ui::AXNodeData
NativeViewAccessibility::GetData() so that it copies the callback on to  the
AXNodeData struct - but that could lead to lifetime trouble.

https://codereview.chromium.org/2230093002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:383: if ([attribute
isEqualToString:NSAccessibilityValueAttribute] &&
On 2016/08/12 01:57:42, Patti Lor wrote:
> On 2016/08/11 04:23:25, tapted wrote:
> > Does NSAccessibilitySelectedTextAttribute already work the same as
NSTextField
> > when it's plumbed through here as well?
> 
> Yep, writing NSAccessibilitySelectedTextAttribute on a NSTextfield will clear
> the existing selection and replace it with new text. To make it work with
> MacViews though I think there needs to be a new method on the delegate that
will
> figure out which substring to replace and replace the whole text with the old
> string + substitution. (Actually, this applies to all the writable attributes
-
> was going to add callbacks and their own methods to the delegate to make them
> work.) Is that what you were asking?

sorta - I guess the question is whether the callback invoked in
NativeViewAccessibility::SetStringValue() always replaces the current selection
(if there is one), or replaces the entire text regardless of any current
selection. But either way, there should be different handling - e.g. we could
pass through a `bool replace_selection` or similar on SetStringValue and the
callback.

https://codereview.chromium.org/2230093002/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):

https://codereview.chromium.org/2230093002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:366: return YES;
There might be some specific semantics of ui::AX_STATE_READ_ONLY, but I would
have thought these 4 could return YES unconditionally. E.g. you can still
highlight text on readonly textfields

https://codereview.chromium.org/2230093002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:371: if
(node_->GetDelegate()->CanSetStringValue())
perhaps just `return node_->GetDelegate()->CanSetStringValue();`

https://codereview.chromium.org/2230093002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:376: if
(ui::AXViewState::IsFlagSet(node_->GetData().state,
same here - just return the function result

https://codereview.chromium.org/2230093002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:391: // TODO(patricialor):
Plumb through all the other writable attributes as
It might be wrong to already return `YES` for the ones not plumbed through,
since it would silently fail here in those cases. (e.g. consider returning NO
above and putting TODO(..): Implement and return YES.) It's possible we run into
blocking problems implementing some of them, like 'selected children'

Of course, you'd have to ignore my previous two comments :)

https://codereview.chromium.org/2230093002/diff/40001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right):

https://codereview.chromium.org/2230093002/diff/40001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:313: id axTextfield
= [widget()->GetNativeWindow() accessibilityHitTest:midpoint];
nit: ax_textfield (or perhaps ax_node?)

https://codereview.chromium.org/2230093002/diff/40001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:335:
forAttribute:NSAccessibilityValueAttribute];
nit: align colons

[commit-bot: I haz the power, 2016-08-12 04:10:18]

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

[commit-bot: I haz the power, 2016-08-12 04:10:18]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-08-12 04:23:37]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 04:23:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 05:06:09]

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

[commit-bot: I haz the power, 2016-08-12 05:06:10]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-08-12 05:16:53]

patricialor@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[Patti Lor, 2016-08-12 05:16:54]

Hi dmazzoni@, PTAL for OWNERs review on everything except
native_widget_mac_accessibility_unittest.cc!

https://codereview.chromium.org/2230093002/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):

https://codereview.chromium.org/2230093002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:366: return YES;
On 2016/08/12 03:50:52, tapted (OOO soon) wrote:
> There might be some specific semantics of ui::AX_STATE_READ_ONLY, but I would
> have thought these 4 could return YES unconditionally. E.g. you can still
> highlight text on readonly textfields

Thanks, didn't know that! Acking this and the next two comments for future CLs.

https://codereview.chromium.org/2230093002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:371: if
(node_->GetDelegate()->CanSetStringValue())
On 2016/08/12 03:50:52, tapted (OOO soon) wrote:
> perhaps just `return node_->GetDelegate()->CanSetStringValue();`

Acknowledged.

https://codereview.chromium.org/2230093002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:376: if
(ui::AXViewState::IsFlagSet(node_->GetData().state,
On 2016/08/12 03:50:52, tapted (OOO soon) wrote:
> same here - just return the function result

Acknowledged.

https://codereview.chromium.org/2230093002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:391: // TODO(patricialor):
Plumb through all the other writable attributes as
On 2016/08/12 03:50:52, tapted (OOO soon) wrote:
> It might be wrong to already return `YES` for the ones not plumbed through,
> since it would silently fail here in those cases. (e.g. consider returning NO
> above and putting TODO(..): Implement and return YES.) It's possible we run
into
> blocking problems implementing some of them, like 'selected children'
> 
> Of course, you'd have to ignore my previous two comments :)

Done.

https://codereview.chromium.org/2230093002/diff/40001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right):

https://codereview.chromium.org/2230093002/diff/40001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:313: id axTextfield
= [widget()->GetNativeWindow() accessibilityHitTest:midpoint];
On 2016/08/12 03:50:52, tapted (OOO soon) wrote:
> nit: ax_textfield (or perhaps ax_node?)

Done.

https://codereview.chromium.org/2230093002/diff/40001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:335:
forAttribute:NSAccessibilityValueAttribute];
On 2016/08/12 03:50:52, tapted (OOO soon) wrote:
> nit: align colons

Done.

[dmazzoni, 2016-08-15 16:57:28]

lgtm

[Patti Lor, 2016-08-16 00:11:28]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2016-08-16 00:11:29]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2230093002/#ps60001
(title: "Don't say attributes are settable until implemented.")

[commit-bot: I haz the power, 2016-08-16 00:11:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 01:08:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-16 01:10:08]

Description was changed from

==========
MacViews a11y: Allow accessibility clients to set the AXValue on some controls.

Cocoa controls allow accessibility clients to set specific values on user input
controls via accessibility clients. This doesn't yet work for MacViews controls.
This CL adds support for writable ui::AX_VALUE attributes on MacViews.

BUG=610591
TEST=Open the XCode 'Accessibility Inspector' and hover the mouse over the
omnibox (for example). Press Cmd+F7 to lock onto the omnibox. Find the
'accessibilityValue' entry in the inspector (it should have a '(W)' next to
it to indicate it's writable). Click it and at the bottom of the inspector has a
textbox to edit the value. Edit and confirm, the text in the omnibox should be
updated.
==========

to

==========
MacViews a11y: Allow accessibility clients to set the AXValue on some controls.

Cocoa controls allow accessibility clients to set specific values on user input
controls via accessibility clients. This doesn't yet work for MacViews controls.
This CL adds support for writable ui::AX_VALUE attributes on MacViews.

BUG=610591
TEST=Open the XCode 'Accessibility Inspector' and hover the mouse over the
omnibox (for example). Press Cmd+F7 to lock onto the omnibox. Find the
'accessibilityValue' entry in the inspector (it should have a '(W)' next to
it to indicate it's writable). Click it and at the bottom of the inspector has a
textbox to edit the value. Edit and confirm, the text in the omnibox should be
updated.

Committed: https://crrev.com/0fa98f7891c47f1f942e3e584fc64227ebf65500
Cr-Commit-Position: refs/heads/master@{#412120}
==========

[commit-bot: I haz the power, 2016-08-16 01:10:09]

Patchset 4 (id:??) landed as
https://crrev.com/0fa98f7891c47f1f942e3e584fc64227ebf65500
Cr-Commit-Position: refs/heads/master@{#412120}