Issue 1134653003: Implement UIElementsForSearchPredicate accessibility APIs for OS X.

Issue 1134653003: Implement UIElementsForSearchPredicate accessibility APIs for OS X. (Closed)

Created:
5 years, 7 months ago by dmazzoni

Modified:
5 years, 7 months ago

Reviewers:
David Tseng

CC:
chromium-reviews, je_julie(Not used), plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, jam, yuzo+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Implement UIElementsForSearchPredicate accessibility APIs for OS X. This enables VoiceOver in OS X 10.10 to use the rotor and other keyboard shortcuts that move by element type. BUG=471119 Committed: https://crrev.com/5ad9f1fd109c670658ea9da85443c9ada7b159d0 Cr-Commit-Position: refs/heads/master@{#329979}

Patch Set 1 #

Total comments: 47

Patch Set 2 : Respond to feedback #

Patch Set 3 : Fix win test failure #

Total comments: 9

Patch Set 4 : Address feedback #

Created: 5 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+819 lines, -9 lines)			Patch
M	content/browser/accessibility/browser_accessibility.h	View	1	2 chunks	+3 lines, -1 line	0 comments	Download
M	content/browser/accessibility/browser_accessibility.cc	View	1 2 3	1 chunk	+32 lines, -0 lines	0 comments	Download
M	content/browser/accessibility/browser_accessibility_cocoa.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	content/browser/accessibility/browser_accessibility_cocoa.mm	View	1 2 3	7 chunks	+316 lines, -7 lines	0 comments	Download
A	content/browser/accessibility/one_shot_accessibility_tree_search.h	View	1	1 chunk	+105 lines, -0 lines	0 comments	Download
A	content/browser/accessibility/one_shot_accessibility_tree_search.cc	View	1	1 chunk	+182 lines, -0 lines	0 comments	Download
A	content/browser/accessibility/one_shot_accessibility_tree_search_unittest.cc	View	1 2	1 chunk	+174 lines, -0 lines	0 comments	Download
M	content/content_browser.gypi	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download
M	content/content_tests.gypi	View	1 2	2 chunks	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 11 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

David Tseng

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibility/accessibility_tree_search.cc File content/browser/accessibility/accessibility_tree_search.cc (right): https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibility/accessibility_tree_search.cc#newcode1 content/browser/accessibility/accessibility_tree_search.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights ...

5 years, 7 months ago (2015-05-08 21:58:25 UTC) #3

dmazzoni

5 years, 7 months ago (2015-05-13 04:22:21 UTC) #4

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_tree_search.cc (right):

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.cc:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
On 2015/05/08 21:58:24, David Tseng wrote:
> nit: remove (c)?

Done.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.cc:28:
strings->push_back(node->GetString16Attribute(ui::AX_ATTR_PLACEHOLDER));
On 2015/05/08 21:58:24, David Tseng wrote:
> What about shortcut and url?

I don't think those are text. shortcut is the access key, and url is the url a
link points to. For an accessible find in page feature I don't think i'd expect
those to be searched.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.cc:33: : tree_(tree),
On 2015/05/08 21:58:24, David Tseng wrote:
> nit: +4 indent

I think this is right? It's +4 from the left edge now, and the line above is a
line continuation, which shouldn't matter.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.cc:36:
result_limit_(-1),
On 2015/05/08 21:58:24, David Tseng wrote:
> nit: -1 means what? (error/unlimited)?

Done.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.cc:93: DCHECK(index <
matches_.size());
On 2015/05/08 21:58:24, David Tseng wrote:
> Should this be a DCHECK?

