Improve omnibox accessibility on Windows.

Improve omnibox accessibility on Windows.

This refactors and improves a bunch of views accessibility
code on Windows and has the result of enabling NVDA to
announce the text selection in the omnibox.

The AutocompleteAccessibility class is removed; its
functionality is rolled into NativeViewAccessibilityWin.

NativeViewAccessibilityWin adds IAccessible2 and
IAccessibleText interfaces, allowing it to directly expose
the caret and selection to compatible assistive technology.
In addition, this gives each accessible object an unique id.

Finally, the reference from a View to its
NativeViewAccessibilityWin is changed from a scoped_refptr
to a ScopedComPtr, because another process may still have a
reference to the accessible COM object when the View is
deleted.

BUG=53380
TEST=Manually test the omnibox with NVDA on Windows. It should announce text that's selected, including autocompletions.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107924

[dmazzoni, 2011-10-25 18:16:00]

sky: overall code quality
dtseng: check implementation of a11y interfaces in particular

[sky, 2011-10-25 20:08:44]

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:332: state->selection_end =
sel_end;
Is this ok if sel_start > sel_end?

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:337: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right):

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.h:284: LRESULT OnGetObject(UINT
message, WPARAM wparamm, LPARAM lparam);
wparamm -> wparam

http://codereview.chromium.org/8391010/diff/4001/views/accessibility/native_v...
File views/accessibility/native_view_accessibility_win.cc (right):

http://codereview.chromium.org/8391010/diff/4001/views/accessibility/native_v...
views/accessibility/native_view_accessibility_win.cc:214: // Remain with the
same dispatch.
Update this comment. Also document who does the release.

