Plumb through selected as a boolean attribute

It is sometimes necessary to distinguish among the three states of the selected attribute: - true: a node is selected (e.g. an option) - false: a node is unselected - undefined: the node is not meant to be selected The |selectable| state is implied by the first two values for selected. With this change, a client can trust that a control is *not* selected rather than not supporting selection. On the author side, (e.g. a views author), one does not need to remember to set selectable state along with the actual selected state. In ChromeVox, we sometimes want to speak the items which are selected (in the case of cells, for Sheets). However, in the common case, which is when selection follows focus, it is extremely verbose to speak selected. Instead, only speak items which are unselected. Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation Change-Id: Iaf76784664a72ff7c99fff4ebf35f9b798b34cec Reviewed-on: https://chromium-review.googlesource.com/985473 Commit-Queue: David Tseng <dtseng@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Emily Stark <estark@chromium.org> Cr-Commit-Position: refs/heads/master@{#548338}
marcosholgado · Apr 5, 2018 · 4c91187 · 4c91187
1 parent afd440f
commit 4c91187
Show file tree

Hide file tree

Showing 67 changed files with 513 additions and 443 deletions.
diff --git a/chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc b/chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc
@@ -207,7 +207,6 @@ void PopulateAXState(AXNodeInfoData* node, ui::AXNodeData* out_data) {
   MAP_STATE(AXBooleanProperty::FOCUSABLE, ax::mojom::State::kFocusable);
   MAP_STATE(AXBooleanProperty::MULTI_LINE, ax::mojom::State::kMultiline);
   MAP_STATE(AXBooleanProperty::PASSWORD, ax::mojom::State::kProtected);
-  MAP_STATE(AXBooleanProperty::SELECTED, ax::mojom::State::kSelected);
 
 #undef MAP_STATE
 
@@ -542,6 +541,9 @@ void AXTreeSourceArc::SerializeNode(AXNodeInfoData* node,
   if (GetProperty(node, AXBooleanProperty::CLICKABLE)) {
     out_data->AddBoolAttribute(ax::mojom::BoolAttribute::kClickable, true);
   }
+  if (GetProperty(node, AXBooleanProperty::SELECTED)) {
+    out_data->AddBoolAttribute(ax::mojom::BoolAttribute::kSelected, true);
+  }
 
   // Range info.
   AXRangeInfoData* range_info = node->range_info.get();

diff --git a/chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs b/chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
@@ -1533,9 +1533,14 @@ TEST_F('BackgroundTest', 'NavigationSyncsSelect', function() {
   */}, function(root) {
     var select = root.find({role: RoleType.POP_UP_BUTTON});
     mockFeedback.call(select.doDefault.bind(select))
-        .expectSpeech('apple', 'Selected')
+        .expectSpeech('apple', 'Menu item', ' 1 of 2 ')
         .call(doCmd('nextObject'))
-        .expectSpeech('grape', 'Selected')
+        .expectNextSpeechUtteranceIsNot('Selected')
+        .expectNextSpeechUtteranceIsNot('Unselected')
+        .expectSpeech('grape', 'Menu item')
+        .expectNextSpeechUtteranceIsNot('Selected')
+        .expectNextSpeechUtteranceIsNot('Unselected')
+        .expectSpeech(' 2 of 2 ')
         .replay();
   });
 });
