Bug 1591409 - improve keyboard a11y checks by avoiding false positive for interactive elements when they are focusable. r=nchevobbe
authorYura Zenevich <yura.zenevich@gmail.com>
Thu, 14 Nov 2019 06:00:27 +0000
changeset 501925 40b09567e312348b311865f3d54bfd08d4dbf58b
parent 501924 9f6892c03bd8dfcc604461ea90e71430c220fb74
child 501926 6b2ea992707b0475e9424b6ae02eb1ebb32df6d6
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnchevobbe
bugs1591409
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1591409 - improve keyboard a11y checks by avoiding false positive for interactive elements when they are focusable. r=nchevobbe Differential Revision: https://phabricator.services.mozilla.com/D52201
devtools/server/actors/accessibility/audit/keyboard.js
devtools/server/tests/browser/browser_accessibility_keyboard_audit.js
devtools/server/tests/browser/doc_accessibility_keyboard_audit.html
--- a/devtools/server/actors/accessibility/audit/keyboard.js
+++ b/devtools/server/actors/accessibility/audit/keyboard.js
@@ -82,16 +82,32 @@ const INTERACTIVE_ROLES = new Set([
   Ci.nsIAccessibleRole.ROLE_OUTLINE,
   Ci.nsIAccessibleRole.ROLE_OUTLINEITEM,
   Ci.nsIAccessibleRole.ROLE_PAGETAB,
   Ci.nsIAccessibleRole.ROLE_PARENT_MENUITEM,
   Ci.nsIAccessibleRole.ROLE_RADIO_MENU_ITEM,
   Ci.nsIAccessibleRole.ROLE_RICH_OPTION,
 ]);
 
