More NVDA browse mode fixes

content/browser/accessibility/browser_accessibility_manager_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/accessibility/browser_accessibility_manager_win.h"
 
 #include "base/command_line.h"
 #include "base/win/scoped_comptr.h"
 #include "base/win/windows_version.h"
 #include "content/browser/accessibility/browser_accessibility_state_impl.h"
@@ skipping 22 matching lines @@
 BrowserAccessibilityManagerWin::BrowserAccessibilityManagerWin(
     LegacyRenderWidgetHostHWND* accessible_hwnd,
     IAccessible* parent_iaccessible,
     const ui::AXTreeUpdate& initial_tree,
     BrowserAccessibilityDelegate* delegate,
     BrowserAccessibilityFactory* factory)
     : BrowserAccessibilityManager(initial_tree, delegate, factory),
       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);
     parent_hwnd_ = accessible_hwnd_->GetParent();
   }
 }
 
 BrowserAccessibilityManagerWin::~BrowserAccessibilityManagerWin() {
   if (tracked_scroll_object_) {
     tracked_scroll_object_->Release();
@@ skipping 22 matching lines @@
     LegacyRenderWidgetHostHWND* accessible_hwnd) {
   accessible_hwnd_ = accessible_hwnd;
   if (accessible_hwnd_) {
     accessible_hwnd_->set_browser_accessibility_manager(this);
     parent_hwnd_ = accessible_hwnd_->GetParent();
   }
 }
 
 void BrowserAccessibilityManagerWin::MaybeCallNotifyWinEvent(DWORD event,
                                                              LONG child_id) {
+  // 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 (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;
 
-  // If on Win 7 and complete accessibility is enabled, use the fake child HWND
-  // to use as the root of the accessibility tree. See comments above
-  // LegacyRenderWidgetHostHWND for details.
-  if (BrowserAccessibilityStateImpl::GetInstance()->IsAccessibleBrowser()) {
-    DCHECK(accessible_hwnd_);
-    parent_hwnd_ = accessible_hwnd_->hwnd();
-    parent_iaccessible_ = accessible_hwnd_->window_accessible();
-  }
   ::NotifyWinEvent(event, parent_hwnd(), OBJID_CLIENT, child_id);
 }
 
 
 void BrowserAccessibilityManagerWin::OnNodeCreated(ui::AXNode* node) {
   BrowserAccessibilityManager::OnNodeCreated(node);
   BrowserAccessibility* obj = GetFromAXNode(node);
   LONG unique_id_win = obj->ToBrowserAccessibilityWin()->unique_id_win();
   unique_id_to_ax_id_map_[unique_id_win] = obj->GetId();
 }
 
 void BrowserAccessibilityManagerWin::OnNodeWillBeDeleted(ui::AXNode* node) {
   BrowserAccessibilityManager::OnNodeWillBeDeleted(node);
   BrowserAccessibility* obj = GetFromAXNode(node);
   if (!obj)
     return;
   unique_id_to_ax_id_map_.erase(
       obj->ToBrowserAccessibilityWin()->unique_id_win());
   if (obj == tracked_scroll_object_) {
     tracked_scroll_object_->Release();
     tracked_scroll_object_ = NULL;
   }
 }
 
 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(
     ui::AXEvent event_type,
     BrowserAccessibility* node) {
   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:
       event_id = IA2_EVENT_ACTIVE_DESCENDANT_CHANGED;
       break;
     case ui::AX_EVENT_ALERT:
       event_id = EVENT_SYSTEM_ALERT;
       break;
     case ui::AX_EVENT_ARIA_ATTRIBUTE_CHANGED:
       event_id = IA2_EVENT_OBJECT_ATTRIBUTE_CHANGED;
@@ skipping 67 matching lines @@
       // accessibility notification.
       break;
   }
 
   if (event_id != EVENT_MIN) {
     // Pass the node's unique id in the |child_id| argument to NotifyWinEvent;
     // the AT client will then call get_accChild on the HWND's accessibility
     // 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);
   }
 
   // If this is a layout complete notification (sent when a container scrolls)
   // and there is a descendant tracked object, send a notification on it.
   // TODO(dmazzoni): remove once http://crbug.com/113483 is fixed.
   if (event_type == ui::AX_EVENT_LAYOUT_COMPLETE &&
       tracked_scroll_object_ &&
       tracked_scroll_object_->IsDescendantOf(node)) {
     MaybeCallNotifyWinEvent(
         IA2_EVENT_VISIBLE_DATA_CHANGED,
         tracked_scroll_object_->ToBrowserAccessibilityWin()->unique_id_win());
     tracked_scroll_object_->Release();
     tracked_scroll_object_ = NULL;
   }
 }
 
 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(
     BrowserAccessibilityWin* node) {
   if (tracked_scroll_object_)
     tracked_scroll_object_->Release();
   tracked_scroll_object_ = node;
   tracked_scroll_object_->AddRef();
 }
 
@@ skipping 14 matching lines @@
   // |parent_iaccessible_| are no longer valid either, since they were
   // derived from AccessibleHWND. We don't have to restore them to
   // previous values, though, because this should only happen
   // during the destruct sequence for this window.
   accessible_hwnd_ = NULL;
   parent_hwnd_ = NULL;
   parent_iaccessible_ = NULL;
 }
 
 }  // namespace content