Improve support for multiselect list box accessibility on Windows.

content/browser/accessibility/browser_accessibility_win.cc

Index: content/browser/accessibility/browser_accessibility_win.cc
===================================================================
--- content/browser/accessibility/browser_accessibility_win.cc	(revision 110985)
+++ content/browser/accessibility/browser_accessibility_win.cc	(working copy)
@@ -7,6 +7,7 @@
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
+#include "base/win/enum_variant.h"
 #include "base/win/scoped_comptr.h"
 #include "content/browser/accessibility/browser_accessibility_manager_win.h"
 #include "content/common/view_messages.h"
@@ -168,7 +169,8 @@
       ia_state_(0),
       ia2_role_(0),
       ia2_state_(0),
-      first_time_(true) {
+      first_time_(true),
+      old_ia_state_(0) {
 }
 
 BrowserAccessibilityWin::~BrowserAccessibilityWin() {
@@ -529,6 +531,45 @@
   if (!instance_active_)
     return E_FAIL;
 
+  if (role_ == WebAccessibility::ROLE_LISTBOX && children_.size() > 0) {
+    unsigned long selected_count = 0;
+    for (size_t i = 0; i < children_.size(); i++) {
+      if ((children_[i]->state() >> WebAccessibility::STATE_SELECTED) & 1)

[darin (slow to review), 2011/11/22 01:16:52]

nit: A HasState method might be nice here.

  if (children_[i]->HasState(WebAccessibility::STATE_SELECTED))
    ...

^^^ more readable

[dmazzoni, 2011/11/22 08:08:14]

Good idea, done.

+        selected_count++;
+    }
+
+    if (selected_count == 0) {
+      selected->vt = VT_EMPTY;
+      return S_OK;
+    }
+
+    if (selected_count == 1) {
+      for (size_t i = 0; i < children_.size(); i++) {
+        if ((children_[i]->state() >> WebAccessibility::STATE_SELECTED) & 1) {
+          selected->vt = VT_DISPATCH;
+          selected->pdispVal =
+              children_[i]->toBrowserAccessibilityWin()->NewReference();
+          return S_OK;
+        }
+      }
+    }
+
+    // Multiple items are selected.
+    base::win::EnumVariant* enum_variant =
+        new base::win::EnumVariant(selected_count);
+    unsigned long index = 0;
+    for (size_t i = 0; i < children_.size(); i++) {
+      if ((children_[i]->state() >> WebAccessibility::STATE_SELECTED) & 1) {
+        enum_variant->get(index)->vt = VT_DISPATCH;
+        enum_variant->get(index)->pdispVal =
+            children_[i]->toBrowserAccessibilityWin()->NewReference();

[darin (slow to review), 2011/11/22 01:16:52]

NewReference appears to take a reference count.  It calls AddRef().  How does
that reference ever get released?

[dmazzoni, 2011/11/22 08:08:14]

It's the responsibility of the client who calls this method. 

See the memory management rules here:
http://msdn.microsoft.com/en-us/library/ms810016.aspx - normally the rules are
pretty simple, in this class we're implementing a COM server so whenever we
return a pointer, we always increment the reference count.

IEnumVARIANT seems confusing at first but you should just think of it as an
array. If the items in the array are pointers, the same semantics apply - we
increment the reference count, and it's the client's responsibility to release
it.

+        index++;
+      }
+    }
+    selected->vt = VT_UNKNOWN;
+    selected->punkVal = enum_variant;
+  }
+
   return E_NOTIMPL;
 }
 
@@ -683,6 +724,22 @@
   return S_OK;
 }
 
+STDMETHODIMP BrowserAccessibilityWin::get_groupPosition(
+    LONG* group_level,
+    LONG* similar_items_in_group,
+    LONG* position_in_group) {
+  if (role_ == WebAccessibility::ROLE_LISTBOX_OPTION &&
+      parent_ &&
+      parent_->role() == WebAccessibility::ROLE_LISTBOX) {
+    *group_level = 0;
+    *similar_items_in_group = parent_->child_count();
+    *position_in_group = index_in_parent_ + 1;
+    return S_OK;
+  }
+
+  return E_NOTIMPL;
+}
+
 //
 // IAccessibleImage methods.
 //
@@ -2335,6 +2392,16 @@
   // Expose "level" attribute for tree nodes.
   IntAttributeToIA2(WebAccessibility::ATTR_HIERARCHICAL_LEVEL, "level");
 
+  // Expose the set size and position in set for listbox options.
+  if (role_ == WebAccessibility::ROLE_LISTBOX_OPTION &&
+      parent_ &&
+      parent_->role() == WebAccessibility::ROLE_LISTBOX) {
+    ia2_attributes_.push_back(
+        string16(L"setsize:") + base::IntToString16(parent_->child_count()));

[darin (slow to review), 2011/11/22 01:16:52]

nit: is it necessary to wrap those constants with string16() constructors?  we
should have an operator+ that is happy provided either the LHS or RHS is a
string16.

[dmazzoni, 2011/11/22 08:08:14]

You're right, thanks.

+    ia2_attributes_.push_back(
+        string16(L"setsize:") + base::IntToString16(index_in_parent_ + 1));
+  }
+
   // Expose live region attributes.
   StringAttributeToIA2(WebAccessibility::ATTR_LIVE_STATUS, "live");
   StringAttributeToIA2(WebAccessibility::ATTR_LIVE_RELEVANT, "relevant");
