More NVDA browse mode fixes

content/browser/accessibility/browser_accessibility_manager_win.cc

Index: content/browser/accessibility/browser_accessibility_manager_win.cc
diff --git a/content/browser/accessibility/browser_accessibility_manager_win.cc b/content/browser/accessibility/browser_accessibility_manager_win.cc
index 0efdb3c54a9665dde4f1222bdce0bc052af8ba6b..e8f694b65a165899c52394c91c0469acc0858a44 100644
--- a/content/browser/accessibility/browser_accessibility_manager_win.cc
+++ b/content/browser/accessibility/browser_accessibility_manager_win.cc
@@ -40,7 +40,8 @@ BrowserAccessibilityManagerWin::BrowserAccessibilityManagerWin(
       parent_hwnd_(NULL),
       parent_iaccessible_(parent_iaccessible),
       tracked_scroll_object_(NULL),
-      accessible_hwnd_(accessible_hwnd) {
+      accessible_hwnd_(accessible_hwnd),
+      focus_event_on_root_needed_(false) {
   ui::win::CreateATLModuleIfNeeded();
   if (accessible_hwnd_) {
     accessible_hwnd_->set_browser_accessibility_manager(this);
@@ -83,18 +84,19 @@ void BrowserAccessibilityManagerWin::SetAccessibleHWND(
 
 void BrowserAccessibilityManagerWin::MaybeCallNotifyWinEvent(DWORD event,
                                                              LONG child_id) {
-  // Don't fire events if this view isn't hooked up to its parent.
-  if (!parent_iaccessible() || !parent_hwnd())
-    return;
-
   // If on Win 7 and complete accessibility is enabled, use the fake child HWND

[aboxhall, 2014/06/18 18:25:29]

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.

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

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.
That hasn't changed - happy to fix in a subsequent change.

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

Sure, SGTM.

   // to use as the root of the accessibility tree. See comments above
   // LegacyRenderWidgetHostHWND for details.
-  if (BrowserAccessibilityStateImpl::GetInstance()->IsAccessibleBrowser()) {
-    DCHECK(accessible_hwnd_);
+  if (accessible_hwnd_ &&
+      BrowserAccessibilityStateImpl::GetInstance()->IsAccessibleBrowser()) {
     parent_hwnd_ = accessible_hwnd_->hwnd();
     parent_iaccessible_ = accessible_hwnd_->window_accessible();
   }
+
+  // Don't fire events if this view isn't hooked up to its parent.
+  if (!parent_iaccessible() || !parent_hwnd())

[aboxhall, 2014/06/18 18:25:29]

Not much point early outing here any more.

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

Done.

+    return;
+
   ::NotifyWinEvent(event, parent_hwnd(), OBJID_CLIENT, child_id);
 }
 
@@ -120,17 +122,27 @@ void BrowserAccessibilityManagerWin::OnNodeWillBeDeleted(ui::AXNode* node) {
 }
 
 void BrowserAccessibilityManagerWin::OnWindowFocused() {
-  // Fire a focus event on the root first and then the focused node.
+  // This is called either when this web frame gets focused, or when
+  // the root of the accessibility tree changes. In both cases, we need
+  // to fire a focus event on the root and then on the focused element
+  // within the page, if different.
+  //

[aboxhall, 2014/06/18 18:25:29]

Is the next comment a continuation of this one? It doesn't seem to be, to my
hasty reading.

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

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

+  // Set this flag so that we'll keep trying to fire these focus events
+  // if they're not successful this time.
+  focus_event_on_root_needed_ = true;
+
+  if (!delegate_ || !delegate_->AccessibilityViewHasFocus())
+    return;
+
+  // Try to fire a focus event on the root first and then the focused node.
+  // This will clear focus_event_on_root_needed_ if successful.
   if (focus_ != tree_->GetRoot())
     NotifyAccessibilityEvent(ui::AX_EVENT_FOCUS, GetRoot());
   BrowserAccessibilityManager::OnWindowFocused();
 }
 
 void BrowserAccessibilityManagerWin::OnWindowBlurred() {
-  // Fire a blur event on the focused node first and then the root.
-  BrowserAccessibilityManager::OnWindowBlurred();
-  if (focus_ != tree_->GetRoot())
-    NotifyAccessibilityEvent(ui::AX_EVENT_BLUR, GetRoot());
+  // Windows doesn't have a specific blur event.

[aboxhall, 2014/06/18 18:25:29]

Inline this?

Was what was there previously wrong/not working?

[dmazzoni, 2014/06/18 22:58:32]

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.

 }
 
 void BrowserAccessibilityManagerWin::NotifyAccessibilityEvent(
@@ -139,15 +151,33 @@ void BrowserAccessibilityManagerWin::NotifyAccessibilityEvent(
   if (node->GetRole() == ui::AX_ROLE_INLINE_TEXT_BOX)
     return;
 
+  // Don't fire focus, blur, or load complete notifications if the
+  // window isn't focused, because that can confuse screen readers into
+  // entering their "browse" mode.
+  if ((event_type == ui::AX_EVENT_FOCUS ||
+       event_type == ui::AX_EVENT_BLUR ||
+       event_type == ui::AX_EVENT_LOAD_COMPLETE) &&
+      (!delegate_ || !delegate_->AccessibilityViewHasFocus())) {
+    return;
+  }
+
   // NVDA gets confused if we focus the main document element when it hasn't
   // finished loading and it has no children at all, so suppress that event.
   if (event_type == ui::AX_EVENT_FOCUS &&
       node == GetRoot() &&
       node->PlatformChildCount() == 0 &&
+      !node->HasState(ui::AX_STATE_BUSY) &&
       !node->GetBoolAttribute(ui::AX_ATTR_DOC_LOADED)) {
     return;
   }
 
+  // If a focus event is needed on the root, fire that first before
+  // this event.
+  if (event_type == ui::AX_EVENT_FOCUS && node == GetRoot())
+    focus_event_on_root_needed_ = false;
+  else if (focus_event_on_root_needed_)
+    OnWindowFocused();
+
   LONG event_id = EVENT_MIN;
   switch (event_type) {
     case ui::AX_EVENT_ACTIVEDESCENDANTCHANGED:
@@ -235,10 +265,6 @@ void BrowserAccessibilityManagerWin::NotifyAccessibilityEvent(
     // object and pass it that same id, which we can use to retrieve the
     // IAccessible for this node.
     LONG child_id = node->ToBrowserAccessibilityWin()->unique_id_win();
-
-    // Always send a focus before a load complete.
-    if (event_type == ui::AX_EVENT_LOAD_COMPLETE)
-      MaybeCallNotifyWinEvent(EVENT_OBJECT_FOCUS, child_id);
     MaybeCallNotifyWinEvent(event_id, child_id);
   }
 
@@ -257,8 +283,9 @@ void BrowserAccessibilityManagerWin::NotifyAccessibilityEvent(
 }
 
 void BrowserAccessibilityManagerWin::OnRootChanged(ui::AXNode* new_root) {
-  if (delegate_ && delegate_->AccessibilityViewHasFocus())
-    NotifyAccessibilityEvent(ui::AX_EVENT_FOCUS, GetRoot());
+  // In order to make screen readers aware of the new accessibility root,
+  // we need to fire a focus event on it.
+  OnWindowFocused();
 }
 
 void BrowserAccessibilityManagerWin::TrackScrollingObject(