http://codereview.chromium.org/8391010/diff/4001/views/accessibility/native_v...
views/accessibility/native_view_accessibility_win.cc:861: if (nav_dir ==
NAVDIR_RIGHT || nav_dir == NAVDIR_DOWN ||
nit: make this the return statement, eg:

return (nav_dir == .. || ... );

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
File views/widget/native_widget_win.cc (right):

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
views/widget/native_widget_win.cc:449: long unique_id = -child_id;
Why do you negate this?

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
views/widget/native_widget_win.cc:452: if (iter !=
accessibility_id_view_map_.end())
nit: convert to a return statement.

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
views/widget/native_widget_win.cc:574: void NativeWidgetWin::ViewRemoved(View*
view) {
Don't you need to do this for all descendants of view too?

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
views/widget/native_widget_win.cc:581: HRESULT hr = accessible->QueryInterface(
How expensive is this? Would it be better to maintain the inverse mapping
(view->id)?

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
views/widget/native_widget_win.cc:586: if (hr == S_OK) {
nit: no {}

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
views/widget/native_widget_win.cc:639: if
(!accessibility_id_view_map_[unique_id])
Can you reset the map all the time? Or maybe DCHECK that NULL or == view.

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
File views/widget/native_widget_win.h (right):

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
views/widget/native_widget_win.h:110: View* GetAccessibilityViewFromChildId(long
id);
How about naming this GetViewByAccessibleId().
I don't like having to keep the map up to date. Is it possible to have this walk
the tree each time it's invoked?

[David Tseng, 2011-10-25 21:55:25]

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:963:
native_view_host_->set_focus_view(parent);
On Windows, is the focused view the parent? (LocationBarView). Perhaps it should
be the NativeViewHost?

http://codereview.chromium.org/8391010/diff/4001/ui/base/accessibility/access...
File ui/base/accessibility/accessibility_types.h (right):

http://codereview.chromium.org/8391010/diff/4001/ui/base/accessibility/access...
ui/base/accessibility/accessibility_types.h:44: STATE_FOCUSED     = 1 << 14,
nit: Alphabetize now?

[dmazzoni, 2011-10-26 16:46:13]

Scott, I changed it as you suggested to search the view hierarchy rather than
maintain the id -> view map. It works fine and I agree it's a little more
foolproof. More details below.

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:332: state->selection_end =
sel_end;
On 2011/10/25 20:08:45, sky wrote:
> Is this ok if sel_start > sel_end?

Yes, I tested creating a selection using shift+right and shift+left from a
cursor and they both work.

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:337: };
On 2011/10/25 20:08:45, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:963:
native_view_host_->set_focus_view(parent);
On 2011/10/25 21:55:25, David Tseng wrote:
> On Windows, is the focused view the parent? (LocationBarView). Perhaps it
should
> be the NativeViewHost?

I think this doesn't affect native focus, it just indicates that the parent is
the one keeping track of views focus.

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right):

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.h:284: LRESULT OnGetObject(UINT
message, WPARAM wparamm, LPARAM lparam);
On 2011/10/25 20:08:45, sky wrote:
> wparamm -> wparam

Done.

http://codereview.chromium.org/8391010/diff/4001/views/accessibility/native_v...
File views/accessibility/native_view_accessibility_win.cc (right):

http://codereview.chromium.org/8391010/diff/4001/views/accessibility/native_v...
views/accessibility/native_view_accessibility_win.cc:214: // Remain with the
same dispatch.
On 2011/10/25 20:08:45, sky wrote:
> Update this comment. Also document who does the release.

I'll move this to a separate changelist, it's not necessary for this change and
could use more testing.

http://codereview.chromium.org/8391010/diff/4001/views/accessibility/native_v...
views/accessibility/native_view_accessibility_win.cc:861: if (nav_dir ==
NAVDIR_RIGHT || nav_dir == NAVDIR_DOWN ||
On 2011/10/25 20:08:45, sky wrote:
> nit: make this the return statement, eg:
> 
> return (nav_dir == .. || ... );

Done.

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
File views/widget/native_widget_win.cc (right):

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
views/widget/native_widget_win.cc:449: long unique_id = -child_id;
On 2011/10/25 20:08:45, sky wrote:
> Why do you negate this?

It's explained in SendNativeAccessibilityEvent; I added a comment pointing
there.

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
views/widget/native_widget_win.cc:574: void NativeWidgetWin::ViewRemoved(View*
view) {
On 2011/10/25 20:08:45, sky wrote:
> Don't you need to do this for all descendants of view too?

Doesn't apply anymore now that we search.

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
views/widget/native_widget_win.cc:581: HRESULT hr = accessible->QueryInterface(
On 2011/10/25 20:08:45, sky wrote:
> How expensive is this? Would it be better to maintain the inverse mapping
> (view->id)?

It couldn't be that expensive - it's called thousands of times per second when
accessibility is enabled.

Maintaining view->id would be just as ugly as maintaining id->view, we'd have to
update it every time the view hierarchy changed.

Since I changed it to search the view hierarchy, QueryInterface will now only be
called once per accessibility notification (these are rare, one or two per user
event like a click), and additional calls will only happen if assistive
technology is running and triggers GetViewByAccessibleId.

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
File views/widget/native_widget_win.h (right):

http://codereview.chromium.org/8391010/diff/4001/views/widget/native_widget_w...
views/widget/native_widget_win.h:110: View* GetAccessibilityViewFromChildId(long
id);
On 2011/10/25 20:08:45, sky wrote:
> How about naming this GetViewByAccessibleId().
> I don't like having to keep the map up to date. Is it possible to have this
walk
> the tree each time it's invoked?

Sure - since the view hierarchy is pretty small, I don't see a problem with
this. I added a helper function to search recursively.

[sky, 2011-10-26 21:16:33]

Nice. LGTM

http://codereview.chromium.org/8391010/diff/4009/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/8391010/diff/4009/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:314: :
omnibox_view_win_(omnibox_view_win) { }
nit: '{ }' -> '{}'

[David Tseng, 2011-10-27 20:42:15]

lgtm

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/8391010/diff/4001/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:963:
native_view_host_->set_focus_view(parent);
On 2011/10/26 16:46:13, Dominic Mazzoni wrote:
> On 2011/10/25 21:55:25, David Tseng wrote:
> > On Windows, is the focused view the parent? (LocationBarView). Perhaps it
> should
> > be the NativeViewHost?
> 
> I think this doesn't affect native focus, it just indicates that the parent is
> the one keeping track of views focus.



Ok; I did see two native IAccessible's with focused state; not sure if this is
an issue, but at least with screen readers, things appear fine.

[dmazzoni, 2011-10-28 19:59:15]

FYI, I decided to revert the changes to native_widget_win for now. Trying to use
the unique ID meant that simply firing an accessibility event resulted in
actually creating an accessibility COM object for a view even if no adaptive
tech is running, which is a much larger added cost and also messes with some
tests.

I'll look for a better solution in a future CL - storing the accessibility ID in
the View seems ugly.

Please have another quick look if you want.

[sky, 2011-10-28 22:14:36]

Bummer. I liked less code.

LGTM