Mac a11y: Add Help, TopLevelUIElement, and Window accessibility attributes.

Mac a11y: Add Help, TopLevelUIElement, and Window accessibility attributes.

Add the accessibilityHelp (derived from the tooltip text),
accessibilityTopLevelUIElement, and accessibilityWindow accessibility attributes
for Mac, with tests.

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

[Patti Lor, 2016-07-14 04:22:55]

Description was changed from

==========
Mac a11y: Add Help, TopLevelUIElement, and Window accessibility attributes.

Add the accessibilityHelp, accessibilityTopLevelUIElement, and
accessibilityWindow for Mac views, with tests.

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

to

==========
Mac a11y: Add Help, TopLevelUIElement, and Window accessibility attributes.

Add the accessibilityHelp, accessibilityTopLevelUIElement, and
accessibilityWindow accessibility attributes for MacViews, with tests.

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

[Patti Lor, 2016-07-15 00:22:40]

Description was changed from

==========
Mac a11y: Add Help, TopLevelUIElement, and Window accessibility attributes.

Add the accessibilityHelp, accessibilityTopLevelUIElement, and
accessibilityWindow accessibility attributes for MacViews, with tests.

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

to

==========
Mac a11y: Add Help, TopLevelUIElement, and Window accessibility attributes.

Add the accessibilityHelp, accessibilityTopLevelUIElement, and
accessibilityWindow accessibility attributes for Mac, with tests.

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

[Patti Lor, 2016-07-15 00:41:16]

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

[commit-bot: I haz the power, 2016-07-15 00:41:48]

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-07-15 01:25:08]

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

[commit-bot: I haz the power, 2016-07-15 01:25:09]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-07-15 03:10:56]

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

[Patti Lor, 2016-07-15 03:10:56]

Hi Trent, PTAL.

[tapted, 2016-07-18 03:38:49]

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

https://codereview.chromium.org/2141013004/diff/60001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:70: if
(view_->GetTooltipText(gfx::Point(), &state.help_text))
passing gfx::Point() might not work for the bookmarks bar or task manager
(tableview), but I think this is OK - they only use tooltips for truncated
strings, so there so there should be a better field already

