Fix Chrome OS virtual keyboard accessibility

Fix Chrome OS virtual keyboard accessibility

The user-visible change here is that Select-to-speak on Chrome OS now
works on the virtual keyboard. It also makes it visible to ChromeVox
so we can make that work better too.

Three fixes were required:

* We need to always set the window property on the window for a
  RenderWidgetHostViewAura, whether that WebContents already has
  accessibility enabled or not. That's solved by moving the
  trigger for the window property to RenderViewReady and
  WebContentsImpl::NotifySwappedFromRenderManager.

* AXAuraObjCache needs to listen for newly-created windows,
  changes to window bounds, and window property changes.
  Otherwise the virtual keyboard window is created but no
  accessibility events fire allowing automation clients to
  find it.

* The bounds for a AXWindowObjWrapper should be global bounds and not
  local. This wasn't caught before because widget bounds were correct,
  but the virtual keyboard is directly in an aura Window, with no Widget
  and no Views.

BUG=699617
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2803823002
Cr-Commit-Position: refs/heads/master@{#468182}
Committed: https://chromium.googlesource.com/chromium/src/+/621114575ac090c0137ac4c9c6eb3c6deebfe61c

[dmazzoni, 2017-04-05 21:00:52]

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

[dmazzoni, 2017-04-05 21:01:22]

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

[commit-bot: I haz the power, 2017-04-05 21:01:42]

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, 2017-04-05 21:09:34]

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

[commit-bot: I haz the power, 2017-04-05 21:09:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[David Tseng, 2017-04-05 23:43:43]

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc
(right):

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:135:
ax_obj->HandleAccessibleAction(action);
I think this should happen inside of the extension using the automation bindings
in js. There are other clients like ChromeVox that might optionally want this
type of behavior for speak what's under the mouse.

[David Tseng, 2017-04-06 16:45:44]

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc
(right):

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:150:
nullptr, nullptr, ui::AX_EVENT_MOUSE_CANCELED);
Btw, it seems like a hack to do this via ax events. There's no node associated
with the event and no browser context. I know I'm guilty of this as well, but it
does seem like ax events are being used for everything now when there are
existing apis already serving this purpose. You could forward mouse events
directly to the extension as DOM events.

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:464:
content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
I don't understand why this is necessary if the hosting view has a child tree
id. We enable the child tree id extension side. This is still needed I think
because of the reset case. When an extension is toggled off then on again, with
a renderer that already has accessibility enabled, we need to reset and resend
the renderer's tree contents. Does that make sense?

https://codereview.chromium.org/2803823002/diff/1/ui/views/accessibility/ax_w...
File ui/views/accessibility/ax_widget_obj_wrapper.cc (right):

