Mac: Add new accessibility attributes for textfields (and some for all views).

Mac: Add accessibility attributes for textfields, and AXEnabled for all views.

This change adds support for the following NSAccessibility attributes:
NSAccessibilityEnabledAttribute
NSAccessibilityFocusedAttribute
NSAccessibilityInsertionPointLineNumberAttribute
NSAccessibilityNumberOfCharactersAttribute
NSAccessibilityPlaceholderValue
NSAccessibilitySelectedTextAttribute
NSAccessibilitySelectedTextrangeAttribute
NSAccessibilityVisibleCharacterRangeAttribute

ui::AX_ATTR_PLACEHOLDER was also not used outside of Blink, so support this
attribute as well.

Add tests for all of these attributes and plumb through textfield change
notifications for ui::AX_EVENT_TEXT_CHANGED, ui::AX_EVENT_VALUE_CHANGED, and
ui::AX_EVENT_TEXT_SELECTION_CHANGED.

BUG=610591
Committed: https://crrev.com/c32a66d5bf5de90e99d220dafe3fff471419343c
Cr-Commit-Position: refs/heads/master@{#403370}

[Patti Lor, 2016-06-29 08:11:50]

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

[Patti Lor, 2016-06-29 08:11:50]

PTAL! Thanks :)

[dmazzoni, 2016-06-29 16:24:09]

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

[dmazzoni, 2016-06-29 16:24:11]

lgtm

https://codereview.chromium.org/2106953005/diff/40001/ui/accessibility/ax_enu...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2106953005/diff/40001/ui/accessibility/ax_enu...
ui/accessibility/ax_enums.idl:210: focused,
We used to have "focused" as a state here, but we got rid of it on purpose
because it was possible for two elements to both report they were focused at the
same time.

It's more foolproof to keep track of which element has focus (in the case of
Views, for example, that's already easy) and compute the
"focused" state dynamically.

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

https://codereview.chromium.org/2106953005/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:330: [axAttributes
addObjectsFromArray:kTextfieldAttributes];
Please add a comment like "fallthrough" so that it's clear you meant for this to
keep going with the switch and you're not accidentally missing a "break".

https://codereview.chromium.org/2106953005/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:422: // TODO(patricialor):
Check with dmazzoni@ to see if AX_STATE_ENABLED is still
This is correct for now, no TODO needed. My plan is to get rid of ENABLED at
some point, so this code will be unaffected.

(Having both is redundant, so given the choice, I prefer all of our boolean
flags are set to false as the default, most common case - so that suggests
calling it "disabled" internally and mapping it to AXEnabled on Mac only)

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

https://codereview.chromium.org/2106953005/diff/40001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:72: if (view_->HasFocus())
You shouldn't need this. Use NativeViewAccessibility::GetFocus() instead, which
returns a single object that's focused

[tapted, 2016-06-30 01:58:57]

lgtm, just some minor nits

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

