Too many accessible nodes were marked as clickable on Android

content/browser/accessibility/browser_accessibility_android.cc

 // Copyright 2013 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_android.h"
 
 #include "base/i18n/break_iterator.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ skipping 136 matching lines @@
   if (HasState(ui::AX_STATE_CHECKED))
     checkable = true;
   return checkable;
 }
 
 bool BrowserAccessibilityAndroid::IsChecked() const {
   return (HasState(ui::AX_STATE_CHECKED) || HasState(ui::AX_STATE_PRESSED));
 }
 
 bool BrowserAccessibilityAndroid::IsClickable() const {
-  if (!PlatformIsLeaf())
-    return false;
-
-  // We are very aggressive about returning true with IsClickable on Android
-  // because there is no way to know for sure what might have a click handler.

[David Tseng, 2016/10/12 20:28:35]

Is this no longer a concern or do you have a more robust way to detect the
associated dom node has a click handler (e.g. onclick set)?

[dmazzoni, 2016/10/12 22:41:58]

An older version of TalkBack was skipping over anything
that wasn't clickable, so we returned "clickable" for more
than what was actually clickable.

Now TalkBack is fixed, so we can revert to returning
clickable when we think a node is clickable. Same as other
platforms, it's not bad but not 100% perfect. It works
correctly for onclick and for real event handlers, but
doesn't handle event delegation where the listener is on
the ancestor.

-  return IsFocusable() || !GetText().empty();
+  // A node is clickable if it's focusable or if it has a default action.
+  return IsFocusable() || HasStringAttribute(ui::AX_ATTR_ACTION);
 }
 
 bool BrowserAccessibilityAndroid::IsCollapsed() const {
   return HasState(ui::AX_STATE_COLLAPSED);
 }
 
 bool BrowserAccessibilityAndroid::IsCollection() const {
   return (GetRole() == ui::AX_ROLE_GRID ||
           GetRole() == ui::AX_ROLE_LIST ||
           GetRole() == ui::AX_ROLE_LIST_BOX ||
@@ skipping 27 matching lines @@
 
 bool BrowserAccessibilityAndroid::IsEnabled() const {
   return !HasState(ui::AX_STATE_DISABLED);
 }
 
 bool BrowserAccessibilityAndroid::IsExpanded() const {
   return HasState(ui::AX_STATE_EXPANDED);
 }
 
 bool BrowserAccessibilityAndroid::IsFocusable() const {
-  bool focusable = HasState(ui::AX_STATE_FOCUSABLE);
-  if (IsIframe() ||
-      GetRole() == ui::AX_ROLE_WEB_AREA) {
-    focusable = false;
-  }
-  return focusable;
+  // Don't count iframes as focusable.
+  if (IsIframe())
+    return false;
+
+  // Only the root of the tree, and not child frames, are focusable.

[David Tseng, 2016/10/12 20:28:35]

Seems strange to have this restriction; what if a page author sets an aria role
on body like application? This code would allow focus in that case. Also, if an
iframe has an aria label, wouldn't it be interesting?

[dmazzoni, 2016/10/12 22:41:58]

Agreed. I changed it so that we skip iframes and child frame
root notes if they don't have a name, but we include them if
they have a name.

I don't think there's an issue with an aria role on the body,
the root of our tree is actually the parent of the body element.

+  if (GetRole() == ui::AX_ROLE_ROOT_WEB_AREA)
+    return !GetParent();
+
+  return HasState(ui::AX_STATE_FOCUSABLE);
 }
 
 bool BrowserAccessibilityAndroid::IsFocused() const {
   return manager()->GetFocus() == this;
 }
 
 bool BrowserAccessibilityAndroid::IsHeading() const {
   BrowserAccessibilityAndroid* parent =
       static_cast<BrowserAccessibilityAndroid*>(GetParent());
   if (parent && parent->IsHeading())
@@ skipping 1194 matching lines @@
 int BrowserAccessibilityAndroid::CountChildrenWithRole(ui::AXRole role) const {
   int count = 0;
   for (uint32_t i = 0; i < PlatformChildCount(); i++) {
     if (PlatformGetChild(i)->GetRole() == role)
       count++;
   }
   return count;
 }
 
 }  // namespace content