https://codereview.chromium.org/2803823002/diff/1/ui/views/accessibility/ax_w...
ui/views/accessibility/ax_widget_obj_wrapper.cc:60: bool
AXWidgetObjWrapper::HandleAccessibleAction(
I think this is the wrong level of abstraction to handle this.

An AXHostDelegate handles actions on behalf of a tree source. So, ideally, this
would belong in the AXHostDelegates for web (RenderFrameHost), desktop
(AutomationMangerAura), arc (ArcAccessibilityHelperBridge)

[dmazzoni, 2017-04-06 19:26:03]

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:464:
content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
On 2017/04/06 16:45:43, David Tseng wrote:
> I don't understand why this is necessary if the hosting view has a child tree
> id. We enable the child tree id extension side. This is still needed I think
> because of the reset case. When an extension is toggled off then on again,
with
> a renderer that already has accessibility enabled, we need to reset and resend
> the renderer's tree contents. Does that make sense?

I'm open to other solutions but I couldn't come up with something else
simple.

Currently the way we enable accessibility for most frames is that
we detect the ui::AX_ROLE_WEB_VIEW, cast it to a views::WebView,
and get a pointer to the WebContents.

In this particular case, the virtual keyboard is a pure aura WIndow
on the screen, it's not inside of a WebView or any View at all.

How are we going to know that this aura Window is really a
RenderWidgetHostViewAura? And if so how would we get its
tree id? Note that on creation it doesn't typically have a
routing id yet, so we'd have to be able to listen for a callback
to know when it's ready and fire an accessibility event on it.

One "clean" solution would be to add some accessibility
support to aura::Window, but that seems kind of heavyweight.
I didn't want to go that route if there was a simpler solution
since this is just a rare corner case.

Can you think of a more straightforward way to do it?
I'd be especially interested if you could come up with a way to
do it without magic strings and typecasting.

Finally, aside from the reason why I did it this way, once the extension
knows about the child tree, wouldn't it still be able to reset it if needed?
It's not clear to me how calling EnableAccessibility here means that
we can't later reset.

[dmazzoni, 2017-04-06 19:26:57]

BTW, your other suggestion was to forward the mouse event
to the extension and have the extension fire the hit test action.
As far as I know that should work fine, I'll do that.

The only open question is the best way to get access to the
virtual keyboard WebContents since it isn't inside of a View.

[David Tseng, 2017-04-06 20:00:32]

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:464:
content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
On 2017/04/06 19:26:03, dmazzoni wrote:
> On 2017/04/06 16:45:43, David Tseng wrote:
> > I don't understand why this is necessary if the hosting view has a child
tree
> > id. We enable the child tree id extension side. This is still needed I think
> > because of the reset case. When an extension is toggled off then on again,
> with
> > a renderer that already has accessibility enabled, we need to reset and
resend
> > the renderer's tree contents. Does that make sense?
> 
> I'm open to other solutions but I couldn't come up with something else
> simple.
> 
> Currently the way we enable accessibility for most frames is that
> we detect the ui::AX_ROLE_WEB_VIEW, cast it to a views::WebView,
> and get a pointer to the WebContents.
> 
> In this particular case, the virtual keyboard is a pure aura WIndow
> on the screen, it's not inside of a WebView or any View at all.

What happened with storing the child tree id as a window property? I thought you
had a cl out for that?

[dmazzoni, 2017-04-06 20:16:41]

Yes, the RenderFrameHostImpl sets the window property...but only if
accessibility is enabled, when it gets an accessibility event. So we still
have a chicken-and-egg problem.

I tried to set the window property more generally, but there are several
complications:
* The aura Window is owned by the RenderWidgetHostViewAura, which has very
little visibility into the RenderFrameHostImpl. The RenderFrameHostImpl has
the AXTreeID, but it has very little visibility into
RenderWidgetHostViewAura. They can be initialized in either order -
sometimes the routing ID and therefore the AXTreeID come first, before we
have a view. Other times we have a view first and then we get the routing
ID. There aren't existing notifications to easily hook into, we'd be
forging new ground here. Not saying it isn't doable, just that it's a bit
of a quagmire.
* Even if we could set the window property, how do we notify the
AXWindowObjWrapper to fire a notification?


On Thu, Apr 6, 2017 at 1:00 PM <dtseng@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2803823002/diff/1/chrome/browser/extensions/a...
> File
>
> chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
> (right):
>
>
>
https://codereview.chromium.org/2803823002/diff/1/chrome/browser/extensions/a...
>
>
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:464:
> content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
> On 2017/04/06 19:26:03, dmazzoni wrote:
> > On 2017/04/06 16:45:43, David Tseng wrote:
> > > I don't understand why this is necessary if the hosting view has a
> child tree
> > > id. We enable the child tree id extension side. This is still needed
> I think
> > > because of the reset case. When an extension is toggled off then on
> again,
> > with
> > > a renderer that already has accessibility enabled, we need to reset
> and resend
> > > the renderer's tree contents. Does that make sense?
> >
> > I'm open to other solutions but I couldn't come up with something else
> > simple.
> >
> > Currently the way we enable accessibility for most frames is that
> > we detect the ui::AX_ROLE_WEB_VIEW, cast it to a views::WebView,
> > and get a pointer to the WebContents.
> >
> > In this particular case, the virtual keyboard is a pure aura WIndow
> > on the screen, it's not inside of a WebView or any View at all.
>
> What happened with storing the child tree id as a window property? I
> thought you had a cl out for that?
>
> https://codereview.chromium.org/2803823002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-04-06 20:43:35]

Ok, makes sense. We're not really trying to get the wrapper objects to fire
a notification, but I think we want to trigger notifications when there's
window events like we have for views. We could write a separate class that
monitors window events from ash and convert them to accessibility events.
It seems like we rely upon something triggering the notification inside of
the window which might not happen for other things in the future.
 In practical terms, when the window gets activated, the render widget host
for the vk should be available, right?

To reply to your other question regarding
BrowserAccessibilityState::EnableAccessibility:
it's not equivalent with the current code. The above sets mode complete
while automation only sets web contents only mode. Also, we'll need to
handle reset.
Basically, you need to do something equivalent to
web_contents_impl::WebContentsImpl::EnableWebContentsOnlyAccessibilityMode.

So you might just want to add a different method in
BrowserAccessibilityState for enabling web contents only accessibility
(excludes native apis and whatever else we need only for other platforms).

I'm fine with either option though I guess the second would be more in
scope for this cl.


On Thu, Apr 6, 2017 at 1:16 PM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> Yes, the RenderFrameHostImpl sets the window property...but only if
> accessibility is enabled, when it gets an accessibility event. So we still
> have a chicken-and-egg problem.
>
> I tried to set the window property more generally, but there are several
> complications:
> * The aura Window is owned by the RenderWidgetHostViewAura, which has very
> little visibility into the RenderFrameHostImpl. The RenderFrameHostImpl has
> the AXTreeID, but it has very little visibility into
> RenderWidgetHostViewAura. They can be initialized in either order -
> sometimes the routing ID and therefore the AXTreeID come first, before we
> have a view. Other times we have a view first and then we get the routing
> ID. There aren't existing notifications to easily hook into, we'd be
> forging new ground here. Not saying it isn't doable, just that it's a bit
> of a quagmire.
> * Even if we could set the window property, how do we notify the
> AXWindowObjWrapper to fire a notification?
>
>
> On Thu, Apr 6, 2017 at 1:00 PM <dtseng@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/2803823002/diff/1/chrome/
>> browser/extensions/api/automation_internal/automation_internal_api.cc
>> File
>> chrome/browser/extensions/api/automation_internal/
>> automation_internal_api.cc
>> (right):
>>
>> https://codereview.chromium.org/2803823002/diff/1/chrome/
>> browser/extensions/api/automation_internal/automation_internal_api.cc#
>> newcode464
>> chrome/browser/extensions/api/automation_internal/
>> automation_internal_api.cc:464:
>> content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
>> On 2017/04/06 19:26:03, dmazzoni wrote:
>> > On 2017/04/06 16:45:43, David Tseng wrote:
>> > > I don't understand why this is necessary if the hosting view has a
>> child tree
>> > > id. We enable the child tree id extension side. This is still needed
>> I think
>> > > because of the reset case. When an extension is toggled off then on
>> again,
>> > with
>> > > a renderer that already has accessibility enabled, we need to reset
>> and resend
>> > > the renderer's tree contents. Does that make sense?
>> >
>> > I'm open to other solutions but I couldn't come up with something else
>> > simple.
>> >
>> > Currently the way we enable accessibility for most frames is that
>> > we detect the ui::AX_ROLE_WEB_VIEW, cast it to a views::WebView,
>> > and get a pointer to the WebContents.
>> >
>> > In this particular case, the virtual keyboard is a pure aura WIndow
>> > on the screen, it's not inside of a WebView or any View at all.
>>
>> What happened with storing the child tree id as a window property? I
>> thought you had a cl out for that?
>>
>> https://codereview.chromium.org/2803823002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to chromium-reviews+unsubscribe@chromium.org.
>>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2017-04-07 23:25:49]

