Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(23)

Issue 2119413004: a11y: Exclude children of nested keyboard accessible controls from a11y tree. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 months, 3 weeks ago by Patti Lor
Modified:
1 month ago
Reviewers:
tapted, dmazzoni
CC:
chromium-reviews, aboxhall+watch_chromium.org, tfarina, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

a11y: Exclude children of nested keyboard accessible controls from a11y tree. Currently, children of keyboard accessible controls, such as the Labels inside LabelButtons, are not hidden from the accessibility tree. This is not useful because the user only cares about the element that has focus, not any children inside it, which are implementation details. Set any children of keyboard accessible controls to have ignored roles, which indicates to the OS (works for Windows, Mac, and Linux) that the ignored element should be excluded from the accessibility tree. BUG=610589, 723574 TEST=On Mac: Open the XCode Accessibility Inspector. On Windows: Open the Windows SDK 'Inspect.exe' and make sure the icon for 'Watch Focus' is selected. Then (on Mac, hover over; on Windows, focus) a views::LabelButton (e.g. on the HTTP Authentication dialog). It should report an empty set of children and its role ("Type"/"accessibilityRole" on Mac; "Role"/"ControlType" on Windows) should say it is a button. Review-Url: https://codereview.chromium.org/2119413004 Cr-Commit-Position: refs/heads/master@{#474867} Committed: https://chromium.googlesource.com/chromium/src/+/ce2e11f2565db306765cc6f6412cf5bfb158fb70

Patch Set 1 #

Patch Set 2 : Tentative switch to use IsAccessibilityFocusable() instead of a flag. #

Total comments: 2

Patch Set 3 : Implement ignored accessibility elements and hide them from accessibility clients, with tests. #

Patch Set 4 : Fix compile errors. #

Patch Set 5 : Fix last two tests on Mac, compile errors for Linux and Windows. #

Patch Set 6 : Re-add compile error fixes. #

Patch Set 7 : Fix compile error and final test in NativeWidgetMacTest. #

Total comments: 1

Patch Set 8 : Before revert of cross-platform ignored a11y elements. #

Patch Set 9 : Move to a Mac-specific approach by ignoring elements with keyboard focusable parents. #

Total comments: 4

Patch Set 10 : Review comments, ignore View::enabled_. #

Patch Set 11 : Don't ignore children if they're accessibility focusable, regardless of enabled state. #

Total comments: 16

Patch Set 12 : Review comments. #

Total comments: 2

Patch Set 13 : Review comments. #

Total comments: 14

Patch Set 14 : Switch back to "using LabelButton::label() and update comment. #

Patch Set 15 : Make things cross-platform (with code from patchset 8). #

Patch Set 16 : Fix Windows/Linux compilation, use insert instead of emplace. #

Patch Set 17 : Don't use assignments inside if conditionals because Windows doesn't like it. #

Patch Set 18 : Fix ChildAtIndex() for Widget children. #

Patch Set 19 : Refactor to use ui::AX_ROLE_IGNORED for excluding a11y elements from the tree instead of focusabili… #

Total comments: 49

Patch Set 20 : Review comments (incomplete). #

Patch Set 21 : Rebase. #

Patch Set 22 : Fix compile error. #

Patch Set 23 : Fix widget children. #

Total comments: 34

Patch Set 24 : Implement FromNativeViewAccessible() for Mac/Linux. #

Patch Set 25 : Fix CrOS. #

Total comments: 6

Patch Set 26 : Address compilation issues + review comments. #

Patch Set 27 : Don't use AXPlatformNode::FromNativeViewAccessible() on CrOS (unimplemented). #

Patch Set 28 : Rebase on top of 2704263002. #

Total comments: 21

Patch Set 29 : Rebase again. #

Total comments: 13

Patch Set 30 : Review comments. #

Total comments: 9

Patch Set 31 : Fix easy review comments. #

Patch Set 32 : Rebase. #

Patch Set 33 : Switch to OS-specific methods of ignoring a11y elements. #

Total comments: 2

