More NVDA browse mode fixes

More NVDA browse mode fixes

The previous change (https://codereview.chromium.org/332893004/) wasn't quite right. To avoid errors, we need to only fire focus and load events when the web view itself has focus, not say when the location bar has focus. And because there are transient periods when we can't fire events, we need to keep track of whether we need to fire a focus event with a flag and fire it later.

BUG=368549
NOTRY=true

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

[aboxhall, 2014-06-18 18:25:30]

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:87: // If on
Win 7 and complete accessibility is enabled, use the fake child HWND
Why has been moved above the check below?

The name of this method is now slightly misleading, as it's going to have side
effects whether or not the notification actually occurs.

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:97: if
(!parent_iaccessible() || !parent_hwnd())
Not much point early outing here any more.

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:129: //
Is the next comment a continuation of this one? It doesn't seem to be, to my
hasty reading.

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:145: //
Windows doesn't have a specific blur event.
Inline this?

Was what was there previously wrong/not working?

[dmazzoni, 2014-06-18 22:58:33]

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:87: // If on
Win 7 and complete accessibility is enabled, use the fake child HWND
On 2014/06/18 18:25:29, aboxhall wrote:
> Why has been moved above the check below?

The reason I moved it is because it didn't make sense to bail out if
parent_iaccessible() was NULL if the following block of code would have updated
parent_iaccessible() to something valid. We were basically dropping events for
no reason.

> The name of this method is now slightly misleading, as it's going to have side
> effects whether or not the notification actually occurs.

That hasn't changed - happy to fix in a subsequent change.

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:97: if
(!parent_iaccessible() || !parent_hwnd())
On 2014/06/18 18:25:29, aboxhall wrote:
> Not much point early outing here any more.

Done.

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:129: //
On 2014/06/18 18:25:29, aboxhall wrote:
> Is the next comment a continuation of this one? It doesn't seem to be, to my
> hasty reading.

You're right. Split into two comments: a top comment for the whole function, and
a comment for the first line.

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:145: //
Windows doesn't have a specific blur event.
On 2014/06/18 18:25:29, aboxhall wrote:
> Inline this?
> 
> Was what was there previously wrong/not working?

Done.

On the web, a blur event just means to focus the document body.

What was there before was not working because we were translating BLUR events on
the whole window into FOCUS events on the root element - that was actually
confusing screen readers into thinking the wrong thing had focus.

[aboxhall, 2014-06-18 23:29:23]

lgtm

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager_win.cc (right):

https://codereview.chromium.org/344573003/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager_win.cc:87: // If on
Win 7 and complete accessibility is enabled, use the fake child HWND
On 2014/06/18 22:58:33, dmazzoni wrote:
> > The name of this method is now slightly misleading, as it's going to have
side
> > effects whether or not the notification actually occurs.
> 
> That hasn't changed - happy to fix in a subsequent change.

Sure, SGTM.

[commit-bot: I haz the power, 2014-06-18 23:31:46]

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

[dmazzoni, 2014-06-19 05:59:59]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2014-06-19 06:00:15]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-06-19 06:02:14]

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

[commit-bot: I haz the power, 2014-06-19 07:53:48]

Change committed as 278303