MacViews/a11y: Allow accessibility clients to focus and unfocus focusable Views.

MacViews/a11y: Allow accessibility clients to focus and unfocus focusable Views.

Currently, the `isAccessibilityFocused` a11y attribute on Views is reported as
unmodifiable, which differs from native UI elements on Mac.

This change will support allowing accessibility clients to focus and unfocus
Views controls which are reported as focusable using a new
NativeViewAccessibility::SetFocused() method and a new
ui::AX_ACTION_FLAGS_SET_UNFOCUSED. SetFocused() will not call
View::HandleAccessibilityAction() because all Views are potentially focusable.

BUG=610591
TEST=With #mac-views-webui-dialogs turned on in chrome:flags, open the XCode
Accessibility Inspector and bring up the HTTP Authentication dialog. Hover over
a control on it (e.g. the username Textfield) and press Cmd+F7 to lock onto it.
In the Accessibility Inspector, click on `isAccessibilityFocused` and check it
has a '(W)' next to it to indicate it's writable. A checkbox should appear at
the bottom which reflects its current focused state. Check it and click
Set Value to focus the control, then uncheck it and click Set Value to unfocus
the control.

Committed: https://crrev.com/f6a5dc8e9e2b639d9d4ce96f8bf1bce88aeef3f2
Cr-Commit-Position: refs/heads/master@{#432142}

[Patti Lor, 2016-11-10 03:28:52]

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

[commit-bot: I haz the power, 2016-11-10 03:29:03]

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-11-10 04:14:31]

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

[commit-bot: I haz the power, 2016-11-10 04:14:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2016-11-10 05:24:08]

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

[commit-bot: I haz the power, 2016-11-10 05:24:22]

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-11-10 06:22:46]

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

[commit-bot: I haz the power, 2016-11-10 06:22:47]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-11-10 06:26:01]

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

[Patti Lor, 2016-11-10 06:26:01]

Hi Trent, PTAL.

[tapted, 2016-11-11 00:36:26]

https://codereview.chromium.org/2490073002/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_delegate.h (right):

https://codereview.chromium.org/2490073002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:99: virtual bool
SetFocused(const bool focused) = 0;
nit: remove const

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

https://codereview.chromium.org/2490073002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:386: ui::AX_STATE_FOCUSABLE))
nit: needs curlies now that the block below goes over 2 lines, but.. can we
instead just remove the `if` and always return?  (otherwise, just `return YES;`
below)

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

https://codereview.chromium.org/2490073002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:74: if
(view_->IsFocusable())
would IsAccessibilityFocusable be more appropriate here?

I lean towards, "yes", but only if it works, even when Full Keyboard Access
isn't enabled in preferences. (e.g. normally buttons don't get focus in regular
tab traversal without full keyboard access, but requesting focus may still give
it focus in that case)

https://codereview.chromium.org/2490073002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:229: else
optional: I'd maybe make this `else if view->HasFocus()` and remove the early
exit above. It's clearer that way ClearFocus() isn't accidentally called when a
different view has focus

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

https://codereview.chromium.org/2490073002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_unittest.cc:93:
std::unique_ptr<Widget> widget = base::MakeUnique<Widget>();
NativeViewAccessibilityTest already has a |widget_| member - can we use that (or
explain why not in the comment above). E.g. I think it would be nicer to do

widget_->GetContentsView()->AddChildView(button_);
rather than
widget_->SetContentsView(button_);

in SetUp(), assuming nothing else breaks

(maybe also change TYPE_WINDOW_FRAMELESS if you need to).

https://codereview.chromium.org/2490073002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_unittest.cc:101:
button_->SetFocusBehavior(ClientView::FocusBehavior::ALWAYS);
ClientView -> View

https://codereview.chromium.org/2490073002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_unittest.cc:117:
widget->GetContentsView()->RemoveChildView(button_);
this shouldn't be necessary... And I'm not sure that button_ is a unique_ptr.
Perhaps it's a lifetime problem with things done in TearDown, and re-using
|widget_| will help.

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

https://codereview.chromium.org/2490073002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:324:
view->SetFocusBehavior(ClientView::FocusBehavior::ALWAYS);
ClientView -> View