Done.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.cc:100: if (start_node_)
{
On 2015/05/08 21:58:24, David Tseng wrote:
> Is it expected that the search starts from the directed next node?

Yes, the search does not include the start node itself.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.cc:112: if
(immediate_descendants_only_ && start_node_) {
On 2015/05/08 21:58:24, David Tseng wrote:
> Isn't start_node_ always going to be non-null here?

Done.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.cc:113: if
(!node->IsDescendantOf(start_node_))
On 2015/05/08 21:58:24, David Tseng wrote:
> This check could be part of your traversal.

Done.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.cc:118: for (size_t i =
0; i < predicates_.size() && is_match; ++i) {
On 2015/05/08 21:58:24, David Tseng wrote:
> Maybe add a CHECK to ensure the caller has actually added at least one
predicate
> or a search string.

Not required - it's okay to use this just to get the next <n> elements in the
tree in tree order if you want.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.cc:138: if
(node_string_lower.find(search_text_lower) != base::string16::npos) {
On 2015/05/08 21:58:25, David Tseng wrote:
> Does Mac expect case insensitive searches?

Yes, this matches their implementation. We could make case sensitivity optional
but honestly I think that consider this is a user-facing feature
case-insensitive is probably best 99% of the time.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_tree_search.h (right):

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.h:29: // over the
matches, and then delete it.
On 2015/05/08 21:58:25, David Tseng wrote:
> Rename to *OneShot?

Done.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.h:52: // Set the maximum
number of results, or -1 for no limit (default).
On 2015/05/08 21:58:25, David Tseng wrote:
> Name -1; also, does it even make sense to allow 0 as a value here?

Done.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search.h:56: void
SetImmediateDescendantsOnly(bool immediate_descendants_only);
On 2015/05/08 21:58:25, David Tseng wrote:
> Is there a difference between immediate and non-immediate descendants?

Turns out I was wrong about the implementation. I found the WebKit bug where
this was added and it's supposed to mean only the children of the start node.

https://bugs.webkit.org/show_bug.cgi?id=128747

I fixed the code and the test.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
File content/browser/accessibility/accessibility_tree_search_unittest.cc
(right):

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/accessibility_tree_search_unittest.cc:20: 
On 2015/05/08 21:58:25, David Tseng wrote:
> nit: private:

This is protected now, it needs to be since each TEST_F test is a subclass. I
can make the next line private, though, done.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:60: if ([searchKey
isEqualToString:@"AXAnyTypeSearchKey"]) {
On 2015/05/08 21:58:25, David Tseng wrote:
> Is this string defined in NS*? Is this API available only on 10.10?

It's not even in 10.10. As far as I can tell the search predicate API is totally
undocumented.

grep Predicate
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSAccessibility*

If you do a web search for the names in this protocol the only hits are WebKit
code.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:70: return nullptr;
 // TODO(dmazzoni): implement this.
On 2015/05/08 21:58:25, David Tseng wrote:
> Can you move the TODO into its own line above?

Done throughout.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:73: return
(current->GetRole() == ui::AX_ROLE_BUTTON ||
On 2015/05/08 21:58:25, David Tseng wrote:
> Not sure if they exist on Mac, but menuButton and switch?

Done.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:84: return
current->HasState(ui::AX_STATE_FOCUSABLE);
On 2015/05/08 21:58:25, David Tseng wrote:
> If a button doesn't take focus (like if it were disabled), would the focusable
> state still be set?

No, good point. I implemented IsControl but decided to also return anything
that's focusable and not a link, because there are some roles like CELL that
could be interactive if focusable, and users probably want anything interactive
that's not a link, right?

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:128:
current->GetIntAttribute(ui::AX_ATTR_HIERARCHICAL_LEVEL) == 5);
On 2015/05/08 21:58:25, David Tseng wrote:
> Maybe worth writing a helper returning a predicate given a heading level.

It's only 6 heading levels and they're 2 lines each and I don't anticipate
needing to change this code again, so I'd prefer to keep it here.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:139:
start->GetIntAttribute(ui::AX_ATTR_HIERARCHICAL_LEVEL));
On 2015/05/08 21:58:25, David Tseng wrote:
> Need to check start is a heading too?

Done.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:196: return
current->GetRole() == ui::AX_ROLE_TABLE;
On 2015/05/08 21:58:25, David Tseng wrote:
> What about grid? I think this means you need to check you don't descend into a
> nested table.

Done.

Added TODO for nested, I wasn't planning on implementing that unless users
request it.

https://codereview.chromium.org/1134653003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_cocoa.mm:277:
search->SetSearchText(searchText);
On 2015/05/08 21:58:25, David Tseng wrote:
> Looks like you do have all args available here; could just make
> AccessibilityTreeSearch take all arguments on construction and eliminate all
the
> setters.

Two reasons not to:
* I think this is more readable than a bunch of positional arguments - it's easy
to get the order wrong by accident
* We may reuse AccessibilityTreeSearch for Android and we wouldn't need all of
these, so I think it'd be cleaner to keep them separate

David Tseng

lgtm https://codereview.chromium.org/1134653003/diff/40001/content/browser/accessibility/browser_accessibility.cc File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/1134653003/diff/40001/content/browser/accessibility/browser_accessibility.cc#newcode771 content/browser/accessibility/browser_accessibility.cc:771: case ui::AX_ROLE_LIST_BOX: I think we want the container ...

5 years, 7 months ago (2015-05-14 19:31:55 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1134653003/60001

5 years, 7 months ago (2015-05-14 23:00:47 UTC) #8

dmazzoni

https://codereview.chromium.org/1134653003/diff/40001/content/browser/accessibility/browser_accessibility.cc File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/1134653003/diff/40001/content/browser/accessibility/browser_accessibility.cc#newcode771 content/browser/accessibility/browser_accessibility.cc:771: case ui::AX_ROLE_LIST_BOX: On 2015/05/14 19:31:54, David Tseng wrote: > ...

5 years, 7 months ago (2015-05-14 23:00:57 UTC) #9

commit-bot: I haz the power

5 years, 7 months ago (2015-05-14 23:58:42 UTC) #11

Message was sent while issue was closed.

Patchset 4 (id:??) landed as
https://crrev.com/5ad9f1fd109c670658ea9da85443c9ada7b159d0
Cr-Commit-Position: refs/heads/master@{#329979}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages