MacViews: Accessibility bridge

MacViews: Accessibility bridge

Implement basic accessibility hierarchy support for MacViews, bridging the
View hierarchy to a hierarchy of Objective-C objects that implement the
NSAccessibility informal protocol.
AXPlatformNode is introduced as the accessibility abstraction that will be
used for views and browser content, for both Mac and Windows. This initial
patch includes the basic views on Mac implementation.

BUG=396137
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286768

[Andre, 2014-07-28 22:54:45]

dmazzoni, here's the rough draft we talked about.

[dmazzoni, 2014-07-29 06:17:40]

You suggested I land the partial patch I was working on first, but I've got a
lot on my plate this week and I don't think I'll have time before the great
migration. It looks like you've incorporated all of my ideas, and as long as
this doesn't break the Windows port I see no reason not to just review and land
your change now since it's ready. I'll merge my other changes on top of it
instead.

Reviewing this now...

[dmazzoni, 2014-07-29 06:19:13]

Can you just merge 423513005 into this change and reupload based on master?

https://codereview.chromium.org/420653003/diff/60001/ui/accessibility/accessi...
File ui/accessibility/accessibility.gyp (right):

https://codereview.chromium.org/420653003/diff/60001/ui/accessibility/accessi...
ui/accessibility/accessibility.gyp:65: #
'../../third_party/iaccessible2/iaccessible2.gyp:iaccessible2',
delete this

[dmazzoni, 2014-07-29 06:35:15]

https://codereview.chromium.org/420653003/diff/60001/ui/accessibility/platfor...
File ui/accessibility/platform/ax_platform_node.cc (right):

https://codereview.chromium.org/420653003/diff/60001/ui/accessibility/platfor...
ui/accessibility/platform/ax_platform_node.cc:38: gfx::Rect bounds =
delegate_->GetData()->location;
Check delegate_ for NULL everywhere

https://codereview.chromium.org/420653003/diff/60001/ui/accessibility/platfor...
File ui/accessibility/platform/ax_platform_node.h (right):

https://codereview.chromium.org/420653003/diff/60001/ui/accessibility/platfor...
ui/accessibility/platform/ax_platform_node.h:25: AXRole GetRole() const;
I don't want these to be part of the public interface that's possible to call
from outside of ui/accessibility/platform.

If this was all C++ code these could be protected or we could use friends - but
since you need to call these from the Cocoa object I think it's best to define
AXPlatformNodeBase and put the helpers there. That way AXPlatformNode is really
simple and only contains the public interface.

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