Patch Set 34 : Linting & review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+118 lines, -15 lines) Patch
M ui/accessibility/platform/ax_platform_node_auralinux.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +2 lines, -0 lines 0 comments Download
M ui/accessibility/platform/ax_platform_node_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +3 lines, -1 line 0 comments Download
M ui/accessibility/platform/ax_platform_node_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +3 lines, -1 line 0 comments Download
M ui/views/accessibility/native_view_accessibility_base.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 2 chunks +29 lines, -0 lines 0 comments Download
M ui/views/accessibility/native_view_accessibility_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 8 chunks +28 lines, -9 lines 0 comments Download
M ui/views/view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 2 chunks +2 lines, -4 lines 0 comments Download
M ui/views/widget/native_widget_mac_accessibility_unittest.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 3 chunks +51 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 317 (275 generated)
Patti Lor
Hi Trent, PTAL. One part of this CL I'm not sure about is whether to ...
11 months, 3 weeks ago (2016-07-07 03:26:00 UTC) #2
tapted
Did you see my comment at http://crbug.com/610589#c3 ? (is that a dead-end?) The notion would ...
11 months, 3 weeks ago (2016-07-07 04:27:26 UTC) #3
Patti Lor
On 2016/07/07 04:27:26, tapted wrote: > Did you see my comment at http://crbug.com/610589#c3 ? (is ...
11 months, 3 weeks ago (2016-07-07 05:24:19 UTC) #4
Patti Lor
So, I think doing it the way you suggested is going to be a bit ...
11 months, 2 weeks ago (2016-07-11 07:16:30 UTC) #5
tapted
On 2016/07/11 07:16:30, Patti Lor wrote: > So, I think doing it the way you ...
11 months, 2 weeks ago (2016-07-12 01:44:14 UTC) #6
tapted
https://codereview.chromium.org/2119413004/diff/20001/ui/views/controls/button/label_button.cc File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/2119413004/diff/20001/ui/views/controls/button/label_button.cc#newcode135 ui/views/controls/button/label_button.cc:135: SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY); The goal would be to make these lines ...
11 months, 2 weeks ago (2016-07-12 01:44:23 UTC) #7
tapted
https://codereview.chromium.org/2119413004/diff/120001/ui/accessibility/platform/ax_platform_node_mac.mm File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2119413004/diff/120001/ui/accessibility/platform/ax_platform_node_mac.mm#newcode283 ui/accessibility/platform/ax_platform_node_mac.mm:283: if (NSPointInRect(point, child.boundsInScreen)) Perhaps we can tackle this logic. ...
8 months, 1 week ago (2016-10-18 04:38:31 UTC) #32
Patti Lor
Hi Trent, PTAL - scrapped the old approach entirely. Thanks! https://codereview.chromium.org/2119413004/diff/20001/ui/views/controls/button/label_button.cc File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/2119413004/diff/20001/ui/views/controls/button/label_button.cc#newcode135 ...
7 months ago (2016-11-25 03:59:28 UTC) #40
tapted
looks pretty good, but I'd loop in an a11y expert -- they might know better ...
7 months ago (2016-11-25 04:36:27 UTC) #43
Patti Lor
Thanks Trent, PTAL. https://codereview.chromium.org/2119413004/diff/160001/ui/views/accessibility/native_view_accessibility.cc File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/160001/ui/views/accessibility/native_view_accessibility.cc#newcode105 ui/views/accessibility/native_view_accessibility.cc:105: if (HasFocusableAncestor()) On 2016/11/25 04:36:27, tapted ...
6 months, 4 weeks ago (2016-11-28 06:40:23 UTC) #50
tapted
https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility/native_view_accessibility.cc File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility/native_view_accessibility.cc#newcode24 ui/views/accessibility/native_view_accessibility.cc:24: bool ViewHasFocusableAncestor(View* parent) { nit: parent -> view (since ...
6 months, 4 weeks ago (2016-11-29 03:12:49 UTC) #53
Patti Lor
Hi Trent, PTAL! Thanks. https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility/native_view_accessibility.cc File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/accessibility/native_view_accessibility.cc#newcode24 ui/views/accessibility/native_view_accessibility.cc:24: bool ViewHasFocusableAncestor(View* parent) { On ...
6 months, 3 weeks ago (2016-11-29 23:26:19 UTC) #58
tapted
https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native_widget_mac_accessibility_unittest.mm File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native_widget_mac_accessibility_unittest.mm#newcode124 ui/views/widget/native_widget_mac_accessibility_unittest.mm:124: child_button->SetSize(gfx::Size()); On 2016/11/29 23:26:18, Patti Lor wrote: > On ...
6 months, 3 weeks ago (2016-11-29 23:43:20 UTC) #59
Patti Lor
PTAL, thanks https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native_widget_mac_accessibility_unittest.mm File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/200001/ui/views/widget/native_widget_mac_accessibility_unittest.mm#newcode124 ui/views/widget/native_widget_mac_accessibility_unittest.mm:124: child_button->SetSize(gfx::Size()); On 2016/11/29 23:43:20, tapted wrote: > ...
6 months, 3 weeks ago (2016-11-30 05:13:25 UTC) #62
tapted
lgtm with the following https://codereview.chromium.org/2119413004/diff/240001/ui/views/widget/native_widget_mac_accessibility_unittest.mm File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/240001/ui/views/widget/native_widget_mac_accessibility_unittest.mm#newcode68 ui/views/widget/native_widget_mac_accessibility_unittest.mm:68: Label* GetLabel() { return LabelButton::label(); ...
6 months, 3 weeks ago (2016-11-30 05:23:17 UTC) #63
Patti Lor
Hi dmazzoni, WDYT of this approach? Thanks! https://codereview.chromium.org/2119413004/diff/240001/ui/views/widget/native_widget_mac_accessibility_unittest.mm File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2119413004/diff/240001/ui/views/widget/native_widget_mac_accessibility_unittest.mm#newcode68 ui/views/widget/native_widget_mac_accessibility_unittest.mm:68: Label* GetLabel() ...
6 months, 3 weeks ago (2016-11-30 05:37:37 UTC) #65
dmazzoni
https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility/native_view_accessibility.cc File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/240001/ui/views/accessibility/native_view_accessibility.cc#newcode110 ui/views/accessibility/native_view_accessibility.cc:110: #if defined(OS_MACOSX) I'd prefer to find a solution that ...
6 months, 3 weeks ago (2016-11-30 23:12:45 UTC) #68
Patti Lor
Thanks for the review dmazzoni - the new patchset is not for your comments, but ...
6 months, 3 weeks ago (2016-12-01 01:12:07 UTC) #71
dmazzoni
On 2016/12/01 01:12:07, Patti Lor wrote: > Yes - this was my approach in patchset ...
6 months, 3 weeks ago (2016-12-02 00:07:16 UTC) #74
Patti Lor
Hi Trent, PTAL - it's changed a lot since you gave lgtm. Merged with the ...
6 months ago (2016-12-21 04:09:09 UTC) #117
tapted
https://codereview.chromium.org/2119413004/diff/460001/ui/accessibility/platform/ax_platform_node_mac.mm File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2119413004/diff/460001/ui/accessibility/platform/ax_platform_node_mac.mm#newcode289 ui/accessibility/platform/ax_platform_node_mac.mm:289: ![child accessibilityIsIgnored]) nit: reorder these so accessibilityIsIgnored is checked ...
6 months ago (2016-12-22 02:56:37 UTC) #120
dmazzoni
Looking really good! https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility/native_view_accessibility.cc File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/460001/ui/views/accessibility/native_view_accessibility.cc#newcode77 ui/views/accessibility/native_view_accessibility.cc:77: // This should only ever happen ...
5 months, 4 weeks ago (2016-12-28 18:02:35 UTC) #121
Patti Lor
Thanks all, PTAL. https://codereview.chromium.org/2119413004/diff/460001/ui/accessibility/platform/ax_platform_node_mac.mm File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2119413004/diff/460001/ui/accessibility/platform/ax_platform_node_mac.mm#newcode289 ui/accessibility/platform/ax_platform_node_mac.mm:289: ![child accessibilityIsIgnored]) On 2016/12/22 02:56:36, tapted ...
5 months, 2 weeks ago (2017-01-11 02:01:48 UTC) #138
dmazzoni
lgtm https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility/native_view_accessibility.cc File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2119413004/diff/540001/ui/views/accessibility/native_view_accessibility.cc#newcode247 ui/views/accessibility/native_view_accessibility.cc:247: // don't have to search further because AXPlatformNodeWin ...
5 months, 2 weeks ago (2017-01-11 07:39:38 UTC) #141
tapted
Sorry... lots more comments - I'm still discovering lots in the a11y code :) I ...
5 months, 2 weeks ago (2017-01-11 18:27:40 UTC) #142
tapted
https://codereview.chromium.org/2119413004/diff/680001/ui/accessibility/platform/ax_platform_node_mac.h File ui/accessibility/platform/ax_platform_node_mac.h (right): https://codereview.chromium.org/2119413004/diff/680001/ui/accessibility/platform/ax_platform_node_mac.h#newcode55 ui/accessibility/platform/ax_platform_node_mac.h:55: - (ui::AXPlatformNodeBase*)getNode; @property(nonatomic, readonly) ui::AXPlatformNodeBase* node; (and @synthesize node ...
4 months, 1 week ago (2017-02-15 02:10:45 UTC) #181
Patti Lor
Ok, finally refactored the whole thing to use AXPlatformNode::FromNativeViewAccessible(), including implementing it for Linux and ...
4 months ago (2017-02-21 03:29:17 UTC) #215
tapted
https://codereview.chromium.org/2119413004/diff/840001/ui/gfx/native_widget_types.h File ui/gfx/native_widget_types.h (right): https://codereview.chromium.org/2119413004/diff/840001/ui/gfx/native_widget_types.h#newcode147 ui/gfx/native_widget_types.h:147: typedef void* NativeViewAccessible; I think we can get rid ...
4 months ago (2017-02-21 06:01:12 UTC) #216
Patti Lor
https://codereview.chromium.org/2119413004/diff/840001/ui/gfx/native_widget_types.h File ui/gfx/native_widget_types.h (right): https://codereview.chromium.org/2119413004/diff/840001/ui/gfx/native_widget_types.h#newcode147 ui/gfx/native_widget_types.h:147: typedef void* NativeViewAccessible; On 2017/02/21 06:01:11, tapted wrote: > ...
3 months, 4 weeks ago (2017-02-27 05:31:04 UTC) #225
Patti Lor
ping @ Trent? Thanks :)
2 months, 3 weeks ago (2017-04-03 03:59:51 UTC) #268
tapted
mostly nits, and some questions https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibility/native_view_accessibility_base.cc File ui/views/accessibility/native_view_accessibility_base.cc (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibility/native_view_accessibility_base.cc#newcode25 ui/views/accessibility/native_view_accessibility_base.cc:25: // being a non-keyboard-focusable ...
2 months, 3 weeks ago (2017-04-03 05:50:36 UTC) #271
Patti Lor
Hi Trent, PTAL https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibility/native_view_accessibility_base.cc File ui/views/accessibility/native_view_accessibility_base.cc (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibility/native_view_accessibility_base.cc#newcode25 ui/views/accessibility/native_view_accessibility_base.cc:25: // being a non-keyboard-focusable child of ...
2 months, 3 weeks ago (2017-04-04 02:46:29 UTC) #284
tapted
https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibility/native_view_accessibility_base.cc File ui/views/accessibility/native_view_accessibility_base.cc (right): https://codereview.chromium.org/2119413004/diff/1060001/ui/views/accessibility/native_view_accessibility_base.cc#newcode221 ui/views/accessibility/native_view_accessibility_base.cc:221: return child_nva->GetParent(); On 2017/04/04 02:46:28, Patti Lor wrote: > ...
2 months, 3 weeks ago (2017-04-04 08:13:13 UTC) #287
tapted
i've managed to get myself really lost - some suggestions below might allow an approach ...
2 months, 3 weeks ago (2017-04-05 06:03:33 UTC) #288
Patti Lor
Thanks Trent - I need to do more investigation for your other comments that I've ...
2 months, 2 weeks ago (2017-04-11 05:40:46 UTC) #291
Patti Lor
Hey Trent, did you want to take a look at this again? It's now just ...
1 month ago (2017-05-24 07:29:11 UTC) #302
tapted
This is awesome. lgtm, but dmazzoni should take a peek too Also does it fix ...
1 month ago (2017-05-24 11:05:31 UTC) #303
Patti Lor
Thanks Trent. Dominic, do you want to take a look now? One thing I forgot ...
1 month ago (2017-05-25 08:24:53 UTC) #309
dmazzoni
still lgtm Just a note that while this general logic (a child of a focusable ...
1 month ago (2017-05-25 21:12:43 UTC) #310
Patti Lor
On 2017/05/25 21:12:43, dmazzoni wrote: > still lgtm > > Just a note that while ...
1 month ago (2017-05-26 01:15:46 UTC) #311
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2119413004/1190001
1 month ago (2017-05-26 01:18:28 UTC) #314
commit-bot: I haz the power
1 month ago (2017-05-26 01:26:00 UTC) #317
Message was sent while issue was closed.
Committed patchset #34 (id:1190001) as
https://chromium.googlesource.com/chromium/src/+/ce2e11f2565db306765cc6f6412c...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318