MacViews/a11y: Disable most text-specific attributes for protected textfields.

MacViews/a11y: Disable most text-specific attributes for protected textfields.

Cocoa doesn't support these attributes in NSSecureTextfields, so match that with
MacViews.

BUG=692380
Review-Url: https://codereview.chromium.org/2706743002
Cr-Commit-Position: refs/heads/master@{#453418}
Committed: https://chromium.googlesource.com/chromium/src/+/54b3acd0692e87dc4d33aa2ffb66ae7697a16f79

[Patti Lor, 2017-02-20 03:30:31]

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

[commit-bot: I haz the power, 2017-02-20 03:30:50]

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, 2017-02-20 03:55:06]

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

[commit-bot: I haz the power, 2017-02-20 03:55:07]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-02-21 03:36:51]

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

[commit-bot: I haz the power, 2017-02-21 03:36:57]

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, 2017-02-21 04:04:30]

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

[commit-bot: I haz the power, 2017-02-21 04:04:31]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-02-21 04:19:08]

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

[Patti Lor, 2017-02-21 04:19:09]

PTAL, thanks!

[tapted, 2017-02-21 04:28:46]

Is it possible to test? It's worth considering doing something similar to what's
done in BridgedNativeWidgetTest::InstallTextField(..) -- there we initialize a
dummy Cocoa textfield and compare its attributes and behaviour to a
views::Textfield. Can we do something like that in
NativeWidgetMacAccessibilityTest ?

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

https://codereview.chromium.org/2706743002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:594: return nil;
nit: blank line after

https://codereview.chromium.org/2706743002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:602: return nil;
nit: blank line after (for the 3 below I wouldn't bother -- it's to break up the
early exit from other stuff, but if the 'other stuff' is just a return we don't
normally worry about it)

[Patti Lor, 2017-02-23 02:00:28]

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

[commit-bot: I haz the power, 2017-02-23 02:01: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, 2017-02-23 02:15:20]

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

[commit-bot: I haz the power, 2017-02-23 02:16:13]

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, 2017-02-23 02:37:12]

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

[commit-bot: I haz the power, 2017-02-23 02:37:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Patti Lor, 2017-02-23 03:46:51]

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

[commit-bot: I haz the power, 2017-02-23 03:47:27]

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, 2017-02-23 04:19:45]

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

[commit-bot: I haz the power, 2017-02-23 04:19:46]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-02-23 04:24:18]

Hi Trent, PTAL. I had originally thought we could test View vs Cocoa by checking
that Views supports a subset of the Cocoa attributes (since there are a couple
of obscure ones we haven't yet implemented), but turns out Cocoa acts kinda
inconsistent with that (see comment). It might be because we're using the legacy
a11y APIs?

I also prevented the value of protected views::Textfields from being edited (it
doesn't look like it works anyway), but both Cocoa and Views (before this patch)
show the value being "editable" but editing it for either doesn't do anything.

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

https://codereview.chromium.org/2706743002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:594: return nil;
On 2017/02/21 04:28:46, tapted wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/2706743002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:602: return nil;
On 2017/02/21 04:28:46, tapted wrote:
> nit: blank line after (for the 3 below I wouldn't bother -- it's to break up
the
> early exit from other stuff, but if the 'other stuff' is just a return we
don't
> normally worry about it)

Done.

[tapted, 2017-02-23 06:20:34]

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

https://codereview.chromium.org/2706743002/diff/80001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:422: else if
(node_->GetData().HasStateFlag(ui::AX_STATE_PROTECTED))
nit: no else after return, also should this check be done first?

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

https://codereview.chromium.org/2706743002/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:498: // Inspector
but aren't reported by -accessibilityAttributeNames.
Is there any use in something like:

if (base::mac::IsOSAtLeast_10.10()) {
  EXPECT_TRUE([cocoa_secure_textfield
isAccessibilitySelectorAllowed:@selector(accessibilityPlaceholderValue)]);
  EXPECT_TRUE([cocoa_secure_textfield
isAccessibilitySelectorAllowed:@selector(accessibilityValue)]);
}

?

https://codereview.chromium.org/2706743002/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:509:
EXPECT_FALSE([cocoa_attributes containsObject:attribute_name]);
so what _is_ in cocoa_attributes?


It's possible with this changing API we're going to get a lot of
OS-version-specific behaviour.. argh. I guess we need to consider that we may
have a period of mess where we need some way to map between the two APIs so that
we can run the same a11y code on 10.9 and 10.10+

i.e. perhaps AppKit on 10.10+ have already started assuming that nothing will
ever query its native controls using the old APIs, so they do nothing.

I'm not sure it's worth maintaining some kind of mapping if we just need it for
tests.

[Patti Lor, 2017-02-24 02:28:40]

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

[commit-bot: I haz the power, 2017-02-24 02:29:37]

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, 2017-02-24 02:47:40]

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

[commit-bot: I haz the power, 2017-02-24 02:47:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Patti Lor, 2017-02-24 03:09:10]

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

[commit-bot: I haz the power, 2017-02-24 03:09:41]

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, 2017-02-24 03:35:48]

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