https://codereview.chromium.org/420653003/diff/60001/ui/accessibility/platfor...
ui/accessibility/platform/ax_platform_node_mac.mm:229: return @[
Cool, no wonder I didn't know about this, I just looked it up and it's only been
part of Obj-C for a short time. Nice.

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

https://codereview.chromium.org/420653003/diff/60001/ui/views/accessibility/n...
ui/views/accessibility/native_view_accessibility.cc:63: return
view_->child_at(index)->GetNativeViewAccessible();
Sanity-check this and return NULL if it's out of bounds. Log a warning but
nothing more.

Accessibility calls come from out-of-process, so it's possible that an external
accessibility client queries the number of children, then before they have a
chance to iterate, the number of children changes.

(This can't happen with the current Mac impl since it builds the vector all at
once, but I want to reuse this code for all platforms.)

https://codereview.chromium.org/420653003/diff/60001/ui/views/accessibility/n...
File ui/views/accessibility/native_view_accessibility.h (right):

https://codereview.chromium.org/420653003/diff/60001/ui/views/accessibility/n...
ui/views/accessibility/native_view_accessibility.h:43:
NativeViewAccessibility(View* view);
This will break Windows - the constructor has to take no arguments.

[Andre, 2014-07-29 21:58:56]

https://codereview.chromium.org/420653003/diff/60001/ui/accessibility/accessi...
File ui/accessibility/accessibility.gyp (right):

https://codereview.chromium.org/420653003/diff/60001/ui/accessibility/accessi...
ui/accessibility/accessibility.gyp:65: #
'../../third_party/iaccessible2/iaccessible2.gyp:iaccessible2',
On 2014/07/29 06:19:12, dmazzoni wrote:
> delete this

Done.

https://codereview.chromium.org/420653003/diff/60001/ui/accessibility/platfor...
File ui/accessibility/platform/ax_platform_node.cc (right):

https://codereview.chromium.org/420653003/diff/60001/ui/accessibility/platfor...
ui/accessibility/platform/ax_platform_node.cc:38: gfx::Rect bounds =
delegate_->GetData()->location;
On 2014/07/29 06:35:14, dmazzoni wrote:
> Check delegate_ for NULL everywhere

Done.

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

https://codereview.chromium.org/420653003/diff/60001/ui/views/accessibility/n...
ui/views/accessibility/native_view_accessibility.cc:63: return
view_->child_at(index)->GetNativeViewAccessible();
On 2014/07/29 06:35:15, dmazzoni wrote:
> Sanity-check this and return NULL if it's out of bounds. Log a warning but
> nothing more.
> 
> Accessibility calls come from out-of-process, so it's possible that an
external
> accessibility client queries the number of children, then before they have a
> chance to iterate, the number of children changes.
> 
> (This can't happen with the current Mac impl since it builds the vector all at
> once, but I want to reuse this code for all platforms.)
> 

Done.

https://codereview.chromium.org/420653003/diff/60001/ui/views/accessibility/n...
File ui/views/accessibility/native_view_accessibility.h (right):

https://codereview.chromium.org/420653003/diff/60001/ui/views/accessibility/n...
ui/views/accessibility/native_view_accessibility.h:43:
NativeViewAccessibility(View* view);
On 2014/07/29 06:35:15, dmazzoni wrote:
> This will break Windows - the constructor has to take no arguments.

Done.

[dmazzoni, 2014-07-29 23:09:31]

+dtseng

This is an FYI if you want to give feedback

[Andre, 2014-07-29 23:14:44]

https://codereview.chromium.org/420653003/diff/80001/ui/views/accessibility/n...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/420653003/diff/80001/ui/views/accessibility/n...
ui/views/accessibility/native_view_accessibility.cc:78: return
view_->GetWidget()->GetNativeView();
I went with this for now and deleted GetNativeViewAccessible from Widget,
because we will need Window specific code not Aura.

[dmazzoni, 2014-07-29 23:20:36]

Looking pretty good, but please upload the full change relative to master rather
than just the part you modified off of my half-finished change.

[Andre, 2014-07-29 23:53:50]

On 2014/07/29 23:20:36, dmazzoni wrote:
> Looking pretty good, but please upload the full change relative to master
rather
> than just the part you modified off of my half-finished change.

Rebased off master in patch 5.

[tapted, 2014-07-29 23:58:21]

[drive-by] change looks really nice :)  - just some comments from a quick scan.
Make sure you flesh out the CL description before landing too.

https://codereview.chromium.org/420653003/diff/120001/ui/gfx/native_widget_ty...
File ui/gfx/native_widget_types.h (right):

https://codereview.chromium.org/420653003/diff/120001/ui/gfx/native_widget_ty...
ui/gfx/native_widget_types.h:151: typedef id NativeViewAccessible;
for mac, could this just be `typedef NativeView NativeViewAccessible;`?

https://codereview.chromium.org/420653003/diff/120001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/420653003/diff/120001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:296: // NSAccessibility
nit: // NSAccessibility informal protocol implementation.

[Andre, 2014-07-30 00:16:51]

On 2014/07/29 23:58:21, tapted wrote:
> [drive-by] change looks really nice :)  - just some comments from a quick
scan.
> Make sure you flesh out the CL description before landing too.
Thanks, done.

> 
>
https://codereview.chromium.org/420653003/diff/120001/ui/gfx/native_widget_ty...
> File ui/gfx/native_widget_types.h (right):
> 
>
https://codereview.chromium.org/420653003/diff/120001/ui/gfx/native_widget_ty...
> ui/gfx/native_widget_types.h:151: typedef id NativeViewAccessible;
> for mac, could this just be `typedef NativeView NativeViewAccessible;`?
> 
>
https://codereview.chromium.org/420653003/diff/120001/ui/views/cocoa/bridged_...
> File ui/views/cocoa/bridged_content_view.mm (right):
> 
>
https://codereview.chromium.org/420653003/diff/120001/ui/views/cocoa/bridged_...
> ui/views/cocoa/bridged_content_view.mm:296: // NSAccessibility
> nit: // NSAccessibility informal protocol implementation.

[Andre, 2014-07-30 00:18:01]

https://codereview.chromium.org/420653003/diff/120001/ui/gfx/native_widget_ty...
File ui/gfx/native_widget_types.h (right):

https://codereview.chromium.org/420653003/diff/120001/ui/gfx/native_widget_ty...
ui/gfx/native_widget_types.h:151: typedef id NativeViewAccessible;
On 2014/07/29 23:58:21, tapted wrote:
> for mac, could this just be `typedef NativeView NativeViewAccessible;`?

I don't think so. NativeViewAccessible is any Obj-C object that implements the
NSAccessibility informal protocol. AXPlatformNodeCocoa is not a subclass of
NSView.