https://codereview.chromium.org/2490073002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:326: EXPECT_EQ(NO,
[AttributeValueAtMidpoint(NSAccessibilityFocusedAttribute)
nit: I think I'd prefer EXPECT_FALSE rather than EXPECT_EQ(NO.

https://codereview.chromium.org/2490073002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:332: EXPECT_EQ(YES,
[AttributeValueAtMidpoint(NSAccessibilityFocusedAttribute)
nit: EXPECT_TRUE

[Patti Lor, 2016-11-11 04:13:24]

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

[commit-bot: I haz the power, 2016-11-11 04:13:36]

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-11-11 04:55:25]

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

[commit-bot: I haz the power, 2016-11-11 04:55:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2016-11-14 02:34:39]

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

[commit-bot: I haz the power, 2016-11-14 02:34:45]

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-11-14 03:43:49]

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

[commit-bot: I haz the power, 2016-11-14 03:43:49]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-11-14 03:51:22]

Hi Trent, PTAL!

https://codereview.chromium.org/2490073002/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_delegate.h (right):

https://codereview.chromium.org/2490073002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:99: virtual bool
SetFocused(const bool focused) = 0;
On 2016/11/11 00:36:25, tapted wrote:
> nit: remove const

Done.

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

https://codereview.chromium.org/2490073002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:386: ui::AX_STATE_FOCUSABLE))
On 2016/11/11 00:36:25, tapted wrote:
> nit: needs curlies now that the block below goes over 2 lines, but.. can we
> instead just remove the `if` and always return?  (otherwise, just `return
YES;`
> below)

Oops, yeah this is definitely a mistake. Fixed now. Thanks!

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

https://codereview.chromium.org/2490073002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:74: if
(view_->IsFocusable())
On 2016/11/11 00:36:25, tapted wrote:
> would IsAccessibilityFocusable be more appropriate here?
> 
> I lean towards, "yes", but only if it works, even when Full Keyboard Access
> isn't enabled in preferences. (e.g. normally buttons don't get focus in
regular
> tab traversal without full keyboard access, but requesting focus may still
give
> it focus in that case)

Oh, I hadn't thought about that, thanks for pointing it out. I just did a quick
test and it looks like Cocoa controls do what you've described in your example,
so have changed it to use IsAccessibilityFocusable().

https://codereview.chromium.org/2490073002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:229: else
On 2016/11/11 00:36:25, tapted wrote:
> optional: I'd maybe make this `else if view->HasFocus()` and remove the early
> exit above. It's clearer that way ClearFocus() isn't accidentally called when
a
> different view has focus

Done, thanks!

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

https://codereview.chromium.org/2490073002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_unittest.cc:93:
std::unique_ptr<Widget> widget = base::MakeUnique<Widget>();
On 2016/11/11 00:36:25, tapted wrote:
> NativeViewAccessibilityTest already has a |widget_| member - can we use that
(or
> explain why not in the comment above). E.g. I think it would be nicer to do
> 
> widget_->GetContentsView()->AddChildView(button_);
> rather than
> widget_->SetContentsView(button_);
> 
> in SetUp(), assuming nothing else breaks
> 
> (maybe also change TYPE_WINDOW_FRAMELESS if you need to).

Ah, thank you! I think it got all mixed up in a rebase. Fixed now :)

https://codereview.chromium.org/2490073002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_unittest.cc:101:
button_->SetFocusBehavior(ClientView::FocusBehavior::ALWAYS);
On 2016/11/11 00:36:25, tapted wrote:
> ClientView -> View

Done.

https://codereview.chromium.org/2490073002/diff/20001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_unittest.cc:117:
widget->GetContentsView()->RemoveChildView(button_);
On 2016/11/11 00:36:25, tapted wrote:
> this shouldn't be necessary... And I'm not sure that button_ is a unique_ptr.
> Perhaps it's a lifetime problem with things done in TearDown, and re-using
> |widget_| will help.

Yep, not necessary - originally had it that way in an older version of this CL.
Removed now.

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

