Fire accessibility focus/blur events on root web document.

Fire accessibility focus/blur events on root web document.

When the aura Window containing the RWHVA gets or loses focus,
we must fire the corresponding accessibility event on that
object, which corresponds to the root "document" in the web
hierarchy. This worked before the Aura switch because native
HWNDs fired these events automatically.

BUG=347448
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261495

[dmazzoni, 2014-04-02 21:05:09]

+jam for content/

@dtseng: FYI, I tested this with NVDA and confirmed this fixes the reported
issue.

[David Tseng, 2014-04-02 22:05:05]

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager.cc (left):

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:89: void
BrowserAccessibilityManager::GotFocus(bool touch_event_context) {
Was this unused?

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:95: if (focus_)
|focus_| could be anything in the accessibility tree; name of this method leads
one to believe it should be fired on the "window" (root of accessibility tree).
This goes for OnWindowFocused as well.

Also, is this behavior we want for all platforms? I know it's not used by any
other platforms at the moment.

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.h:91: // view lost
focus.
nit: View.
Also, what view?

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager_win.cc:103:
BrowserAccessibilityManager::OnWindowFocused();
Does ordering matter here? If I were a client, I may expect to see the root
focused first, then the descendant node focused.

[dmazzoni, 2014-04-02 22:09:54]

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager.cc (left):

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:89: void
BrowserAccessibilityManager::GotFocus(bool touch_event_context) {
On 2014/04/02 22:05:05, David Tseng wrote:
> Was this unused?

This used to be called from render_widget_host_view_win.cc, which doesn't exist
anymore.

The touch_event_context flag was from old Win 8 touch support logic, currently
unused.

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager.cc:95: if (focus_)
On 2014/04/02 22:05:05, David Tseng wrote:
> Also, is this behavior we want for all platforms? I know it's not used by any
> other platforms at the moment.

The way I implemented it, all platforms will fire a focus event on the actual
element that has focus - I believe that's correct everywhere. Only on Windows do
we extend it to also fire a focus event on the root of the tree first.

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager_win.cc:103:
BrowserAccessibilityManager::OnWindowFocused();
On 2014/04/02 22:05:05, David Tseng wrote:
> Does ordering matter here? If I were a client, I may expect to see the root
> focused first, then the descendant node focused.

Yes, that's the order you'll get: first root, then the descendant (fired by the
parent class).

[David Tseng, 2014-04-02 22:19:51]

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_manager_win.cc:107: if
(focus_ != root_)
Sorry; this was the one I meant to comment on; seems we should blur the focused
node first then the root right?

[dmazzoni, 2014-04-02 22:30:20]

On 2014/04/02 22:19:51, David Tseng wrote:
>
https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
> File content/browser/accessibility/browser_accessibility_manager_win.cc
(right):
> 
>
https://codereview.chromium.org/214243005/diff/1/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility_manager_win.cc:107: if
> (focus_ != root_)
> Sorry; this was the one I meant to comment on; seems we should blur the
focused
> node first then the root right?

Oh! Good point. OK, I reversed the order of blur and added a comment indicating
it's intentional.

[David Tseng, 2014-04-02 23:11:06]

LGTM


On Wed, Apr 2, 2014 at 3:30 PM, <dmazzoni@chromium.org> wrote:

> On 2014/04/02 22:19:51, David Tseng wrote:
>
> https://codereview.chromium.org/214243005/diff/1/content/
> browser/accessibility/browser_accessibility_manager_win.cc
>
>> File content/browser/accessibility/browser_accessibility_manager_win.cc
>>
> (right):
>
>
> https://codereview.chromium.org/214243005/diff/1/content/
> browser/accessibility/browser_accessibility_manager_win.cc#newcode107
>
>> content/browser/accessibility/browser_accessibility_manager_win.cc:107:
>> if
>> (focus_ != root_)
>> Sorry; this was the one I meant to comment on; seems we should blur the
>>
> focused
>
>> node first then the root right?
>>
>
> Oh! Good point. OK, I reversed the order of blur and added a comment
> indicating
> it's intentional.
>
>
>
> https://codereview.chromium.org/214243005/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-04-02 23:45:25]

content/browser/renderer_host/render_widget_host_view_aura.cc lgtm

[dmazzoni, 2014-04-03 05:23:57]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-04-03 05:25:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/214243005/20001

[commit-bot: I haz the power, 2014-04-03 07:42:38]

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

[commit-bot: I haz the power, 2014-04-03 07:42:38]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[dmazzoni, 2014-04-03 16:17:09]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-04-03 16:17:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/214243005/40001

[commit-bot: I haz the power, 2014-04-03 19:00:30]

Change committed as 261495