https://codereview.chromium.org/2106953005/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:208: for (size_t i = 0; i <
arraysize(events); ++i) {
nit: no curlies (consistent with the others)

https://codereview.chromium.org/2106953005/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:214: void
NotifyMacEvent(ui::AXEvent event_type, gfx::AcceleratedWidget target) {
AcceleratedWidget -> NSView* (it's easier to read with the specific type, which
we can assume since this is mac-only)

https://codereview.chromium.org/2106953005/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:507: gfx::AcceleratedWidget
target =
nit: NSView*

https://codereview.chromium.org/2106953005/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:516:
NSAccessibilityPostNotification(
I'd go with

NotifyMacEvent(target, ui::AX_EVENT_VALUE_CHANGED)

it's a bit clearer what the intention is then (also it's shorter :)

[Patti Lor, 2016-06-30 05:29:53]

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

[Patti Lor, 2016-06-30 05:29:54]

PTAL sky for OWNER's review of ui/views/controls/textfield/textfield.cc.
Thanks!

https://codereview.chromium.org/2106953005/diff/40001/ui/accessibility/ax_enu...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2106953005/diff/40001/ui/accessibility/ax_enu...
ui/accessibility/ax_enums.idl:210: focused,
On 2016/06/29 16:24:10, dmazzoni wrote:
> We used to have "focused" as a state here, but we got rid of it on purpose
> because it was possible for two elements to both report they were focused at
the
> same time.
> 
> It's more foolproof to keep track of which element has focus (in the case of
> Views, for example, that's already easy) and compute the
> "focused" state dynamically.

Oh I see, thank you! Have removed it and used
NativeViewAccessibility::GetFocus() as you advised.

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

https://codereview.chromium.org/2106953005/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:330: [axAttributes
addObjectsFromArray:kTextfieldAttributes];
On 2016/06/29 16:24:11, dmazzoni wrote:
> Please add a comment like "fallthrough" so that it's clear you meant for this
to
> keep going with the switch and you're not accidentally missing a "break".

Done.

https://codereview.chromium.org/2106953005/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:422: // TODO(patricialor):
Check with dmazzoni@ to see if AX_STATE_ENABLED is still
On 2016/06/29 16:24:11, dmazzoni wrote:
> This is correct for now, no TODO needed. My plan is to get rid of ENABLED at
> some point, so this code will be unaffected.
> 
> (Having both is redundant, so given the choice, I prefer all of our boolean
> flags are set to false as the default, most common case - so that suggests
> calling it "disabled" internally and mapping it to AXEnabled on Mac only)
> 

Removed, thanks for clarifying!

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

https://codereview.chromium.org/2106953005/diff/40001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:72: if (view_->HasFocus())
On 2016/06/29 16:24:11, dmazzoni wrote:
> You shouldn't need this. Use NativeViewAccessibility::GetFocus() instead,
which
> returns a single object that's focused

Done.

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

https://codereview.chromium.org/2106953005/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:208: for (size_t i = 0; i <
arraysize(events); ++i) {
On 2016/06/30 01:58:57, tapted wrote:
> nit: no curlies (consistent with the others)

Done.

https://codereview.chromium.org/2106953005/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:214: void
NotifyMacEvent(ui::AXEvent event_type, gfx::AcceleratedWidget target) {
On 2016/06/30 01:58:57, tapted wrote:
> AcceleratedWidget -> NSView* (it's easier to read with the specific type,
which
> we can assume since this is mac-only)

Done.

https://codereview.chromium.org/2106953005/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:507: gfx::AcceleratedWidget
target =
On 2016/06/30 01:58:57, tapted wrote:
> nit: NSView*

Done.

https://codereview.chromium.org/2106953005/diff/60001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:516:
NSAccessibilityPostNotification(
On 2016/06/30 01:58:57, tapted wrote:
> I'd go with
> 
> NotifyMacEvent(target, ui::AX_EVENT_VALUE_CHANGED)
> 
> it's a bit clearer what the intention is then (also it's shorter :)

Done, thanks!

[Patti Lor, 2016-06-30 05:33:59]

Description was changed from

==========
Mac: Add new accessibility attributes for textfields (and some for all views).

This change adds support for the following NSAccessibility attributes:
NSAccessibilityEnabledAttribute
NSAccessibilityFocusedAttribute
NSAccessibilityInsertionPointLineNumberAttribute
NSAccessibilityNumberOfCharactersAttribute
NSAccessibilityPlaceholderValue
NSAccessibilitySelectedTextAttribute
NSAccessibilitySelectedTextrangeAttribute
NSAccessibilityVisibleCharacterRangeAttribute

ui::AX_ATTR_PLACEHOLDER was also not used outside of Blink, so support this
attribute for all views. A new state ui::AX_STATE_FOCUSED is also added (not to
be confused with ui::AX_STATE_FOCUSABLE).

Add tests for all of these attributes and plumb through textfield change
notifications for ui::AX_EVENT_TEXT_CHANGED, ui::AX_EVENT_VALUE_CHANGED, and
ui::AX_EVENT_TEXT_SELECTION_CHANGED.

BUG=610591
==========

to

==========
Mac: Add accessibility attributes for textfields, and AXEnabled for all views.

This change adds support for the following NSAccessibility attributes:
NSAccessibilityEnabledAttribute
NSAccessibilityFocusedAttribute
NSAccessibilityInsertionPointLineNumberAttribute
NSAccessibilityNumberOfCharactersAttribute
NSAccessibilityPlaceholderValue
NSAccessibilitySelectedTextAttribute
NSAccessibilitySelectedTextrangeAttribute
NSAccessibilityVisibleCharacterRangeAttribute

ui::AX_ATTR_PLACEHOLDER was also not used outside of Blink, so support this
attribute as well.

Add tests for all of these attributes and plumb through textfield change
notifications for ui::AX_EVENT_TEXT_CHANGED, ui::AX_EVENT_VALUE_CHANGED, and
ui::AX_EVENT_TEXT_SELECTION_CHANGED.

BUG=610591
==========

[sky, 2016-06-30 16:37:20]

LGTM

[Patti Lor, 2016-07-01 00:13:48]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2016-07-01 00:13:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2106953005/#ps80001
(title: "Address review comments.")

[commit-bot: I haz the power, 2016-07-01 00:14:10]

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-07-01 00:29:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-01 00:31:57]

Description was changed from

==========
Mac: Add accessibility attributes for textfields, and AXEnabled for all views.

This change adds support for the following NSAccessibility attributes:
NSAccessibilityEnabledAttribute
NSAccessibilityFocusedAttribute
NSAccessibilityInsertionPointLineNumberAttribute
NSAccessibilityNumberOfCharactersAttribute
NSAccessibilityPlaceholderValue
NSAccessibilitySelectedTextAttribute
NSAccessibilitySelectedTextrangeAttribute
NSAccessibilityVisibleCharacterRangeAttribute

ui::AX_ATTR_PLACEHOLDER was also not used outside of Blink, so support this
attribute as well.

Add tests for all of these attributes and plumb through textfield change
notifications for ui::AX_EVENT_TEXT_CHANGED, ui::AX_EVENT_VALUE_CHANGED, and
ui::AX_EVENT_TEXT_SELECTION_CHANGED.

BUG=610591
==========

to

==========
Mac: Add accessibility attributes for textfields, and AXEnabled for all views.

This change adds support for the following NSAccessibility attributes:
NSAccessibilityEnabledAttribute
NSAccessibilityFocusedAttribute
NSAccessibilityInsertionPointLineNumberAttribute
NSAccessibilityNumberOfCharactersAttribute
NSAccessibilityPlaceholderValue
NSAccessibilitySelectedTextAttribute
NSAccessibilitySelectedTextrangeAttribute
NSAccessibilityVisibleCharacterRangeAttribute

ui::AX_ATTR_PLACEHOLDER was also not used outside of Blink, so support this
attribute as well.

Add tests for all of these attributes and plumb through textfield change
notifications for ui::AX_EVENT_TEXT_CHANGED, ui::AX_EVENT_VALUE_CHANGED, and
ui::AX_EVENT_TEXT_SELECTION_CHANGED.

BUG=610591

Committed: https://crrev.com/c32a66d5bf5de90e99d220dafe3fff471419343c
Cr-Commit-Position: refs/heads/master@{#403370}
==========

[commit-bot: I haz the power, 2016-07-01 00:31:59]

Patchset 5 (id:??) landed as
https://crrev.com/c32a66d5bf5de90e99d220dafe3fff471419343c
Cr-Commit-Position: refs/heads/master@{#403370}