https://codereview.chromium.org/2490073002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:324:
view->SetFocusBehavior(ClientView::FocusBehavior::ALWAYS);
On 2016/11/11 00:36:25, tapted wrote:
> ClientView -> View

Done.

https://codereview.chromium.org/2490073002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:326: EXPECT_EQ(NO,
[AttributeValueAtMidpoint(NSAccessibilityFocusedAttribute)
On 2016/11/11 00:36:25, tapted wrote:
> nit: I think I'd prefer EXPECT_FALSE rather than EXPECT_EQ(NO. 

Done.

https://codereview.chromium.org/2490073002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:332: EXPECT_EQ(YES,
[AttributeValueAtMidpoint(NSAccessibilityFocusedAttribute)
On 2016/11/11 00:36:25, tapted wrote:
> nit: EXPECT_TRUE

Done.

[tapted, 2016-11-14 06:21:22]

lgtm

[Patti Lor, 2016-11-14 22:00:39]

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

[Patti Lor, 2016-11-14 22:00:40]

Hi dmazzoni, PTAL. Thank you!

[dmazzoni, 2016-11-14 23:13:38]

lgtm

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

https://codereview.chromium.org/2490073002/diff/60001/ui/accessibility/ax_enu...
ui/accessibility/ax_enums.idl:283: set_unfocused
Unused?

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

https://codereview.chromium.org/2490073002/diff/60001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:74: if
(view_->IsAccessibilityFocusable())
This is probably a good change, but just a warning
that it may break something. We made everything in
"views" focusable as a hack years ago when we switched
from native UI to views, because so many things were
not marked focusable that should have been.

I'm guessing we're in much better shape now, but
I wouldn't be surprised if a few things are not
marked focusable that should be.

Would it work to land without this change in M56,
then fix it as a follow-up in M57 so we have lots
of time to catch regressions?

[Patti Lor, 2016-11-15 01:00:04]

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

[commit-bot: I haz the power, 2016-11-15 01:01:02]

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-11-15 02:22:34]

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

[commit-bot: I haz the power, 2016-11-15 02:22:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2016-11-15 02:37:57]

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

[commit-bot: I haz the power, 2016-11-15 02:38:36]

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-11-15 04:13:35]

Thanks!

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

https://codereview.chromium.org/2490073002/diff/60001/ui/accessibility/ax_enu...
ui/accessibility/ax_enums.idl:283: set_unfocused
On 2016/11/14 23:13:37, dmazzoni wrote:
> Unused?

Done, thanks.

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

https://codereview.chromium.org/2490073002/diff/60001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:74: if
(view_->IsAccessibilityFocusable())
On 2016/11/14 23:13:37, dmazzoni wrote:
> This is probably a good change, but just a warning
> that it may break something. We made everything in
> "views" focusable as a hack years ago when we switched
> from native UI to views, because so many things were
> not marked focusable that should have been.
> 
> I'm guessing we're in much better shape now, but
> I wouldn't be surprised if a few things are not
> marked focusable that should be.
> 
> Would it work to land without this change in M56,
> then fix it as a follow-up in M57 so we have lots
> of time to catch regressions?

That sounds OK to me - either way it will be an improvement in the current
behaviour since it doesn't work at all at the moment. Have split out this change
into https://codereview.chromium.org/2506483002/ for now and won't try to land
this til after Nov 17, which seems to be when m56 splits off. Is there a way we
can pro-actively try and catch these cases instead of waiting for a bug to be
reported? e.g. go through all View subclasses, or something like that?

[commit-bot: I haz the power, 2016-11-15 04:26:10]

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

[commit-bot: I haz the power, 2016-11-15 04:26:10]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-11-15 06:46:05]

On 2016/11/15 04:13:35, Patti Lor wrote:
> That sounds OK to me - either way it will be an improvement in the current
> behaviour since it doesn't work at all at the moment. Have split out this
change
> into https://codereview.chromium.org/2506483002/ for now and won't try to land
> this til after Nov 17, which seems to be when m56 splits off. Is there a way
we
> can pro-actively try and catch these cases instead of waiting for a bug to be
> reported? e.g. go through all View subclasses, or something like that?