https://codereview.chromium.org/420653003/diff/120001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/420653003/diff/120001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:296: // NSAccessibility
On 2014/07/29 23:58:21, tapted wrote:
> nit: // NSAccessibility informal protocol implementation.

Done.

[dmazzoni, 2014-07-30 05:53:12]

This is a great first step! It doesn't have to be perfect, if this passes
trybots and doesn't break anything, let's land it and go from here.

https://codereview.chromium.org/420653003/diff/210001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/420653003/diff/210001/content/content_browser...
content/content_browser.gypi:1569:
'../ui/accessibility/accessibility.gyp:accessibility_platform',
nit: sort this

https://codereview.chromium.org/420653003/diff/210001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_base.h (right):

https://codereview.chromium.org/420653003/diff/210001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.h:17: class AX_EXPORT
AXPlatformNodeBase {
I imagined this the other way around - AXPlatformNodeBase would be a subclass of
AXPlatformNode.

Then only AXPlatformNode needs to be exported.

I was basing this off of the pattern used in content:

content/public/browser/render_widget_host_view.h

content/browser/renderer_host/render_widget_host_view_base.h

content/browser/renderer_host/render_widget_host_view_mac.h
content/browser/renderer_host/render_widget_host_view_aura.h

https://codereview.chromium.org/420653003/diff/210001/ui/gfx/native_widget_ty...
File ui/gfx/native_widget_types.h (right):

https://codereview.chromium.org/420653003/diff/210001/ui/gfx/native_widget_ty...
ui/gfx/native_widget_types.h:146: typedef void* NativeViewAccessible;
Why wouldn't this be the same as OS_MACOSX? Is there no "id" in iOS?

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

https://codereview.chromium.org/420653003/diff/210001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:64: if (index >=
view_->child_count())
Just to be safe,  return null if index < 0 also, since |index| might come
directly from an untrusted third-party client.

[dmazzoni, 2014-07-30 05:53:20]

lgtm

[Andre, 2014-07-30 18:50:16]

https://codereview.chromium.org/420653003/diff/210001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/420653003/diff/210001/content/content_browser...
content/content_browser.gypi:1569:
'../ui/accessibility/accessibility.gyp:accessibility_platform',
On 2014/07/30 05:53:12, dmazzoni wrote:
> nit: sort this

Done.

https://codereview.chromium.org/420653003/diff/210001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_base.h (right):

https://codereview.chromium.org/420653003/diff/210001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_base.h:17: class AX_EXPORT
AXPlatformNodeBase {
On 2014/07/30 05:53:12, dmazzoni wrote:
> I imagined this the other way around - AXPlatformNodeBase would be a subclass
of
> AXPlatformNode.
> 
> Then only AXPlatformNode needs to be exported.
> 
> I was basing this off of the pattern used in content:
> 
> content/public/browser/render_widget_host_view.h
> 
> content/browser/renderer_host/render_widget_host_view_base.h
> 
> content/browser/renderer_host/render_widget_host_view_mac.h
> content/browser/renderer_host/render_widget_host_view_aura.h
> 

That makes more sense. Done.

https://codereview.chromium.org/420653003/diff/210001/ui/gfx/native_widget_ty...
File ui/gfx/native_widget_types.h (right):

https://codereview.chromium.org/420653003/diff/210001/ui/gfx/native_widget_ty...
ui/gfx/native_widget_types.h:146: typedef void* NativeViewAccessible;
On 2014/07/30 05:53:12, dmazzoni wrote:
> Why wouldn't this be the same as OS_MACOSX? Is there no "id" in iOS?

Done.
The only reason I didn't do it for iOS is because it's not used.

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

https://codereview.chromium.org/420653003/diff/210001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:64: if (index >=
view_->child_count())
On 2014/07/30 05:53:12, dmazzoni wrote:
> Just to be safe,  return null if index < 0 also, since |index| might come
> directly from an untrusted third-party client.

Done.

[dmazzoni, 2014-07-30 20:56:53]

Still lg. +sky for ui/views

[sky, 2014-07-30 21:59:16]

LGTM

[Andre, 2014-07-31 01:32:55]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-07-31 01:34:01]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andresantoso@chromium.org/420653003/31...

[commit-bot: I haz the power, 2014-07-31 12:42:57]

Change committed as 286768

[Avi (use Gerrit), 2014-07-31 19:55:40]

On 2014/07/31 12:42:57, I haz the power (commit-bot) wrote:
> Change committed as 286768

This breaks the Mac 64 component build. Fix is at
https://codereview.chromium.org/437633002 .