https://codereview.chromium.org/2141013004/diff/60001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:122: // TODO: move this to
NativeViewAccessibilityMac.
do we need to do something with this? (or is there a way to leverage GetParent()
rather than adding a separate GetTopLevelWidget()?

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

https://codereview.chromium.org/2141013004/diff/60001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:187:
widget()->GetContentsView()->AddChildView(view);
will this be sized correctly to hit the view? (maybe first check some other
property at AttributeValueAtMidpoint that could only apply to the label and not
the window itself, to ensure it's the label being hit)

[Patti Lor, 2016-07-19 01:14:10]

Thanks Trent, PTAL.

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

https://codereview.chromium.org/2141013004/diff/60001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:70: if
(view_->GetTooltipText(gfx::Point(), &state.help_text))
On 2016/07/18 03:38:48, tapted wrote:
> passing gfx::Point() might not work for the bookmarks bar or task manager
> (tableview), but I think this is OK - they only use tooltips for truncated
> strings, so there so there should be a better field already

Have double checked this and you are right - both have their accessibilityTitle
set to the full text. I'm not sure if the truncating applies though - I see
elided text in the tooltip for bookmark buttons but the accessibilityHelp is
also the full string (same as accessibilityTitle). The task manager rows on the
other hand don't have tooltips at all (but their accessibilityTitle is set).

Either way it should be fine :)

https://codereview.chromium.org/2141013004/diff/60001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:122: // TODO: move this to
NativeViewAccessibilityMac.
On 2016/07/18 03:38:48, tapted wrote:
> do we need to do something with this? (or is there a way to leverage
GetParent()
> rather than adding a separate GetTopLevelWidget()?

NativeViewAccessibilityMac doesn't exist yet, so I think if we want to get rid
of this TODO it probably belongs in a new CL.

Using GetParent() is more promising, I thought about using it but rejected that
because I couldn't find anything that indicated that the top level widget
wouldn't be skipped somehow. E.g., if |view_| was a bubble (which qualifies as a
top level widget (see
https://cs.chromium.org/chromium/src/ui/views/widget/widget.h?type=cs&sq=pack...),
but bubbles also usually have a parent/owning widget), then the top level widget
would be |view_| and not the bubble parent. GetParent() seems to return the
latter, in which case calling GetTopLevelWidget() on that would return the top
level widget associated with the bubble parent which is wrong. So I don't think
GetParent() is useful here, but let me know if you think otherwise (or if you
were thinking of a different method of using GetParent() entirely).

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

https://codereview.chromium.org/2141013004/diff/60001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:187:
widget()->GetContentsView()->AddChildView(view);
On 2016/07/18 03:38:49, tapted wrote:
> will this be sized correctly to hit the view? (maybe first check some other
> property at AttributeValueAtMidpoint that could only apply to the label and
not
> the window itself, to ensure it's the label being hit)

You're right, I forgot to size it! Have added this in along with a test for the
AX_ROLE_GROUP set in the construction of |view|.

[Patti Lor, 2016-07-19 03:06:57]

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

[commit-bot: I haz the power, 2016-07-19 03:07:13]

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

[tapted, 2016-07-19 03:22:51]

lgtm with a null check

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

https://codereview.chromium.org/2141013004/diff/60001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:70: if
(view_->GetTooltipText(gfx::Point(), &state.help_text))
On 2016/07/19 01:14:10, Patti Lor wrote:
> On 2016/07/18 03:38:48, tapted wrote:
> > passing gfx::Point() might not work for the bookmarks bar or task manager
> > (tableview), but I think this is OK - they only use tooltips for truncated
> > strings, so there so there should be a better field already
> 
> Have double checked this and you are right - both have their
accessibilityTitle
> set to the full text. I'm not sure if the truncating applies though - I see
> elided text in the tooltip for bookmark buttons but the accessibilityHelp is
> also the full string (same as accessibilityTitle). The task manager rows on
the
> other hand don't have tooltips at all (but their accessibilityTitle is set).
> 
> Either way it should be fine :)

Acknowledged. Yeah I only had a skim of the bookmarks bar / tableview code since
they make use of the |point| argument, but I'm not sure how the a11y hit test
will map - probably fine for a follow-up if we find an error

https://codereview.chromium.org/2141013004/diff/60001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:115: return
view_->GetWidget()->GetTopLevelWidget()->GetNativeWindow();
oops - I thought I'd written a comment here but I might have gotten distracted
by GetParent(). I think this should check `view_->GetWidget()` first to ensure
it's non-null (like GetParent does)

https://codereview.chromium.org/2141013004/diff/60001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:122: // TODO: move this to
NativeViewAccessibilityMac.
On 2016/07/19 01:14:10, Patti Lor wrote:
> On 2016/07/18 03:38:48, tapted wrote:
> > do we need to do something with this? (or is there a way to leverage
> GetParent()
> > rather than adding a separate GetTopLevelWidget()?
> 
> NativeViewAccessibilityMac doesn't exist yet, so I think if we want to get rid
> of this TODO it probably belongs in a new CL.
> 
> Using GetParent() is more promising, I thought about using it but rejected
that
> because I couldn't find anything that indicated that the top level widget
> wouldn't be skipped somehow. E.g., if |view_| was a bubble (which qualifies as
a
> top level widget (see
>
https://cs.chromium.org/chromium/src/ui/views/widget/widget.h?type=cs&sq=pack...),
> but bubbles also usually have a parent/owning widget), then the top level
widget
> would be |view_| and not the bubble parent. GetParent() seems to return the
> latter, in which case calling GetTopLevelWidget() on that would return the top
> level widget associated with the bubble parent which is wrong. So I don't
think
> GetParent() is useful here, but let me know if you think otherwise (or if you
> were thinking of a different method of using GetParent() entirely).

Acknowledged.

[Patti Lor, 2016-07-19 03:45:23]

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

[commit-bot: I haz the power, 2016-07-19 03:45: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-07-19 04:36:23]

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

[commit-bot: I haz the power, 2016-07-19 04:36:24]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-07-19 05:24:33]

Description was changed from

==========
Mac a11y: Add Help, TopLevelUIElement, and Window accessibility attributes.

Add the accessibilityHelp, accessibilityTopLevelUIElement, and
accessibilityWindow accessibility attributes for Mac, with tests.

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

to

==========
Mac a11y: Add Help, TopLevelUIElement, and Window accessibility attributes.

Add the accessibilityHelp (derived from the tooltip text),
accessibilityTopLevelUIElement, and accessibilityWindow accessibility attributes
for Mac, with tests.

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

[Patti Lor, 2016-07-19 05:29:23]

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

[Patti Lor, 2016-07-19 05:29:24]

+dmazzoni@ for ui/accessibility/* and ui/views/accessibility/*

PTAL, thank you!

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

https://codereview.chromium.org/2141013004/diff/60001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:70: if
(view_->GetTooltipText(gfx::Point(), &state.help_text))
On 2016/07/19 03:22:50, tapted wrote:
> On 2016/07/19 01:14:10, Patti Lor wrote:
> > On 2016/07/18 03:38:48, tapted wrote:
> > > passing gfx::Point() might not work for the bookmarks bar or task manager
> > > (tableview), but I think this is OK - they only use tooltips for truncated
> > > strings, so there so there should be a better field already
> > 
> > Have double checked this and you are right - both have their
> accessibilityTitle
> > set to the full text. I'm not sure if the truncating applies though - I see
> > elided text in the tooltip for bookmark buttons but the accessibilityHelp is
> > also the full string (same as accessibilityTitle). The task manager rows on
> the
> > other hand don't have tooltips at all (but their accessibilityTitle is set).
> > 
> > Either way it should be fine :)
> 
> Acknowledged. Yeah I only had a skim of the bookmarks bar / tableview code
since
> they make use of the |point| argument, but I'm not sure how the a11y hit test
> will map - probably fine for a follow-up if we find an error

I checked this using the XCode Accessibility Inspector, so hit tests for task
manager rows are fine. But I had to dig around a bit for the bookmark bar
buttons, but I think that will be fixed when http://crbug.com/610589 (hiding
labels inside LabelButtons from the a11y tree) is fixed.

https://codereview.chromium.org/2141013004/diff/60001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility.cc:115: return
view_->GetWidget()->GetTopLevelWidget()->GetNativeWindow();
On 2016/07/19 03:22:50, tapted wrote:
> oops - I thought I'd written a comment here but I might have gotten distracted
> by GetParent(). I think this should check `view_->GetWidget()` first to ensure
> it's non-null (like GetParent does)

Done, thank you.

[dmazzoni, 2016-07-26 22:26:56]

lgtm

lgtm but just use AX_ATTR_DESCRIPTION for secondary accessible text like tooltip
text that maps to AXHelp on Mac. Thanks!

https://codereview.chromium.org/2141013004/diff/100001/ui/accessibility/ax_en...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2141013004/diff/100001/ui/accessibility/ax_en...
ui/accessibility/ax_enums.idl:245: description,
It's not obvious at all, but "description" maps to the AXHelp attribute on Mac.
We use this convention because on the web, the primary and secondary accessible
text for an object are called "name" and "description", so we map those to the
primary and secondary on Mac, which is AXTitle/AXDescription (primary) and
AXHelp (secondary). In newer versions of OS X it's AXLabel instead of
AXTitle/AXDescription.

[Patti Lor, 2016-07-27 01:40:52]

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

[commit-bot: I haz the power, 2016-07-27 01:41:09]

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-07-27 02:33:52]

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

[commit-bot: I haz the power, 2016-07-27 02:33:53]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-07-27 02:59:00]

https://codereview.chromium.org/2141013004/diff/100001/ui/accessibility/ax_en...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/2141013004/diff/100001/ui/accessibility/ax_en...
ui/accessibility/ax_enums.idl:245: description,
On 2016/07/26 22:26:56, dmazzoni wrote:
> It's not obvious at all, but "description" maps to the AXHelp attribute on
Mac.
> We use this convention because on the web, the primary and secondary
accessible
> text for an object are called "name" and "description", so we map those to the
> primary and secondary on Mac, which is AXTitle/AXDescription (primary) and
> AXHelp (secondary). In newer versions of OS X it's AXLabel instead of
> AXTitle/AXDescription.

Done - this involved rewriting the AXRoleDescription selector to only generate a
role description from the role and subrole though (this is what it currently
does now if AX_ATTR_DESCRIPTION isn't populated). Thanks!

[Patti Lor, 2016-07-27 02:59:07]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2016-07-27 02:59:07]

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/2141013004/#ps120001
(title: "Use AX_ATTR_DESCRIPTION instead of adding AX_ATTR_HELP_TEXT, remove use
of AX_ATTR_DESCRIPTION from…")

[commit-bot: I haz the power, 2016-07-27 02:59:26]

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-27 03:02:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-27 03:04:31]

Description was changed from

==========
Mac a11y: Add Help, TopLevelUIElement, and Window accessibility attributes.

Add the accessibilityHelp (derived from the tooltip text),
accessibilityTopLevelUIElement, and accessibilityWindow accessibility attributes
for Mac, with tests.

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

to

==========
Mac a11y: Add Help, TopLevelUIElement, and Window accessibility attributes.

Add the accessibilityHelp (derived from the tooltip text),
accessibilityTopLevelUIElement, and accessibilityWindow accessibility attributes
for Mac, with tests.

BUG=610591

Committed: https://crrev.com/37f9fc7c83fd0386d554fb93213287b698b037c0
Cr-Commit-Position: refs/heads/master@{#408029}
==========

[commit-bot: I haz the power, 2016-07-27 03:04:33]

Patchset 7 (id:??) landed as
https://crrev.com/37f9fc7c83fd0386d554fb93213287b698b037c0
Cr-Commit-Position: refs/heads/master@{#408029}