Yeah, it might be worth looking for any View that has a click handler or an
accessibility role that implies it should be focusable, but it isn't focusable
or accessibility focusable.

Would be nice as a presubmit check too, using a clang plugin:

https://chromium.googlesource.com/chromium/src/+/master/docs/writing_clang_pl...

...though it sounds like clang plugins are difficult to maintain.

[Patti Lor, 2016-11-15 07:43:05]

On 2016/11/15 06:46:05, dmazzoni wrote:
> On 2016/11/15 04:13:35, Patti Lor wrote:
> > That sounds OK to me - either way it will be an improvement in the current
> > behaviour since it doesn't work at all at the moment. Have split out this
> change
> > into https://codereview.chromium.org/2506483002/ for now and won't try to
land
> > this til after Nov 17, which seems to be when m56 splits off. Is there a way
> we
> > can pro-actively try and catch these cases instead of waiting for a bug to
be
> > reported? e.g. go through all View subclasses, or something like that?
> 
> Yeah, it might be worth looking for any View that has a click handler or an
> accessibility role that implies it should be focusable, but it isn't focusable
> or accessibility focusable.
> 
> Would be nice as a presubmit check too, using a clang plugin:
> 
>
https://chromium.googlesource.com/chromium/src/+/master/docs/writing_clang_pl...
> 
> ...though it sounds like clang plugins are difficult to maintain.

I'll opt for the former option and try do a skim over the Views subclasses -
hopefully it's a manageable number. Will update the other CL (that is,
https://codereview.chromium.org/2506483002/), if I find anything.

[Patti Lor, 2016-11-15 07:43:27]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2016-11-15 07:43:27]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2490073002/#ps100001
(title: "Fix test.")

[commit-bot: I haz the power, 2016-11-15 07:43:42]

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-11-15 07:48:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-15 07:49:41]

Description was changed from

==========
MacViews/a11y: Allow accessibility clients to focus and unfocus focusable Views.

Currently, the `isAccessibilityFocused` a11y attribute on Views is reported as
unmodifiable, which differs from native UI elements on Mac.

This change will support allowing accessibility clients to focus and unfocus
Views controls which are reported as focusable using a new
NativeViewAccessibility::SetFocused() method and a new
ui::AX_ACTION_FLAGS_SET_UNFOCUSED. SetFocused() will not call
View::HandleAccessibilityAction() because all Views are potentially focusable.

BUG=610591
TEST=With #mac-views-webui-dialogs turned on in chrome:flags, open the XCode
Accessibility Inspector and bring up the HTTP Authentication dialog. Hover over
a control on it (e.g. the username Textfield) and press Cmd+F7 to lock onto it.
In the Accessibility Inspector, click on `isAccessibilityFocused` and check it
has a '(W)' next to it to indicate it's writable. A checkbox should appear at
the bottom which reflects its current focused state. Check it and click
Set Value to focus the control, then uncheck it and click Set Value to unfocus
the control.
==========

to

==========
MacViews/a11y: Allow accessibility clients to focus and unfocus focusable Views.

Currently, the `isAccessibilityFocused` a11y attribute on Views is reported as
unmodifiable, which differs from native UI elements on Mac.

This change will support allowing accessibility clients to focus and unfocus
Views controls which are reported as focusable using a new
NativeViewAccessibility::SetFocused() method and a new
ui::AX_ACTION_FLAGS_SET_UNFOCUSED. SetFocused() will not call
View::HandleAccessibilityAction() because all Views are potentially focusable.

BUG=610591
TEST=With #mac-views-webui-dialogs turned on in chrome:flags, open the XCode
Accessibility Inspector and bring up the HTTP Authentication dialog. Hover over
a control on it (e.g. the username Textfield) and press Cmd+F7 to lock onto it.
In the Accessibility Inspector, click on `isAccessibilityFocused` and check it
has a '(W)' next to it to indicate it's writable. A checkbox should appear at
the bottom which reflects its current focused state. Check it and click
Set Value to focus the control, then uncheck it and click Set Value to unfocus
the control.

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

[commit-bot: I haz the power, 2016-11-15 07:49:42]

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