Improve support for multiselect list box accessibility on Windows.

content/browser/accessibility/browser_accessibility_win.cc

 // Copyright (c) 2011 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_win.h"
 
 #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"
 #include "net/base/escape.h"
 #include "ui/base/accessibility/accessible_text_utils.h"
 
 using webkit_glue::WebAccessibility;
 
 // The GUID for the ISimpleDOM service is not defined in the IDL files.
 // This is taken directly from the Mozilla sources
@@ skipping 141 matching lines @@
 
 BrowserAccessibilityWin* BrowserAccessibility::toBrowserAccessibilityWin() {
   return static_cast<BrowserAccessibilityWin*>(this);
 }
 
 BrowserAccessibilityWin::BrowserAccessibilityWin()
     : ia_role_(0),
       ia_state_(0),
       ia2_role_(0),
       ia2_state_(0),
-      first_time_(true) {
+      first_time_(true),
+      old_ia_state_(0) {
 }
 
 BrowserAccessibilityWin::~BrowserAccessibilityWin() {
   for (size_t i = 0; i < relations_.size(); ++i)
     relations_[i]->Release();
 }
 
 //
 // IAccessible methods.
 //
@@ skipping 340 matching lines @@
 
 STDMETHODIMP BrowserAccessibilityWin::get_accHelpTopic(
     BSTR* help_file, VARIANT var_id, LONG* topic_id) {
   return E_NOTIMPL;
 }
 
 STDMETHODIMP BrowserAccessibilityWin::get_accSelection(VARIANT* selected) {
   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;
 }
 
 STDMETHODIMP BrowserAccessibilityWin::accSelect(
     LONG flags_sel, VARIANT var_id) {
   if (!instance_active_)
     return E_FAIL;
 
   if (flags_sel & SELFLAG_TAKEFOCUS) {
     manager_->SetFocus(this, true);
@@ skipping 134 matching lines @@
     return S_FALSE;
 
   for (long i = 0; i < count; ++i) {
     relations_[i]->AddRef();
     relations[i] = relations_[i];
   }
 
   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.
 //
 
 STDMETHODIMP BrowserAccessibilityWin::get_description(BSTR* desc) {
   if (!instance_active_)
     return E_FAIL;
 
   if (!desc)
     return E_INVALIDARG;
@@ skipping 1632 matching lines @@
   }
 
   // Expose the "display" and "tag" attributes.
   StringAttributeToIA2(WebAccessibility::ATTR_DISPLAY, "display");
   StringAttributeToIA2(WebAccessibility::ATTR_HTML_TAG, "tag");
   StringAttributeToIA2(WebAccessibility::ATTR_ROLE, "xml-roles");
 
   // 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");
   BoolAttributeToIA2(WebAccessibility::ATTR_LIVE_ATOMIC, "atomic");
   BoolAttributeToIA2(WebAccessibility::ATTR_LIVE_BUSY, "busy");
 
   // Expose container live region attributes.
   StringAttributeToIA2(WebAccessibility::ATTR_CONTAINER_LIVE_STATUS,
                        "container-live");
   StringAttributeToIA2(WebAccessibility::ATTR_CONTAINER_LIVE_RELEVANT,
@@ skipping 32 matching lines @@
       }
       if (index >= 0) {
         ia2_attributes_.push_back(string16(L"table-cell-index:") +
                                   base::IntToString16(index));
       }
     } else {
       NOTREACHED();
     }
   }
 
-  // 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
   // announce the name.
   if (name_.empty())
     GetStringAttribute(WebAccessibility::ATTR_DESCRIPTION, &name_);
 
   // If this doesn't have a value and is linked then set its value to the url
   // attribute. This allows screen readers to read an empty link's destination.
   string16 url;
@@ skipping 33 matching lines @@
       manager_->NotifyAccessibilityEvent(
           ViewHostMsg_AccEvent::OBJECT_SHOW, this);
     }
 
     // TODO(dmazzoni): Look into HIDE events, too.
 
     old_text_ = previous_text_;
     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;
 }
 
 void BrowserAccessibilityWin::NativeAddReference() {
   AddRef();
 }
 
 void BrowserAccessibilityWin::NativeReleaseReference() {
   Release();
 }
@@ skipping 128 matching lines @@
   if ((state_ >> WebAccessibility::STATE_HASPOPUP) & 1)
     ia_state_|= STATE_SYSTEM_HASPOPUP;
   if ((state_ >> WebAccessibility::STATE_HOTTRACKED) & 1)
     ia_state_|= STATE_SYSTEM_HOTTRACKED;
   if ((state_ >> WebAccessibility::STATE_INDETERMINATE) & 1)
     ia_state_|= STATE_SYSTEM_INDETERMINATE;
   if ((state_ >> WebAccessibility::STATE_INVISIBLE) & 1)
     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;
   if ((state_ >> WebAccessibility::STATE_PRESSED) & 1)
     ia_state_|= STATE_SYSTEM_PRESSED;
   if ((state_ >> WebAccessibility::STATE_PROTECTED) & 1)
     ia_state_|= STATE_SYSTEM_PROTECTED;
   if ((state_ >> WebAccessibility::STATE_REQUIRED) & 1)
     ia2_state_|= IA2_STATE_REQUIRED;
   if ((state_ >> WebAccessibility::STATE_SELECTABLE) & 1)
@@ skipping 175 matching lines @@
       break;
     case WebAccessibility::ROLE_LIST:
       ia_role_ = ROLE_SYSTEM_LIST;
       ia_state_|= STATE_SYSTEM_READONLY;
       break;
     case WebAccessibility::ROLE_LISTBOX:
       ia_role_ = ROLE_SYSTEM_LIST;
       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;
       ia_state_|= STATE_SYSTEM_READONLY;
       break;
     case WebAccessibility::ROLE_LIST_MARKER:
       ia_role_ = ROLE_SYSTEM_TEXT;
       ia_state_|= STATE_SYSTEM_READONLY;
       break;
     case WebAccessibility::ROLE_MATH:
@@ skipping 165 matching lines @@
   }
 
   // The role should always be set.
   DCHECK(!role_name_.empty() || ia_role_);
 
   // If we didn't explicitly set the IAccessible2 role, make it the same
   // as the MSAA role.
   if (!ia2_role_)
     ia2_role_ = ia_role_;
 }