Text controls should expose their value instead of their name via IAccessibleText

content/browser/accessibility/browser_accessibility_win.cc

Index: content/browser/accessibility/browser_accessibility_win.cc
diff --git a/content/browser/accessibility/browser_accessibility_win.cc b/content/browser/accessibility/browser_accessibility_win.cc
index ed687849c384faf53eabbd953683ad2eca03954b..13070924367fed2a199d5414b277667e0369f513 100644
--- a/content/browser/accessibility/browser_accessibility_win.cc
+++ b/content/browser/accessibility/browser_accessibility_win.cc
@@ -2002,7 +2002,7 @@ STDMETHODIMP BrowserAccessibilityWin::get_nCharacters(LONG* n_characters) {
   if (!n_characters)
     return E_INVALIDARG;
 
-  *n_characters = TextForIAccessibleText().length();
+  *n_characters = hypertext().length();
   return S_OK;
 }
 
@@ -2039,7 +2039,7 @@ STDMETHODIMP BrowserAccessibilityWin::get_characterExtents(
   if (!out_x || !out_y || !out_width || !out_height)
     return E_INVALIDARG;
 
-  const base::string16& text_str = TextForIAccessibleText();
+  const base::string16& text_str = hypertext();
   HandleSpecialTextOffset(text_str, &offset);
 
   if (offset < 0 || offset > static_cast<LONG>(text_str.size()))
@@ -2122,7 +2122,7 @@ STDMETHODIMP BrowserAccessibilityWin::get_text(LONG start_offset,
   if (!text)
     return E_INVALIDARG;
 
-  const base::string16& text_str = TextForIAccessibleText();
+  const base::string16& text_str = hypertext();
 
   // Handle special text offsets.
   HandleSpecialTextOffset(text_str, &start_offset);
@@ -2165,7 +2165,7 @@ STDMETHODIMP BrowserAccessibilityWin::get_textAtOffset(
   if (!start_offset || !end_offset || !text)
     return E_INVALIDARG;
 
-  const base::string16& text_str = TextForIAccessibleText();
+  const base::string16& text_str = hypertext();
   HandleSpecialTextOffset(text_str, &offset);
   if (offset < 0)
     return E_INVALIDARG;
@@ -2220,7 +2220,7 @@ STDMETHODIMP BrowserAccessibilityWin::get_textBeforeOffset(
     return S_FALSE;
   }
 
-  const base::string16& text_str = TextForIAccessibleText();
+  const base::string16& text_str = hypertext();
 
   *start_offset = FindBoundary(
       text_str, boundary_type, offset, ui::BACKWARDS_DIRECTION);
@@ -2249,7 +2249,7 @@ STDMETHODIMP BrowserAccessibilityWin::get_textAfterOffset(
     return S_FALSE;
   }
 
-  const base::string16& text_str = TextForIAccessibleText();
+  const base::string16& text_str = hypertext();
 
   *start_offset = offset;
   *end_offset = FindBoundary(
@@ -2342,7 +2342,7 @@ STDMETHODIMP BrowserAccessibilityWin::addSelection(LONG start_offset,
   if (!instance_active())
     return E_FAIL;
 
-  const base::string16& text_str = TextForIAccessibleText();
+  const base::string16& text_str = hypertext();
   HandleSpecialTextOffset(text_str, &start_offset);
   HandleSpecialTextOffset(text_str, &end_offset);
 
@@ -2365,7 +2365,7 @@ STDMETHODIMP BrowserAccessibilityWin::setCaretOffset(LONG offset) {
   if (!instance_active())
     return E_FAIL;
 
-  const base::string16& text_str = TextForIAccessibleText();
+  const base::string16& text_str = hypertext();
   HandleSpecialTextOffset(text_str, &offset);
   manager()->SetTextSelection(*this, offset, offset);
   return S_OK;
@@ -2380,7 +2380,7 @@ STDMETHODIMP BrowserAccessibilityWin::setSelection(LONG selection_index,
   if (selection_index != 0)
     return E_INVALIDARG;
 
-  const base::string16& text_str = TextForIAccessibleText();
+  const base::string16& text_str = hypertext();
   HandleSpecialTextOffset(text_str, &start_offset);
   HandleSpecialTextOffset(text_str, &end_offset);
 
@@ -2475,14 +2475,14 @@ STDMETHODIMP BrowserAccessibilityWin::get_anchor(long index, VARIANT* anchor) {
   if (index != 0 || !anchor)
     return E_INVALIDARG;
 
-  BSTR hypertext = SysAllocString(TextForIAccessibleText().c_str());
-  DCHECK(hypertext);
+  BSTR ia2_hypertext = SysAllocString(hypertext().c_str());
+  DCHECK(ia2_hypertext);
   anchor->vt = VT_BSTR;
-  anchor->bstrVal = hypertext;
+  anchor->bstrVal = ia2_hypertext;
 
   // Returning S_FALSE is not mentioned in the IA2 Spec, but it might have been
   // an oversight.
-  if (!SysStringLen(hypertext))
+  if (!SysStringLen(ia2_hypertext))
     return S_FALSE;
 
   return S_OK;
@@ -3057,7 +3057,7 @@ STDMETHODIMP BrowserAccessibilityWin::get_unclippedSubstringBounds(
   if (!out_x || !out_y || !out_width || !out_height)
     return E_INVALIDARG;
 
-  const base::string16& text_str = TextForIAccessibleText();
+  const base::string16& text_str = hypertext();
   if (start_index > text_str.size() ||
       end_index > text_str.size() ||
       start_index > end_index) {
@@ -3079,7 +3079,7 @@ STDMETHODIMP BrowserAccessibilityWin::scrollToSubstring(
   if (!instance_active())
     return E_FAIL;
 
-  const base::string16& text_str = TextForIAccessibleText();
+  const base::string16& text_str = hypertext();
   if (start_index > text_str.size() ||
       end_index > text_str.size() ||
       start_index > end_index) {
@@ -3528,7 +3528,11 @@ void BrowserAccessibilityWin::UpdateStep1ComputeWinAttributes() {
 
 void BrowserAccessibilityWin::UpdateStep2ComputeHypertext() {
   if (!PlatformChildCount()) {
-    win_attributes_->hypertext += name();
+    if (IsControlWithValue())

[dmazzoni, 2015/10/29 20:15:29]

I like the idea behind this change, but I don't think IsControlWithValue is the
correct semantics.

Only a *text* control should expose its value via IAccessibleText.

So a combo box or search box role, absolutely - that's a valid fix. Maybe a
couple of others.

A slider or listbox? No, I don't buy it. You can't arrow through the characters
in the values of those roles, so it doesn't make sense to expose anything via
IAccessibleText for those controls. We already have accValue for those.

+      win_attributes_->hypertext += value();
+    else
+      win_attributes_->hypertext += name();
+
     return;
   }
 
@@ -3541,6 +3545,7 @@ void BrowserAccessibilityWin::UpdateStep2ComputeHypertext() {
     BrowserAccessibilityWin* child =
         PlatformGetChild(i)->ToBrowserAccessibilityWin();
     DCHECK(child);
+    // Similar to Firefox, we don't expose text-only objects in IA2 hypertext.
     if (child->IsTextOnlyObject()) {
       win_attributes_->hypertext += child->name();
     } else {
@@ -3815,7 +3820,7 @@ int32 BrowserAccessibilityWin::GetHypertextOffsetFromChild(
           PlatformGetChild(i)->ToBrowserAccessibilityWin();
       DCHECK(sibling);
       if (sibling->IsTextOnlyObject())
-        hypertextOffset += sibling->TextForIAccessibleText().length();
+        hypertextOffset += sibling->hypertext().length();
       else
         ++hypertextOffset;
     }
@@ -3907,7 +3912,7 @@ int BrowserAccessibilityWin::GetHypertextOffsetFromEndpoint(
   if (endpoint_index_in_common_parent < index_in_common_parent)
     return 0;
   if (endpoint_index_in_common_parent > index_in_common_parent)
-    return TextForIAccessibleText().length();
+    return hypertext().length();
 
   NOTREACHED();
   return -1;
@@ -4021,16 +4026,6 @@ base::string16 BrowserAccessibilityWin::GetValueText() {
   return value;
 }
 
-base::string16 BrowserAccessibilityWin::TextForIAccessibleText() const {
-  switch (GetRole()) {
-    case ui::AX_ROLE_TEXT_FIELD:
-    case ui::AX_ROLE_MENU_LIST_OPTION:
-      return value();
-    default:
-      return hypertext();
-  }
-}
-
 bool BrowserAccessibilityWin::IsSameHypertextCharacter(size_t old_char_index,
                                                        size_t new_char_index) {
   CHECK(old_win_attributes_);