Description was changed from

==========
Support hit test in views accessibility, for Select-to-speak of virtual kbd.

The user-visible change here is that Select-to-speak on Chrome OS now
works on the virtual keyboard.

The main technical reason it didn't work before is that we hadn't finished
implementing hit testing across all of ui/views/accessibility. This change
adds support for ui::AX_ACTION_HIT_TEST, which forwards the action to the
deepest descendant and then fires a given accessibility event on the hit
object. That allows us to do a hit test starting from the whole desktop and
hit a window, then a widget, then a view. The event that fires on the hit
object can then be caught by select-to-speak, or any other accessibility
service.

This change does not yet forward the hit test to the web, that will be in a
follow-up change, but this is a step in the right direction.

Another related change here is to enable accessibility for all frames when
an automation extension requests the desktop. That way we get the virtual
keyboard's WebContents even though it isn't in a WebView.

Finally this fixes a bug in the bounds for a AXWindowObjWrapper, which
should be global bounds and not local. This wasn't caught before because
widget bounds were correct, but the virtual keyboard is directly in an
aura Window, with no Widget and no Views.

BUG=699617
==========

to

==========
Support hit test in views accessibility, for Select-to-speak of virtual kbd.

The user-visible change here is that Select-to-speak on Chrome OS now
works on the virtual keyboard.

The main technical reason it didn't work before is that we hadn't finished
implementing hit testing across all of ui/views/accessibility. This change
adds support for ui::AX_ACTION_HIT_TEST, which forwards the action to the
deepest descendant and then fires a given accessibility event on the hit
object. That allows us to do a hit test starting from the whole desktop and
hit a window, then a widget, then a view. The event that fires on the hit
object can then be caught by select-to-speak, or any other accessibility
service.

This change does not yet forward the hit test to the web, that will be in a
follow-up change, but this is a step in the right direction.

Another related change here is to enable accessibility for all frames when
an automation extension requests the desktop. That way we get the virtual
keyboard's WebContents even though it isn't in a WebView.