@@ -2387,9 +2454,11 @@
     }
   }
 
-  // If this is static text, put the text in the name rather than the value.
-  if (role_ == WebAccessibility::ROLE_STATIC_TEXT && name_.empty())
+  if (name_.empty() &&
+      (role_ == WebAccessibility::ROLE_LISTBOX_OPTION ||
+       role_ == WebAccessibility::ROLE_STATIC_TEXT)) {
     name_.swap(value_);
+  }
 
   // If this object doesn't have a name but it does have a description,
   // use the description as its name - because some screen readers only
@@ -2443,6 +2512,40 @@
     previous_text_ = text;
   }
 
+  // Fire events if the state has changed.
+  if (!first_time_ && ia_state_ != old_ia_state_) {
+    // Normally focus events are handled elsewhere, however
+    // focus for managed descendants is platform-specific.
+    // Fire a focus event if the focused descendant in a multi-select
+    // list box changes.
+    if (role_ == WebAccessibility::ROLE_LISTBOX_OPTION &&
+        (ia_state_ & STATE_SYSTEM_FOCUSABLE) &&
+        (ia_state_ & STATE_SYSTEM_SELECTABLE) &&
+        (ia_state_ & STATE_SYSTEM_FOCUSED) &&
+        !(old_ia_state_ & STATE_SYSTEM_FOCUSED)) {
+      ::NotifyWinEvent(EVENT_OBJECT_FOCUS,
+                       manager_->GetParentView(),
+                       OBJID_CLIENT,
+                       child_id());
+    }
+
+    if ((ia_state_ & STATE_SYSTEM_SELECTED) &&
+        !(old_ia_state_ & STATE_SYSTEM_SELECTED)) {
+      ::NotifyWinEvent(EVENT_OBJECT_SELECTIONADD,
+                       manager_->GetParentView(),
+                       OBJID_CLIENT,
+                       child_id());
+    } else if (!(ia_state_ & STATE_SYSTEM_SELECTED) &&
+               (old_ia_state_ & STATE_SYSTEM_SELECTED)) {
+      ::NotifyWinEvent(EVENT_OBJECT_SELECTIONREMOVE,
+                       manager_->GetParentView(),
+                       OBJID_CLIENT,
+                       child_id());
+    }
+
+    old_ia_state_ = ia_state_;

[darin (slow to review), 2011/11/22 01:16:52]

do we have to worry about |this| being deleted upon return from NotifyWinEvent? 
since you are calling into crazy hooker code, i worry about that foreign code
doing things like trying to close the browser that might cause the
BrowserAccessibilityWin object to get deleted before returning from
NotifyWinEvent.  is that a valid concern?

[dmazzoni, 2011/11/22 08:08:14]

I don't think we have to worry about that. It'd do that by sending a WM_CLOSE or
WM_DESTROY to the window, which would get caught in the main message loop, not
inside this call stack.

The main thing we do have to worry about here is that some client that's hooked
NotifyWinEvent might query this object and maybe even explore the a11y tree
before NotifyWinEvent returns. That's why these notifications are sent in a
separate SendNodeUpdateEvents method, which happens after the tree is connected
up, rather than during initialization.

+  }
+
   first_time_ = false;
 }
 
@@ -2591,8 +2694,10 @@
     ia_state_|= STATE_SYSTEM_INVISIBLE;
   if ((state_ >> WebAccessibility::STATE_LINKED) & 1)
     ia_state_|= STATE_SYSTEM_LINKED;
-  if ((state_ >> WebAccessibility::STATE_MULTISELECTABLE) & 1)
+  if ((state_ >> WebAccessibility::STATE_MULTISELECTABLE) & 1) {
+    ia_state_|= STATE_SYSTEM_EXTSELECTABLE;
     ia_state_|= STATE_SYSTEM_MULTISELECTABLE;
+  }
   // TODO(ctguil): Support STATE_SYSTEM_EXTSELECTABLE/accSelect.
   if ((state_ >> WebAccessibility::STATE_OFFSCREEN) & 1)
     ia_state_|= STATE_SYSTEM_OFFSCREEN;
@@ -2788,6 +2893,11 @@
       break;
     case WebAccessibility::ROLE_LISTBOX_OPTION:
       ia_role_ = ROLE_SYSTEM_LISTITEM;
+      if (ia_state_ & STATE_SYSTEM_SELECTABLE) {
+        ia_state_ |= STATE_SYSTEM_FOCUSABLE;
+        if ((state_ >> WebAccessibility::STATE_FOCUSED) & 1)

[darin (slow to review), 2011/11/22 01:16:52]

again, it feels like a HasState type function might help with readability.

[dmazzoni, 2011/11/22 08:08:14]

Done.

+          ia_state_|= STATE_SYSTEM_FOCUSED;
+      }
       break;
     case WebAccessibility::ROLE_LIST_ITEM:
       ia_role_ = ROLE_SYSTEM_LISTITEM;