diff --git a/chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js b/chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js
@@ -179,7 +179,7 @@ DesktopAutomationHandler.prototype = {
    * @param {!AutomationEvent} evt
    */
   onEventIfSelected: function(evt) {
-    if (evt.target.state.selected)
+    if (evt.target.selected)
       this.onEventDefault(evt);
   },
 

diff --git a/chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js b/chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
@@ -226,7 +226,6 @@ Output.STATE_INFO_ = {
   expanded: {on: {msgId: 'aria_expanded_true'}},
   multiselectable: {on: {msgId: 'aria_multiselectable_true'}},
   required: {on: {msgId: 'aria_required_true'}},
-  selected: {on: {msgId: 'aria_selected_true'}},
   visited: {on: {msgId: 'visited_state'}}
 };
 
@@ -325,7 +324,8 @@ Output.RULES = {
       speak: `$name $cellIndexText $node(tableColumnHeader)
           $state $description`,
       braille: `$state
-          $name $cellIndexText $node(tableColumnHeader) $description`
+          $name $cellIndexText $node(tableColumnHeader) $description
+          $if($selected, @aria_selected_true)`
     },
     checkBox: {
       speak: `$if($checked, $earcon(CHECK_ON), $earcon(CHECK_OFF))
@@ -390,7 +390,11 @@ Output.RULES = {
     },
     listBoxOption: {
       speak: `$state $name $role @describe_index($posInSet, $setSize)
-          $description $restriction`
+          $description $restriction
+          $nif($selected, @aria_selected_false)`,
+      braille: `$state $name $role @describe_index($posInSet, $setSize)
+          $description $restriction
+          $if($selected, @aria_selected_true, @aria_selected_false)`
     },
     listMarker: {speak: `$name`},
     menu: {
@@ -418,6 +422,10 @@ Output.RULES = {
     },
     menuListOption: {
       speak: `$name $role @describe_index($posInSet, $setSize) $state
+          $nif($selected, @aria_selected_false)
+          $restriction $description`,
+      braille: `$name $role @describe_index($posInSet, $setSize) $state
+          $if($selected, @aria_selected_true, @aria_selected_false)
           $restriction $description`
     },
     paragraph: {speak: `$nameOrDescendants`},
@@ -434,10 +442,14 @@ Output.RULES = {
     },
     rootWebArea: {enter: `$name`, speak: `$if($name, $name, $docUrl)`},
     region: {speak: `$state $nameOrTextContent $description $roleDescription`},
-    row: {enter: `$node(tableRowHeader)`},
+    row: {
+      enter: `$node(tableRowHeader)`,
+      speak: `$name $node(activeDescendant) $value $state $restriction $role
+          $if($selected, @aria_selected_true) $description`
+    },
     rowHeader: {
       speak: `$nameOrTextContent $description $roleDescription
-        $state`
+        $state $if($selected, @aria_selected_true)`
     },
     staticText: {speak: `$name=`},
     switch: {
@@ -448,7 +460,8 @@ Output.RULES = {
     },
     tab: {
       speak: `@describe_tab($name) $roleDescription $description
-          @describe_index($posInSet, $setSize) $state $restriction `,
+          @describe_index($posInSet, $setSize) $state $restriction
+          $if($selected, @aria_selected_true)`,
     },
     table: {
       enter: `@table_summary($name,
@@ -490,6 +503,7 @@ Output.RULES = {
           @describe_depth($hierarchicalLevel)`,
       speak: `$name
           $role $description $state $restriction
+          $nif($selected, @aria_selected_false)
           @describe_index($posInSet, $setSize)
           @describe_depth($hierarchicalLevel)`
     },
@@ -633,11 +647,26 @@ Output.isTruthy = function(node, attrib) {
     // These attributes default to false for empty strings.
     case 'roleDescription':
       return !!node.roleDescription;
+    case 'selected':
+      return node.selected === true;
     default:
       return node[attrib] !== undefined || node.state[attrib];
   }
 };
 
+/**
+ * represents something 'falsey', e.g.: for selected:
+ * node.selected === false
+ */
+Output.isFalsey = function(node, attrib) {
+  switch (attrib) {
+    case 'selected':
+      return node.selected === false;
+    default:
+      return !Output.isTruthy(node, attrib);
+  }
+};
+
 Output.prototype = {
   /**
    * @return {boolean} True if there's any speech that will be output.
@@ -1324,9 +1353,6 @@ Output.prototype = {
               resolvedInfo.msgId + '_brl' :
               resolvedInfo.msgId;
           var msg = Msgs.getMsg(msgId);
-          if (token == StateType.SELECTED)
-            options.annotation.push(new Output.SelectionSpan(
-                buff.length, buff.length + msg.length));
           this.append_(buff, msg, options);
         } else if (token == 'posInSet') {
           if (node.posInSet !== undefined)
@@ -1345,7 +1371,14 @@ Output.prototype = {
             var attrib = cond.value.slice(1);
             if (Output.isTruthy(node, attrib))
               this.format_(node, cond.nextSibling, buff);
-            else
+            else if (Output.isFalsey(node, attrib))
+              this.format_(node, cond.nextSibling.nextSibling, buff);
+          } else if (token == 'nif') {
+            var cond = tree.firstChild;
+            var attrib = cond.value.slice(1);
+            if (Output.isFalsey(node, attrib))
+              this.format_(node, cond.nextSibling, buff);
+            else if (Output.isTruthy(node, attrib))
               this.format_(node, cond.nextSibling.nextSibling, buff);
           } else if (token == 'earcon') {
             // Ignore unless we're generating speech output.

diff --git a/chrome/browser/resources/chromeos/chromevox/cvox2/background/output_test.extjs b/chrome/browser/resources/chromeos/chromevox/cvox2/background/output_test.extjs
@@ -493,17 +493,17 @@ TEST_F('OutputE2ETest', 'ListBox', function() {
     var el = root.firstChild.firstChild.firstChild;
     var range = cursors.Range.fromNode(el);
     var o = new Output().withSpeechAndBraille(range, null, 'navigate');
-    checkSpeechOutput('1|List item| 1 of 2 |List box|with 2 items',
+    checkSpeechOutput('1|List item| 1 of 2 |Not selected|List box|with 2 items',
         [
           {value: 'name', start: 0, end: 1},
           {value: new Output.EarconAction('LIST_ITEM'), start: 0,end: 1},
-          {value: 'role', start: 21, end: 29}
+          {value: 'role', start: 34, end: 42}
         ],
         o);
     checkBrailleOutput(
-        '1 lstitm 1/2 lstbx +2',
-        [{value: new Output.NodeSpan(el), start: 0, end: 12},
-         {value: new Output.NodeSpan(el.parent), start: 13, end: 21}],
+        '1 lstitm 1/2 ( ) lstbx +2',
+        [{value: new Output.NodeSpan(el), start: 0, end: 16},
+         {value: new Output.NodeSpan(el.parent), start: 17, end: 25}],
         o);
   });
 });

diff --git a/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc b/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
@@ -69,10 +69,8 @@ class AutofillPopupChildView : public views::View {
     } else {
       // Options are selectable.
       node_data->role = ax::mojom::Role::kMenuItem;
-      if (is_selected_) {
-        node_data->AddState(ax::mojom::State::kSelected);
-      }
-      node_data->AddState(ax::mojom::State::kSelectable);
+      node_data->AddBoolAttribute(ax::mojom::BoolAttribute::kSelected,
+                                  is_selected_);
     }
 
     node_data->AddIntAttribute(ax::mojom::IntAttribute::kSetSize, set_size_);

diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -431,9 +431,8 @@ void OmniboxResultView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->AddIntAttribute(ax::mojom::IntAttribute::kSetSize,
                              model_->child_count());
 
-  node_data->AddState(ax::mojom::State::kSelectable);
-  if (IsSelected())
-    node_data->AddState(ax::mojom::State::kSelected);
+  node_data->AddBoolAttribute(ax::mojom::BoolAttribute::kSelected,
+                              IsSelected());
   if (is_hovered_)
     node_data->AddState(ax::mojom::State::kHovered);
 }

diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view_unittest.cc b/chrome/browser/ui/views/omnibox/omnibox_result_view_unittest.cc
@@ -217,8 +217,10 @@ TEST_F(OmniboxResultViewTest, AccessibleNodeData) {
   result_view()->SetMatch(match);
   ui::AXNodeData result_node_data;
   result_view()->GetAccessibleNodeData(&result_node_data);
-  EXPECT_TRUE(result_node_data.HasState(ax::mojom::State::kSelectable));
-  EXPECT_FALSE(result_node_data.HasState(ax::mojom::State::kSelected));
+  EXPECT_TRUE(
+      result_node_data.HasBoolAttribute(ax::mojom::BoolAttribute::kSelected));
+  EXPECT_FALSE(
+      result_node_data.GetBoolAttribute(ax::mojom::BoolAttribute::kSelected));
   EXPECT_EQ(result_node_data.role, ax::mojom::Role::kListBoxOption);
   EXPECT_EQ(
       result_node_data.GetString16Attribute(ax::mojom::StringAttribute::kName),
@@ -230,10 +232,12 @@ TEST_F(OmniboxResultViewTest, AccessibleNodeData) {
             6);
 
   // Select it and check selected state.
+  ui::AXNodeData result_after_click;
   result_view()->OnMousePressed(
       CreateEvent(ui::ET_MOUSE_PRESSED, ui::EF_LEFT_MOUSE_BUTTON));
-  result_view()->GetAccessibleNodeData(&result_node_data);
-  EXPECT_TRUE(result_node_data.HasState(ax::mojom::State::kSelected));
+  result_view()->GetAccessibleNodeData(&result_after_click);
+  EXPECT_TRUE(
+      result_after_click.GetBoolAttribute(ax::mojom::BoolAttribute::kSelected));
 
   // Check accessibility of list box.
   ui::AXNodeData popup_node_data;

diff --git a/chrome/browser/ui/views/tabs/tab.cc b/chrome/browser/ui/views/tabs/tab.cc
@@ -863,9 +863,8 @@ void Tab::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->role = ax::mojom::Role::kTab;
   node_data->SetName(controller_->GetAccessibleTabName(this));
   node_data->AddState(ax::mojom::State::kMultiselectable);
-  node_data->AddState(ax::mojom::State::kSelectable);
-  if (IsSelected())
-    node_data->AddState(ax::mojom::State::kSelected);
+  node_data->AddBoolAttribute(ax::mojom::BoolAttribute::kSelected,
+                              IsSelected());
 }
 
 void Tab::OnGestureEvent(ui::GestureEvent* event) {

diff --git a/chrome/common/extensions/api/automation.idl b/chrome/common/extensions/api/automation.idl
@@ -219,8 +219,6 @@
     protected,
     required,
     richlyEditable,
-    selectable,
-    selected,
     vertical,
     visited
   };
@@ -759,6 +757,9 @@
     // Indicates node text is line through.
     boolean lineThrough;
 
+    // Indicates whether this node is selected, unselected, or neither.
+    boolean? selected;
+
     //
     // Walking the tree.
     //

diff --git a/chrome/renderer/resources/extensions/automation/automation_node.js b/chrome/renderer/resources/extensions/automation/automation_node.js
@@ -884,7 +884,8 @@ var boolAttributes = [
     'containerLiveBusy',
     'liveAtomic',
     'modal',
-    'scrollable'
+    'scrollable',
+    'selected'
 ];
 
 var intAttributes = [

diff --git a/content/browser/accessibility/browser_accessibility_android.cc b/content/browser/accessibility/browser_accessibility_android.cc
@@ -289,7 +289,7 @@ bool BrowserAccessibilityAndroid::IsScrollable() const {
 }
 
 bool BrowserAccessibilityAndroid::IsSelected() const {
-  return HasState(ax::mojom::State::kSelected);
+  return GetBoolAttribute(ax::mojom::BoolAttribute::kSelected);
 }
 
 bool BrowserAccessibilityAndroid::IsSlider() const {

diff --git a/content/browser/accessibility/browser_accessibility_cocoa.mm b/content/browser/accessibility/browser_accessibility_cocoa.mm
@@ -1837,7 +1837,7 @@ - (NSArray*)selectedChildren {
   for (uint32_t index = 0; index < childCount; ++index) {
     BrowserAccessibility* child =
       browserAccessibility_->PlatformGetChild(index);
-    if (child->HasState(ax::mojom::State::kSelected))
+    if (child->GetBoolAttribute(ax::mojom::BoolAttribute::kSelected))
       [ret addObject:ToBrowserAccessibilityCocoa(child)];
   }
 
@@ -2112,7 +2112,8 @@ - (id)value {
         value = 2;
         break;
       default:
-        value = GetState(browserAccessibility_, ax::mojom::State::kSelected)
+        value = browserAccessibility_->GetBoolAttribute(
+                    ax::mojom::BoolAttribute::kSelected)
                     ? 1
                     : 0;
         break;

diff --git a/content/browser/accessibility/web_contents_accessibility_android.cc b/content/browser/accessibility/web_contents_accessibility_android.cc
@@ -1083,7 +1083,7 @@ void WebContentsAccessibilityAndroid::OnAutofillPopupDisplayed(
   ax_node_data.SetName("Autofill");
   ax_node_data.SetRestriction(ax::mojom::Restriction::kReadOnly);
   ax_node_data.AddState(ax::mojom::State::kFocusable);
-  ax_node_data.AddState(ax::mojom::State::kSelectable);
+  ax_node_data.AddBoolAttribute(ax::mojom::BoolAttribute::kSelected, false);
   g_autofill_popup_proxy_node_ax_node->SetData(ax_node_data);
   g_autofill_popup_proxy_node->Init(root_manager_,
                                     g_autofill_popup_proxy_node_ax_node);

diff --git a/content/renderer/accessibility/aom_content_ax_tree.cc b/content/renderer/accessibility/aom_content_ax_tree.cc
@@ -24,6 +24,8 @@ ax::mojom::BoolAttribute GetCorrespondingAXAttribute(
       return ax::mojom::BoolAttribute::kBusy;
     case blink::WebAOMBoolAttribute::AOM_ATTR_MODAL:
       return ax::mojom::BoolAttribute::kModal;
+    case blink::WebAOMBoolAttribute::AOM_ATTR_SELECTED:
+      return ax::mojom::BoolAttribute::kSelected;
     default:
       return ax::mojom::BoolAttribute::kNone;
   }
@@ -111,8 +113,6 @@ ax::mojom::State GetCorrespondingStateFlag(blink::WebAOMBoolAttribute attr) {
       return ax::mojom::State::kMultiselectable;
     case blink::WebAOMBoolAttribute::AOM_ATTR_REQUIRED:
       return ax::mojom::State::kRequired;
-    case blink::WebAOMBoolAttribute::AOM_ATTR_SELECTED:
-      return ax::mojom::State::kSelected;
     default:
       return ax::mojom::State::kNone;
   }

diff --git a/content/renderer/accessibility/blink_ax_enum_conversion.cc b/content/renderer/accessibility/blink_ax_enum_conversion.cc
@@ -44,14 +44,13 @@ void AXStateFromBlink(const blink::WebAXObject& o, ui::AXNodeData* dst) {
   if (o.IsRequired())
     dst->AddState(ax::mojom::State::kRequired);
 
-  if (o.IsSelected() != blink::kWebAXSelectedStateUndefined)
-    dst->AddState(ax::mojom::State::kSelectable);
-
   if (o.IsEditable())
     dst->AddState(ax::mojom::State::kEditable);
 
-  if (o.IsSelected() == blink::kWebAXSelectedStateTrue)
-    dst->AddState(ax::mojom::State::kSelected);
+  if (o.IsSelected() != blink::kWebAXSelectedStateUndefined) {
+    dst->AddBoolAttribute(ax::mojom::BoolAttribute::kSelected,
+                          o.IsSelected() == blink::kWebAXSelectedStateTrue);
+  }
 
   if (o.IsRichlyEditable())
     dst->AddState(ax::mojom::State::kRichlyEditable);

diff --git a/content/test/data/accessibility/aria/aria-cell-expected-blink.txt b/content/test/data/accessibility/aria/aria-cell-expected-blink.txt
@@ -1,17 +1,17 @@
 rootWebArea
 ++table
-++++row selectable
-++++++columnHeader selectable name='Browser'
+++++row selected=false
+++++++columnHeader name='Browser' selected=false
 ++++++++staticText name='Browser'
 ++++++++++inlineTextBox name='Browser'
-++++++columnHeader selectable name='Rendering Engine'
+++++++columnHeader name='Rendering Engine' selected=false
 ++++++++staticText name='Rendering Engine'
 ++++++++++inlineTextBox name='Rendering Engine'
-++++row selectable
-++++++cell selectable name='Chrome'
+++++row selected=false
+++++++cell name='Chrome' selected=false
 ++++++++staticText name='Chrome'
 ++++++++++inlineTextBox name='Chrome'
-++++++cell selectable name='Blink'
+++++++cell name='Blink' selected=false
 ++++++++staticText name='Blink'
 ++++++++++inlineTextBox name='Blink'
 ++++column