+const INTERACTIVE_IF_FOCUSABLE_ROLES = new Set([
+  // If article is focusable, we can assume it is inside a feed.
+  Ci.nsIAccessibleRole.ROLE_ARTICLE,
+  // Column header can be focusable.
+  Ci.nsIAccessibleRole.ROLE_COLUMNHEADER,
+  Ci.nsIAccessibleRole.ROLE_GRID_CELL,
+  Ci.nsIAccessibleRole.ROLE_MENUBAR,
+  Ci.nsIAccessibleRole.ROLE_MENUPOPUP,
+  Ci.nsIAccessibleRole.ROLE_PAGETABLIST,
+  // Row header can be focusable.
+  Ci.nsIAccessibleRole.ROLE_ROWHEADER,
+  Ci.nsIAccessibleRole.ROLE_SCROLLBAR,
+  Ci.nsIAccessibleRole.ROLE_SEPARATOR,
+  Ci.nsIAccessibleRole.ROLE_TOOLBAR,
+]);
+
 /**
  * Determine if a node is dead or is not an element node.
  *
  * @param   {DOMNode} node
  *          Node to be tested for validity.
  *
  * @returns {Boolean}
  *          True if the node is either dead or is not an element node.
@@ -101,16 +117,59 @@ function isInvalidNode(node) {
     !node ||
     Cu.isDeadWrapper(node) ||
     node.nodeType !== nodeConstants.ELEMENT_NODE ||
     !node.ownerGlobal
   );
 }
 
 /**
+ * Get role attribute for an accessible object if specified for its
+ * corresponding DOMNode.
+ *
+ * @param   {nsIAccessible} accessible
+ *          Accessible for which to determine its role attribute value.
+ *
+ * @returns {null|String}
+ *          Role attribute value if specified.
+ */
+function getAriaRoles(accessible) {
+  try {
+    return accessible.attributes.getStringProperty("xml-roles");
+  } catch (e) {
+    // No xml-roles. nsPersistentProperties throws if the attribute for a key
+    // is not found.
+  }
+
+  return null;
+}
+
+/**
+ * Determine if accessible is focusable with the keyboard.
+ *
+ * @param   {nsIAccessible} accessible
+ *          Accessible for which to determine if it is keyboard focusable.
+ *
+ * @returns {Boolean}
+ *          True if focusable with the keyboard.
+ */
+function isKeyboardFocusable(accessible) {
+  const state = {};
+  accessible.getState(state, {});
+  // State will be focusable even if the tabindex is negative.
+  return (
+    state.value & Ci.nsIAccessibleStates.STATE_FOCUSABLE &&
+    // Platform accessibility will still report STATE_FOCUSABLE even with the
+    // tabindex="-1" so we need to check that it is >= 0 to be considered
+    // keyboard focusable.
+    accessible.DOMNode.tabIndex > -1
+  );
+}
+
+/**
  * Determine if a current node has focus specific styling by applying a
  * focus-related pseudo class (such as :focus or :-moz-focusring) to a focusable
  * node.
  *
  * @param   {DOMNode} focusableNode
  *          Node to apply focus-related pseudo class to.
  * @param   {DOMNode} currentNode
  *          Node to be checked for having focus specific styling.
@@ -240,19 +299,17 @@ function hasFocusStyling(focusableNode, 
  */
 function focusStyleRule(accessible) {
   const { DOMNode } = accessible;
   if (isInvalidNode(DOMNode)) {
     return null;
   }
 
   // Ignore non-focusable elements.
-  const state = {};
-  accessible.getState(state, {});
-  if (!(state.value & Ci.nsIAccessibleStates.STATE_FOCUSABLE)) {
+  if (!isKeyboardFocusable(accessible)) {
     return null;
   }
 
   if (hasFocusStyling(DOMNode, DOMNode)) {
     return null;
   }
 
   // If no browser or author focus styling was found, check if the node is a
@@ -309,31 +366,21 @@ function focusableRule(accessible) {
   const state = {};
   accessible.getState(state, {});
   // We only expect in interactive accessible object to be focusable if it is
   // not disabled.
   if (state.value & Ci.nsIAccessibleStates.STATE_UNAVAILABLE) {
     return null;
   }
 
-  // State will be focusable even if the tabindex is negative.
-  if (
-    state.value & Ci.nsIAccessibleStates.STATE_FOCUSABLE &&
-    accessible.DOMNode.tabIndex > -1
-  ) {
+  if (isKeyboardFocusable(accessible)) {
     return null;
   }
 
-  let ariaRoles;
-  try {
-    ariaRoles = accessible.attributes.getStringProperty("xml-roles");
-  } catch (e) {
-    // No xml-roles. nsPersistentProperties throws if the attribute for a key
-    // is not found.
-  }
+  const ariaRoles = getAriaRoles(accessible);
   if (
     ariaRoles &&
     (ariaRoles.includes("combobox") || ariaRoles.includes("listbox"))
   ) {
     // Do not force ARIA combobox or listbox to be focusable.
     return null;
   }
 
@@ -356,22 +403,34 @@ function semanticsRule(accessible) {
   if (
     INTERACTIVE_ROLES.has(accessible.role) ||
     // Visible listboxes will have focusable state when inside comboboxes.
     accessible.role === Ci.nsIAccessibleRole.ROLE_COMBOBOX_LIST
   ) {
     return null;
   }
 
-  const state = {};
-  accessible.getState(state, {});
-  if (state.value & Ci.nsIAccessibleStates.STATE_FOCUSABLE) {
+  if (isKeyboardFocusable(accessible)) {
+    if (INTERACTIVE_IF_FOCUSABLE_ROLES.has(accessible.role)) {
+      return null;
+    }
+
+    // ROLE_TABLE is used for grids too which are considered interactive.
+    if (accessible.role === Ci.nsIAccessibleRole.ROLE_TABLE) {
+      const ariaRoles = getAriaRoles(accessible);
+      if (ariaRoles && ariaRoles.includes("grid")) {
+        return null;
+      }
+    }
+
     return { score: WARNING, issue: FOCUSABLE_NO_SEMANTICS };
   }
 
+  const state = {};
+  accessible.getState(state, {});
   if (
     // Ignore text leafs.
     accessible.role === Ci.nsIAccessibleRole.ROLE_TEXT_LEAF ||
     // Ignore accessibles with no accessible actions.
     accessible.actionCount === 0 ||
     // Ignore labels that have a label for relation with their target because
     // they are clickable.
     (accessible.role === Ci.nsIAccessibleRole.ROLE_LABEL &&
@@ -407,19 +466,17 @@ function semanticsRule(accessible) {
  *         tabindex attribute is less than 1, audit report object otherwise.
  */
 function tabIndexRule(accessible) {
   const { DOMNode } = accessible;
   if (isInvalidNode(DOMNode)) {
     return null;
   }
 
-  const state = {};
-  accessible.getState(state, {});
-  if (!(state.value & Ci.nsIAccessibleStates.STATE_FOCUSABLE)) {
+  if (!isKeyboardFocusable(accessible)) {
     return null;
   }
 
   if (DOMNode.tabIndex > 0) {
     return { score: WARNING, issue: FOCUSABLE_POSITIVE_TABINDEX };
   }
 
   return null;
--- a/devtools/server/tests/browser/browser_accessibility_keyboard_audit.js
+++ b/devtools/server/tests/browser/browser_accessibility_keyboard_audit.js
@@ -180,16 +180,162 @@ add_task(async function() {
     ["Image inside an anchor (onmousedown)", "#img-6", null],
     ["Image inside an anchor (onclick)", "#img-7", null],
     ["Image inside an anchor (onmouseup)", "#img-8", null],
     [
       "Section with a collapse action from aria-expanded attribute",
       "#section-1",
       null,
     ],
+    ["Tabindex -1 should not report an element as focusable", "#main", null],
+    [
+      "Not keyboard focusable element with no focus styling.",
+      "#not-keyboard-focusable-1",
+      null,
+    ],
+    ["Interactive grid that is not focusable.", "#grid-1", null],
+    ["Focusable interactive grid.", "#grid-2", null],
+    [
+      "Non interactive ARIA table does not need to be focusable.",
+      "#table-1",
+      null,
+    ],
+    [
+      "Focusable ARIA table does not have interactive semantics",
+      "#table-2",
+      { score: "WARNING", issue: "FOCUSABLE_NO_SEMANTICS" },
+    ],
+    ["Non interactive table does not need to be focusable.", "#table-3", null],
+    [
+      "Focusable table does not have interactive semantics",
+      "#table-4",
+      { score: "WARNING", issue: "FOCUSABLE_NO_SEMANTICS" },
+    ],
+    [
+      "Article that is not focusable is not considered interactive",
+      "#article-1",
+      null,
+    ],
+    ["Focusable article is considered interactive", "#article-2", null],
+    [
+      "Column header that is not focusable is not considered interactive (ARIA grid)",
+      "#columnheader-1",
+      null,
+    ],
+    [
+      "Column header that is not focusable is not considered interactive (ARIA table)",
+      "#columnheader-2",
+      null,
+    ],
+    [
+      "Column header that is not focusable is not considered interactive (table)",
+      "#columnheader-3",
+      null,
+    ],
+    [
+      "Column header that is focusable is considered interactive (table)",
+      "#columnheader-4",
+      null,
+    ],
+    [
+      "Column header that is not focusable is not considered interactive (table as ARIA grid)",
+      "#columnheader-5",
+      null,
+    ],
+    [
+      "Column header that is focusable is considered interactive (table as ARIA grid)",
+      "#columnheader-6",
+      null,
+    ],
+    [
+      "Row header that is not focusable is not considered interactive",
+      "#rowheader-1",
+      null,
+    ],
+    [
+      "Row header that is not focusable is not considered interactive",
+      "#rowheader-2",
+      null,
+    ],
+    [
+      "Row header that is not focusable is not considered interactive",
+      "#rowheader-3",
+      null,
+    ],
+    [
+      "Row header that is focusable is considered interactive",
+      "#rowheader-4",
+      null,
+    ],
+    [
+      "Row header that is not focusable is not considered interactive (table as ARIA grid)",
+      "#rowheader-5",
+      null,
+    ],
+    [
+      "Row header that is focusable is considered interactive (table as ARIA grid)",
+      "#rowheader-6",
+      null,
+    ],
+    [
+      "Gridcell that is not focusable is not considered interactive (ARIA grid)",
+      "#gridcell-1",
+      null,
+    ],
+    [
+      "Gridcell that is focusable is considered interactive (ARIA grid)",
+      "#gridcell-2",
+      null,
+    ],
+    [
+      "Gridcell that is not focusable is not considered interactive (table as ARIA grid)",
+      "#gridcell-3",
+      null,
+    ],
+    [
+      "Gridcell that is focusable is considered interactive (table as ARIA grid)",
+      "#gridcell-4",
+      null,
+    ],
+    [
+      "Tab list that is not focusable is not considered interactive",
+      "#tablist-1",
+      null,
+    ],
+    ["Focusable tab list is considered interactive", "#tablist-2", null],
+    [
+      "Scrollbar that is not focusable is not considered interactive",
+      "#scrollbar-1",
+      null,
+    ],
+    ["Focusable scrollbar is considered interactive", "#scrollbar-2", null],
+    [
+      "Separator that is not focusable is not considered interactive",
+      "#separator-1",
+      null,
+    ],
+    ["Focusable separator is considered interactive", "#separator-2", null],
+    [
+      "Toolbar that is not focusable is not considered interactive",
+      "#toolbar-1",
+      null,
+    ],
+    ["Focusable toolbar is considered interactive", "#toolbar-2", null],
+    [
+      "Menu popup that is not focusable is not considered interactive",
+      "#menu-1",
+      null,
+    ],
+    ["Focusable menu popup is considered interactive", "#menu-2", null],
+    [
+      "Menubar that is not focusable is not considered interactive",
+      "#menubar-1",
+      null,
+    ],
+    ["Focusable menubar is considered interactive", "#menubar-2", null],
   ];
 
   for (const [description, selector, expected] of tests) {
     info(description);
     const node = await walker.querySelector(walker.rootNode, selector);
     const front = await a11yWalker.getAccessibleFor(node);
     const audit = await front.audit({ types: [KEYBOARD] });
     Assert.deepEqual(
--- a/devtools/server/tests/browser/doc_accessibility_keyboard_audit.html
+++ b/devtools/server/tests/browser/doc_accessibility_keyboard_audit.html
@@ -77,10 +77,74 @@
   </a>
   <a onclick="">
     <img id="img-7" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABwAAAAcCAYAAAByDd+UAAAAJklEQVRIie3NMREAAAgAoe9fWls4eAzMVM0xoVAoFAqFQqFQ+C9chp4NHvu+4Q4AAAAASUVORK5CYII=" alt="alt text">
   </a>
   <a onmouseup="">
     <img id="img-8" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABwAAAAcCAYAAAByDd+UAAAAJklEQVRIie3NMREAAAgAoe9fWls4eAzMVM0xoVAoFAqFQqFQ+C9chp4NHvu+4Q4AAAAASUVORK5CYII=" alt="alt text">
   </a>
   <section id="section-1" class="collapsible-section top-sites animation-enabled" aria-expanded="true"></section>
+  <main id="main" tabindex="-1">Main content</main>
+  <div id="not-keyboard-focusable-1" tabindex="-1">Not keyboard fqocusable with no focus style.</div>
+  <div id="grid-1" role="grid" aria-label="Interactive grid"></div>
+  <div id="grid-2" tabindex="0" role="grid" aria-label="Interactive grid"></div>
+  <div id="table-1" role="table" aria-label="Non-interactive ARIA table"></div>
+  <div id="table-2" tabindex="0" role="table" aria-label="Non-interactive ARIA table"></div>
+  <table id="table-3" aria-label="Non-interactive table"></table>
+  <table id="table-4" tabindex="0" aria-label="Non-interactive table"></table>
+  <div id="article-1" role="article"></div>
+  <div id="article-2" role="article" tabindex="0"></div>
+  <div role="grid" aria-label="Interactive grid">
+    <div id="columnheader-1" role="columnheader"></div>
+    <div id="rowheader-1" role="rowheader"></div>
+    <div id="gridcell-1" role="gridcell"></div>
+    <div id="gridcell-2" role="gridcell" tabindex="0"></div>
+  </div>
+  <div role="table" aria-label="Non-interactive table">
+    <div id="columnheader-2" role="columnheader"></div>
+    <div id="rowheader-2" role="rowheader"></div>
+  </div>
+  <table>
+    <tr>
+      <th id="columnheader-3">Animals</th>
+    </tr>
+    <tr>
+      <th id="columnheader-4" tabindex="0">Hippopotamus</th>
+    </tr>
+    <tr>
+      <th id="rowheader-3">Horse</th>
+      <td>Mare</td>
+    </tr>
+    <tr>
+      <th id="rowheader-4" tabindex="0">Chicken</th>
+      <td>Hen</td>
+    </tr>
+  </table>
+  <table role="grid">
+    <tr>
+      <th id="columnheader-5">Animals</th>
+    </tr>
+    <tr>
+      <th id="columnheader-6" tabindex="0">Hippopotamus</th>
+    </tr>
+    <tr>
+      <th id="rowheader-5">Horse</th>
+      <td id="gridcell-3">Mare</td>
+    </tr>
+    <tr>
+      <th id="rowheader-6" tabindex="0">Chicken</th>
+      <td id="gridcell-4" tabindex="0">Hen</td>
+    </tr>
+  </table>
+  <div id="tablist-1" role="tablist"></div>
+  <div id="tablist-2" role="tablist" tabindex="0"></div>
+  <div id="scrollbar-1" role="scrollbar"></div>
+  <div id="scrollbar-2" role="scrollbar" tabindex="0"></div>
+  <div id="separator-1" role="separator"></div>
+  <div id="separator-2" role="separator" tabindex="0"></div>
+  <div id="toolbar-1" role="toolbar"></div>
+  <div id="toolbar-2" role="toolbar" tabindex="0"></div>
+  <div id="menu-1" role="menu"></div>
+  <div id="menu-2" role="menu" tabindex="0"></div>
+  <div id="menubar-1" role="menubar"></div>
+  <div id="menubar-2" role="menubar" tabindex="0"></div>
 </body>
 </html>