[commit-bot: I haz the power, 2017-02-24 03:35:49]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-02-24 03:52:52]

Patchset #6 (id:100001) has been deleted

[Patti Lor, 2017-02-24 04:10:47]

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

[commit-bot: I haz the power, 2017-02-24 04:11:01]

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

[Patti Lor, 2017-02-24 04:35:25]

Patchset #6 (id:120001) has been deleted

[Patti Lor, 2017-02-24 04:36:14]

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

https://codereview.chromium.org/2706743002/diff/80001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:422: else if
(node_->GetData().HasStateFlag(ui::AX_STATE_PROTECTED))
On 2017/02/23 06:20:34, tapted wrote:
> nit: no else after return, also should this check be done first?

The else is a no-op, so I just left it out. Is it better to have it there and
explicitly say so?

Have switched the checks around though, thanks :)

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

https://codereview.chromium.org/2706743002/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:498: // Inspector
but aren't reported by -accessibilityAttributeNames.
On 2017/02/23 06:20:34, tapted wrote:
> Is there any use in something like:
> 
> if (base::mac::IsOSAtLeast_10.10()) {
>   EXPECT_TRUE([cocoa_secure_textfield
> isAccessibilitySelectorAllowed:@selector(accessibilityPlaceholderValue)]);
>   EXPECT_TRUE([cocoa_secure_textfield
> isAccessibilitySelectorAllowed:@selector(accessibilityValue)]);
> }
> 
> ?

Yep! That works, thanks for the suggestion and the help offline.

https://codereview.chromium.org/2706743002/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:509:
EXPECT_FALSE([cocoa_attributes containsObject:attribute_name]);
On 2017/02/23 06:20:34, tapted wrote:
> so what _is_ in cocoa_attributes?
> 
> 
> It's possible with this changing API we're going to get a lot of
> OS-version-specific behaviour.. argh. I guess we need to consider that we may
> have a period of mess where we need some way to map between the two APIs so
that
> we can run the same a11y code on 10.9 and 10.10+
> 
> i.e. perhaps AppKit on 10.10+ have already started assuming that nothing will
> ever query its native controls using the old APIs, so they do nothing.
> 
> I'm not sure it's worth maintaining some kind of mapping if we just need it
for
> tests.

Erm, pretty much everything is still in |cocoa_attributes| (at least for
password fields). I think "AXTitle" is missing and "AXSubrole" (along with the
Value and PlaceholderValue). But yeah, not really sure what to do about that
either, but also agree it's not worth it for tests only. :(

[commit-bot: I haz the power, 2017-02-24 04:41:53]

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

[commit-bot: I haz the power, 2017-02-24 04:41:54]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-02-24 05:23:42]

lgtm