Finally this fixes a bug in the bounds for a AXWindowObjWrapper, which
should be global bounds and not local. This wasn't caught before because
widget bounds were correct, but the virtual keyboard is directly in an
aura Window, with no Widget and no Views.

BUG=699617
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dmazzoni, 2017-04-07 23:34:09]

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc
(right):

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:135:
ax_obj->HandleAccessibleAction(action);
On 2017/04/05 23:43:43, David Tseng wrote:
> I think this should happen inside of the extension using the automation
bindings
> in js. There are other clients like ChromeVox that might optionally want this
> type of behavior for speak what's under the mouse.

I spend all day trying to make this work. It's slightly
harder to forward a mouse event than a keyboard event
due to coordinate transformations, but it works, for
mouse down and mouse up. But mouse move just doesn't work,
there's some throttling code, probably designed to keep
background pages from wasting processing on mouse move
events.

There may be a way around it, but I'm thinking this might
be the wrong approach.

Options:
* Use a private API to send the mouse event to the extension
* Keep this existing code for now

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
(right):

https://codereview.chromium.org/2803823002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:464:
content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
On 2017/04/06 20:00:32, David Tseng wrote:
> On 2017/04/06 19:26:03, dmazzoni wrote:
> > On 2017/04/06 16:45:43, David Tseng wrote:
> > > I don't understand why this is necessary if the hosting view has a child
> tree
> > > id. We enable the child tree id extension side. This is still needed I
think
> > > because of the reset case. When an extension is toggled off then on again,
> > with
> > > a renderer that already has accessibility enabled, we need to reset and
> resend
> > > the renderer's tree contents. Does that make sense?
> > 
> > I'm open to other solutions but I couldn't come up with something else
> > simple.
> > 
> > Currently the way we enable accessibility for most frames is that
> > we detect the ui::AX_ROLE_WEB_VIEW, cast it to a views::WebView,
> > and get a pointer to the WebContents.
> > 
> > In this particular case, the virtual keyboard is a pure aura WIndow
> > on the screen, it's not inside of a WebView or any View at all.
> 
> What happened with storing the child tree id as a window property? I thought
you
> had a cl out for that?

OK, see latest patch set. I figured out a way to do it.

https://codereview.chromium.org/2803823002/diff/1/ui/views/accessibility/ax_w...
File ui/views/accessibility/ax_widget_obj_wrapper.cc (right):