https://codereview.chromium.org/2706743002/diff/140001/base/mac/sdk_forward_d...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/2706743002/diff/140001/base/mac/sdk_forward_d...
base/mac/sdk_forward_declarations.h:160: @interface NSView (Yosemite_2_SDK)
I think the guards say this came in for 10.10.3, so perhaps Yosemite_3_SDK, and
I think we should change the one on line 156 to match (but not worry about the
ones for other point release versions)

https://codereview.chromium.org/2706743002/diff/140001/ui/accessibility/platf...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):

https://codereview.chromium.org/2706743002/diff/140001/ui/accessibility/platf...
ui/accessibility/platform/ax_platform_node_mac.mm:423: else if
(node_->GetData().role == ui::AX_ROLE_TAB) {
nit: remove "else" (since the prior `if` always returns it has no effect)

[Patti Lor, 2017-02-24 05:52:23]

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

[commit-bot: I haz the power, 2017-02-24 05:52:35]

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, 2017-02-24 06:16:06]

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

[commit-bot: I haz the power, 2017-02-24 06:16:08]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-02-26 23:16:37]

Thanks for the review!

https://codereview.chromium.org/2706743002/diff/140001/base/mac/sdk_forward_d...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/2706743002/diff/140001/base/mac/sdk_forward_d...
base/mac/sdk_forward_declarations.h:160: @interface NSView (Yosemite_2_SDK)
On 2017/02/24 05:23:42, tapted wrote:
> I think the guards say this came in for 10.10.3, so perhaps Yosemite_3_SDK,
and
> I think we should change the one on line 156 to match (but not worry about the
> ones for other point release versions)

Done.

https://codereview.chromium.org/2706743002/diff/140001/ui/accessibility/platf...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):

https://codereview.chromium.org/2706743002/diff/140001/ui/accessibility/platf...
ui/accessibility/platform/ax_platform_node_mac.mm:423: else if
(node_->GetData().role == ui::AX_ROLE_TAB) {
On 2017/02/24 05:23:42, tapted wrote:
> nit: remove "else" (since the prior `if` always returns it has no effect)

Done.

[Patti Lor, 2017-02-26 23:16:42]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2017-02-26 23:16:42]

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/2706743002/#ps160001
(title: "Review comments.")

[Patti Lor, 2017-02-26 23:16:47]

The CQ bit was unchecked by patricialor@chromium.org

[Patti Lor, 2017-02-26 23:18:33]

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

[Patti Lor, 2017-02-26 23:18:33]

Hi dmazzoni, PTAL, thanks!

[dmazzoni, 2017-02-27 20:00:50]

lgtm

[Patti Lor, 2017-02-27 23:43:04]

The CQ bit was checked by patricialor@chromium.org

[commit-bot: I haz the power, 2017-02-27 23:43:47]

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, 2017-02-28 00:39:55]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488238984831020,
"parent_rev": "d37bf2eb3912ef5c89a512f294ee3e4b8546e653", "commit_rev":
"54b3acd0692e87dc4d33aa2ffb66ae7697a16f79"}

[commit-bot: I haz the power, 2017-02-28 00:40:36]

Description was changed from

==========
MacViews/a11y: Disable most text-specific attributes for protected textfields.

Cocoa doesn't support these attributes in NSSecureTextfields, so match that with
MacViews.

BUG=692380
==========

to

==========
MacViews/a11y: Disable most text-specific attributes for protected textfields.

Cocoa doesn't support these attributes in NSSecureTextfields, so match that with
MacViews.

BUG=692380

Review-Url: https://codereview.chromium.org/2706743002
Cr-Commit-Position: refs/heads/master@{#453418}
Committed:
https://chromium.googlesource.com/chromium/src/+/54b3acd0692e87dc4d33aa2ffb66...
==========

[commit-bot: I haz the power, 2017-02-28 00:40:38]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/54b3acd0692e87dc4d33aa2ffb66...