https://codereview.chromium.org/2803823002/diff/1/ui/views/accessibility/ax_w...
ui/views/accessibility/ax_widget_obj_wrapper.cc:60: bool
AXWidgetObjWrapper::HandleAccessibleAction(
On 2017/04/06 16:45:44, David Tseng wrote:
> I think this is the wrong level of abstraction to handle this.
> 
> An AXHostDelegate handles actions on behalf of a tree source. So, ideally,
this
> would belong in the AXHostDelegates for web (RenderFrameHost), desktop
> (AutomationMangerAura), arc (ArcAccessibilityHelperBridge)

No, because a HIT_TEST is fired on the root. We just have coordinates,
we don't know whether to delegate it to a window, a widget, or
a particular frame yet.

Yes, the AXHostDelegate should handle the HIT_TEST action, but it
should forward it to its root node to handle from there.

[David Tseng, 2017-04-08 00:06:48]

I'm fine with keeping it as is for now with a TODO to make it work with the
forwarded DOM event.

I think the advantage of the DOM api/MouseEvent structure is we can get for
free stuff like
altKey, metaKey, movementX, movementY, which, force, etc

which all seem super useful for developing cool features in Select to Speak
and in ChromeVox.


On Fri, Apr 7, 2017 at 4:34 PM, <dmazzoni@chromium.org> wrote:

>
> https://codereview.chromium.org/2803823002/diff/1/chrome/browser/chromeos/
> accessibility/select_to_speak_event_handler.cc
> File
> chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc
> (right):
>
> https://codereview.chromium.org/2803823002/diff/1/chrome/browser/chromeos/
> accessibility/select_to_speak_event_handler.cc#newcode135
> chrome/browser/chromeos/accessibility/select_to_speak_
> event_handler.cc:135:
> ax_obj->HandleAccessibleAction(action);
> On 2017/04/05 23:43:43, David Tseng wrote:
> > I think this should happen inside of the extension using the
> automation bindings
> > in js. There are other clients like ChromeVox that might optionally
> want this
> > type of behavior for speak what's under the mouse.
>
> I spend all day trying to make this work. It's slightly
> harder to forward a mouse event than a keyboard event
> due to coordinate transformations, but it works, for
> mouse down and mouse up. But mouse move just doesn't work,
> there's some throttling code, probably designed to keep
> background pages from wasting processing on mouse move
> events.
>
> There may be a way around it, but I'm thinking this might
> be the wrong approach.
>
> Options:
> * Use a private API to send the mouse event to the extension
> * Keep this existing code for now
>
> https://codereview.chromium.org/2803823002/diff/1/chrome/
> browser/extensions/api/automation_internal/automation_internal_api.cc
> File
> chrome/browser/extensions/api/automation_internal/
> automation_internal_api.cc
> (right):
>
> https://codereview.chromium.org/2803823002/diff/1/chrome/
> browser/extensions/api/automation_internal/automation_internal_api.cc#
> newcode464
> chrome/browser/extensions/api/automation_internal/
> automation_internal_api.cc:464:
> content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
> On 2017/04/06 20:00:32, David Tseng wrote:
> > On 2017/04/06 19:26:03, dmazzoni wrote:
> > > On 2017/04/06 16:45:43, David Tseng wrote:
> > > > I don't understand why this is necessary if the hosting view has a
> child
> > tree
> > > > id. We enable the child tree id extension side. This is still
> needed I think
> > > > because of the reset case. When an extension is toggled off then
> on again,
> > > with
> > > > a renderer that already has accessibility enabled, we need to
> reset and
> > resend
> > > > the renderer's tree contents. Does that make sense?
> > >
> > > I'm open to other solutions but I couldn't come up with something
> else
> > > simple.
> > >
> > > Currently the way we enable accessibility for most frames is that
> > > we detect the ui::AX_ROLE_WEB_VIEW, cast it to a views::WebView,
> > > and get a pointer to the WebContents.
> > >
> > > In this particular case, the virtual keyboard is a pure aura WIndow
> > > on the screen, it's not inside of a WebView or any View at all.
> >
> > What happened with storing the child tree id as a window property? I
> thought you
> > had a cl out for that?
>
> OK, see latest patch set. I figured out a way to do it.
>
> https://codereview.chromium.org/2803823002/diff/1/ui/
> views/accessibility/ax_widget_obj_wrapper.cc
> File ui/views/accessibility/ax_widget_obj_wrapper.cc (right):
>
> https://codereview.chromium.org/2803823002/diff/1/ui/
> views/accessibility/ax_widget_obj_wrapper.cc#newcode60
> ui/views/accessibility/ax_widget_obj_wrapper.cc:60: bool
> AXWidgetObjWrapper::HandleAccessibleAction(
> On 2017/04/06 16:45:44, David Tseng wrote:
> > I think this is the wrong level of abstraction to handle this.
> >
> > An AXHostDelegate handles actions on behalf of a tree source. So,
> ideally, this
> > would belong in the AXHostDelegates for web (RenderFrameHost), desktop
> > (AutomationMangerAura), arc (ArcAccessibilityHelperBridge)
>
> No, because a HIT_TEST is fired on the root. We just have coordinates,
> we don't know whether to delegate it to a window, a widget, or
> a particular frame yet.
>
> Yes, the AXHostDelegate should handle the HIT_TEST action, but it
> should forward it to its root node to handle from there.
>
> https://codereview.chromium.org/2803823002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2017-04-12 19:33:53]

Description was changed from

==========
Support hit test in views accessibility, for Select-to-speak of virtual kbd.

The user-visible change here is that Select-to-speak on Chrome OS now
works on the virtual keyboard.

The main technical reason it didn't work before is that we hadn't finished
implementing hit testing across all of ui/views/accessibility. This change
adds support for ui::AX_ACTION_HIT_TEST, which forwards the action to the
deepest descendant and then fires a given accessibility event on the hit
object. That allows us to do a hit test starting from the whole desktop and
hit a window, then a widget, then a view. The event that fires on the hit
object can then be caught by select-to-speak, or any other accessibility
service.

This change does not yet forward the hit test to the web, that will be in a
follow-up change, but this is a step in the right direction.

Another related change here is to enable accessibility for all frames when
an automation extension requests the desktop. That way we get the virtual
keyboard's WebContents even though it isn't in a WebView.

Finally this fixes a bug in the bounds for a AXWindowObjWrapper, which
should be global bounds and not local. This wasn't caught before because
widget bounds were correct, but the virtual keyboard is directly in an
aura Window, with no Widget and no Views.

BUG=699617
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix Chrome OS virtual keyboard accessibility

The user-visible change here is that Select-to-speak on Chrome OS now
works on the virtual keyboard. It also makes it visible to ChromeVox
so we can make that work better too.

Three fixes were required:

* We need to always set the window property on the window for a
  RenderWidgetHostViewAura, whether that WebContents already has
  accessibility enabled or not. That's solved by moving the
  trigger for the window property to RenderViewReady and
  WebContentsImpl::NotifySwappedFromRenderManager.

* AXAuraObjCache needs to listen for newly-created windows,
  changes to window bounds, and window property changes.
  Otherwise the virtual keyboard window is created but no
  accessibility events fire allowing automation clients to
  find it.

* The bounds for a AXWindowObjWrapper should be global bounds and not
  local. This wasn't caught before because widget bounds were correct,
  but the virtual keyboard is directly in an aura Window, with no Widget
  and no Views.

BUG=699617
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dmazzoni, 2017-04-12 19:34:00]

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

[commit-bot: I haz the power, 2017-04-12 19:35:41]

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

[dmazzoni, 2017-04-12 19:35:51]

Please take a look! I pulled out most of the change into
https://codereview.chromium.org/2813083003/ and all
that's left here is the fixes specific to the virtual
keyboard.

The change to forward the MouseEvent will follow.

[commit-bot: I haz the power, 2017-04-12 21:04:08]

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

[commit-bot: I haz the power, 2017-04-12 21:04:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Tseng, 2017-04-17 15:39:54]

https://codereview.chromium.org/2803823002/diff/40001/ui/views/accessibility/...
File ui/views/accessibility/ax_aura_obj_cache.cc (right):

https://codereview.chromium.org/2803823002/diff/40001/ui/views/accessibility/...
ui/views/accessibility/ax_aura_obj_cache.cc:190: void
AXAuraObjCache::OnWindowHierarchyChanged(
I don't know if this is what you really want. This |WindowObserver| gets added
on the focused root window. It's not added for all root windows. For that, you
would need the  activation client belonging to ash shell.

The focus client gets requested each time we use GetOrCreate. I'm really not
sure that's correct either since it depends on the next call to GetOrCreate
which might not happen if there were no events within the subtree for the
window.

I think what we need is for AutomationManagerAura to observe both focus client
and activation client from ash shell similar to compoents/exo/wm_helper_ ash.cc.
Doing it here would not be an option due to DEPS.

[dmazzoni, 2017-04-19 18:07:34]

https://codereview.chromium.org/2803823002/diff/40001/ui/views/accessibility/...
File ui/views/accessibility/ax_aura_obj_cache.cc (right):

https://codereview.chromium.org/2803823002/diff/40001/ui/views/accessibility/...
ui/views/accessibility/ax_aura_obj_cache.cc:190: void
AXAuraObjCache::OnWindowHierarchyChanged(
On 2017/04/17 15:39:54, David Tseng wrote:
> I don't know if this is what you really want. This |WindowObserver| gets added
> on the focused root window. It's not added for all root windows. For that, you
> would need the  activation client belonging to ash shell.

So on Chrome OS, there's only one root window for each screen. We add an
observer for each root window as soon as we encounter it, and never remove
any observers for them, so I think it's reasonable to make use of
OnWindowHierarchyChanged here.

I filed http://crbug.com/713278 to track that.

Let me know if there's anything else I could make more clear in this change.

[David Tseng, 2017-04-19 19:21:31]

lgtm

https://codereview.chromium.org/2803823002/diff/40001/ui/views/accessibility/...
File ui/views/accessibility/ax_aura_obj_cache.cc (right):

https://codereview.chromium.org/2803823002/diff/40001/ui/views/accessibility/...
ui/views/accessibility/ax_aura_obj_cache.cc:190: void
AXAuraObjCache::OnWindowHierarchyChanged(
On 2017/04/19 18:07:34, dmazzoni wrote:
> On 2017/04/17 15:39:54, David Tseng wrote:
> > I don't know if this is what you really want. This |WindowObserver| gets
added
> > on the focused root window. It's not added for all root windows. For that,
you
> > would need the  activation client belonging to ash shell.
> 
> So on Chrome OS, there's only one root window for each screen. We add an
> observer for each root window as soon as we encounter it, and never remove
> any observers for them, so I think it's reasonable to make use of
> OnWindowHierarchyChanged here.
> 
> I filed http://crbug.com/713278 to track that.
> 
> Let me know if there's anything else I could make more clear in this change.

Please add a comment when we call root_window->AddObserver(this)

stating that |root_window| is the root of all Aura windows on a single screen.
It is equivalent to ash shell's primary root window when there is only one
screen.

That is not obvious to me, but that was my understanding from what you said
offline. Thanks.

[dmazzoni, 2017-04-19 21:36:59]

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

[dmazzoni, 2017-04-19 21:37:03]

https://codereview.chromium.org/2803823002/diff/40001/ui/views/accessibility/...
File ui/views/accessibility/ax_aura_obj_cache.cc (right):

https://codereview.chromium.org/2803823002/diff/40001/ui/views/accessibility/...
ui/views/accessibility/ax_aura_obj_cache.cc:190: void
AXAuraObjCache::OnWindowHierarchyChanged(
On 2017/04/19 19:21:31, David Tseng wrote:
> On 2017/04/19 18:07:34, dmazzoni wrote:
> > On 2017/04/17 15:39:54, David Tseng wrote:
> > > I don't know if this is what you really want. This |WindowObserver| gets
> added
> > > on the focused root window. It's not added for all root windows. For that,
> you
> > > would need the  activation client belonging to ash shell.
> > 
> > So on Chrome OS, there's only one root window for each screen. We add an
> > observer for each root window as soon as we encounter it, and never remove
> > any observers for them, so I think it's reasonable to make use of
> > OnWindowHierarchyChanged here.
> > 
> > I filed http://crbug.com/713278 to track that.
> > 
> > Let me know if there's anything else I could make more clear in this change.
> 
> Please add a comment when we call root_window->AddObserver(this)
> 
> stating that |root_window| is the root of all Aura windows on a single screen.
> It is equivalent to ash shell's primary root window when there is only one
> screen.
> 
> That is not obvious to me, but that was my understanding from what you said
> offline. Thanks.

Done.

[commit-bot: I haz the power, 2017-04-19 21:37:55]

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

[dmazzoni, 2017-04-19 21:41:36]

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

[dmazzoni, 2017-04-19 21:41:37]

+jam for content/browser/
(follow-up to https://codereview.chromium.org/2742533003/)

[commit-bot: I haz the power, 2017-04-19 22:28:33]

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

[commit-bot: I haz the power, 2017-04-19 22:28:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2017-04-20 00:17:52]

lgtm

[dmazzoni, 2017-04-21 21:44:07]

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

[commit-bot: I haz the power, 2017-04-21 21:45:02]

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, 2017-04-21 22:41:14]

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

[commit-bot: I haz the power, 2017-04-21 22:41:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2017-04-21 23:33:18]

Description was changed from

==========
Fix Chrome OS virtual keyboard accessibility

The user-visible change here is that Select-to-speak on Chrome OS now
works on the virtual keyboard. It also makes it visible to ChromeVox
so we can make that work better too.

Three fixes were required:

* We need to always set the window property on the window for a
  RenderWidgetHostViewAura, whether that WebContents already has
  accessibility enabled or not. That's solved by moving the
  trigger for the window property to RenderViewReady and
  WebContentsImpl::NotifySwappedFromRenderManager.

* AXAuraObjCache needs to listen for newly-created windows,
  changes to window bounds, and window property changes.
  Otherwise the virtual keyboard window is created but no
  accessibility events fire allowing automation clients to
  find it.

* The bounds for a AXWindowObjWrapper should be global bounds and not
  local. This wasn't caught before because widget bounds were correct,
  but the virtual keyboard is directly in an aura Window, with no Widget
  and no Views.

BUG=699617
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix Chrome OS virtual keyboard accessibility

The user-visible change here is that Select-to-speak on Chrome OS now
works on the virtual keyboard. It also makes it visible to ChromeVox
so we can make that work better too.

Three fixes were required:

* We need to always set the window property on the window for a
  RenderWidgetHostViewAura, whether that WebContents already has
  accessibility enabled or not. That's solved by moving the
  trigger for the window property to RenderViewReady and
  WebContentsImpl::NotifySwappedFromRenderManager.

* AXAuraObjCache needs to listen for newly-created windows,
  changes to window bounds, and window property changes.
  Otherwise the virtual keyboard window is created but no
  accessibility events fire allowing automation clients to
  find it.

* The bounds for a AXWindowObjWrapper should be global bounds and not
  local. This wasn't caught before because widget bounds were correct,
  but the virtual keyboard is directly in an aura Window, with no Widget
  and no Views.

BUG=699617
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:linux_site_isolation
==========

[dmazzoni, 2017-04-21 23:33:23]

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

[commit-bot: I haz the power, 2017-04-21 23:34:11]

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, 2017-04-22 00:32:39]

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

[commit-bot: I haz the power, 2017-04-22 00:32:40]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[dmazzoni, 2017-04-25 23:25:00]

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

[commit-bot: I haz the power, 2017-04-25 23:25:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-04-25 23:38:16]

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

[commit-bot: I haz the power, 2017-04-25 23:39:05]

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, 2017-04-26 00:46:45]

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

[commit-bot: I haz the power, 2017-04-26 00:46:46]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-04-26 02:35:00]

David, in order to get all tests to pass I ended up needing to
fix the issue where a WebContents is parented by both a
WebView and an aura Window. The fix is
a one-liner in AXWindowObjWrapper::Serialize.

I also added a browser test specifically for that issue.

Want to take one more quick look before this lands?

[David Tseng, 2017-04-27 23:03:12]

https://codereview.chromium.org/2803823002/diff/160001/ui/views/accessibility...
File ui/views/accessibility/ax_window_obj_wrapper.cc (right):

https://codereview.chromium.org/2803823002/diff/160001/ui/views/accessibility...
ui/views/accessibility/ax_window_obj_wrapper.cc:68: if
(!window_->GetToplevelWindow() ||
What about this first case? Do we want to add the child tree if the window has
no top level window?

https://codereview.chromium.org/2803823002/diff/160001/ui/views/accessibility...
ui/views/accessibility/ax_window_obj_wrapper.cc:113:
AXAuraObjCache::GetInstance()->FireEvent(this,
Is |this| always the wrapper for |window|?

[dmazzoni, 2017-04-27 23:20:15]

https://codereview.chromium.org/2803823002/diff/160001/ui/views/accessibility...
File ui/views/accessibility/ax_window_obj_wrapper.cc (right):

https://codereview.chromium.org/2803823002/diff/160001/ui/views/accessibility...
ui/views/accessibility/ax_window_obj_wrapper.cc:68: if
(!window_->GetToplevelWindow() ||
On 2017/04/27 23:03:12, David Tseng wrote:
> What about this first case? Do we want to add the child tree if the window has
> no top level window?

I don't think that should ever happen for a legit reason, but
maybe during teardown.

https://codereview.chromium.org/2803823002/diff/160001/ui/views/accessibility...
ui/views/accessibility/ax_window_obj_wrapper.cc:113:
AXAuraObjCache::GetInstance()->FireEvent(this,
On 2017/04/27 23:03:12, David Tseng wrote:
> Is |this| always the wrapper for |window|?

Good catch, added check for window_ == window

[David Tseng, 2017-04-28 22:21:04]

lgtm

[dmazzoni, 2017-04-28 22:22:07]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-04-28 22:22:07]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2803823002/#ps180001
(title: "Check for window_ == window")

[commit-bot: I haz the power, 2017-04-28 22:22:39]

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, 2017-04-28 23:46:58]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493418127388560,
"parent_rev": "d33a171b7c308a64dc3372fac3da2179c63b419e", "commit_rev":
"621114575ac090c0137ac4c9c6eb3c6deebfe61c"}

[commit-bot: I haz the power, 2017-04-28 23:47:12]

Description was changed from

==========
Fix Chrome OS virtual keyboard accessibility

The user-visible change here is that Select-to-speak on Chrome OS now
works on the virtual keyboard. It also makes it visible to ChromeVox
so we can make that work better too.

Three fixes were required:

* We need to always set the window property on the window for a
  RenderWidgetHostViewAura, whether that WebContents already has
  accessibility enabled or not. That's solved by moving the
  trigger for the window property to RenderViewReady and
  WebContentsImpl::NotifySwappedFromRenderManager.

* AXAuraObjCache needs to listen for newly-created windows,
  changes to window bounds, and window property changes.
  Otherwise the virtual keyboard window is created but no
  accessibility events fire allowing automation clients to
  find it.

* The bounds for a AXWindowObjWrapper should be global bounds and not
  local. This wasn't caught before because widget bounds were correct,
  but the virtual keyboard is directly in an aura Window, with no Widget
  and no Views.

BUG=699617
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix Chrome OS virtual keyboard accessibility

The user-visible change here is that Select-to-speak on Chrome OS now
works on the virtual keyboard. It also makes it visible to ChromeVox
so we can make that work better too.

Three fixes were required:

* We need to always set the window property on the window for a
  RenderWidgetHostViewAura, whether that WebContents already has
  accessibility enabled or not. That's solved by moving the
  trigger for the window property to RenderViewReady and
  WebContentsImpl::NotifySwappedFromRenderManager.

* AXAuraObjCache needs to listen for newly-created windows,
  changes to window bounds, and window property changes.
  Otherwise the virtual keyboard window is created but no
  accessibility events fire allowing automation clients to
  find it.

* The bounds for a AXWindowObjWrapper should be global bounds and not
  local. This wasn't caught before because widget bounds were correct,
  but the virtual keyboard is directly in an aura Window, with no Widget
  and no Views.

BUG=699617
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2803823002
Cr-Commit-Position: refs/heads/master@{#468182}
Committed:
https://chromium.googlesource.com/chromium/src/+/621114575ac090c0137ac4c9c6eb...
==========

[commit-bot: I haz the power, 2017-04-28 23:47:13]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/621114575ac090c0137ac4c9c6eb...