From 4d1f8851539af02e5c37095d9231c48dbde4a13b Mon Sep 17 00:00:00 2001
From: Todd Kloots <kloots@twitter.com>
Date: Wed, 10 Jun 2015 22:51:40 -0700
Subject: [PATCH] [fixed] bug where placeholder links required a tabindex and
 an ARIA role (closes #62) [fixed] bug where elements with role=none required
 a label (closes #63) [fixed] bug where elements with aria-hidden required a
 label (closes #64) [added] test to ensure interactive elements hidden using
 aria-hidden are removed from the tab flow

---
 lib/__tests__/index-test.js | 86 ++++++++++++++++++++++++++++++++++++-
 lib/assertions.js           | 70 ++++++++++++++++++++++++------
 lib/index.js                |  8 +---
 3 files changed, 142 insertions(+), 22 deletions(-)

diff --git a/lib/__tests__/index-test.js b/lib/__tests__/index-test.js
index ad760a6..3b62e4f 100644
--- a/lib/__tests__/index-test.js
+++ b/lib/__tests__/index-test.js
@@ -61,6 +61,12 @@ describe('props', () => {
         <div onClick={k} role="button"/>;
       });
     });
+
+    it('does not warn with no role and `aria-hidden="true"`', () => {
+      doNotExpectWarning(assertions.props.onClick.NO_ROLE.msg, () => {
+        <a aria-hidden="true" onClick={k}/>;
+      });
+    });
   });
 
   describe('tabIndex', () => {
@@ -98,6 +104,48 @@ describe('props', () => {
       });
     });
   });
+
+  describe('aria-hidden', () => {
+    describe('when set to `true`', () => {
+      it('warns when applied to an interactive element without `tabIndex="-1"`', () => {
+        expectWarning(assertions.props['aria-hidden'].TABINDEX_REQUIRED_WHEN_ARIA_HIDDEN.msg, () => {
+          <a aria-hidden="true" href="/foo"/>;
+        });
+      });
+
+      it('warns when applied to an interactive element with `tabIndex="0"`', () => {
+        expectWarning(assertions.props['aria-hidden'].TABINDEX_REQUIRED_WHEN_ARIA_HIDDEN.msg, () => {
+          <a aria-hidden="true" tabIndex="0"/>;
+        });
+      });
+
+      it('does not warn when applied to a placeholder link', () => {
+        expectWarning(assertions.props['aria-hidden'].TABINDEX_REQUIRED_WHEN_ARIA_HIDDEN.msg, () => {
+          <a aria-hidden="true"/>;
+        });
+      });
+
+      it('does not warn when applied to an interactive element with `tabIndex="-1"`', () => {
+        doNotExpectWarning(assertions.props['aria-hidden'].TABINDEX_REQUIRED_WHEN_ARIA_HIDDEN.msg, () => {
+          <a aria-hidden="true" tabIndex="-1"/>;
+        });
+      });
+
+      it('does not warn when applied to a non-interactive element', () => {
+        doNotExpectWarning(assertions.props['aria-hidden'].TABINDEX_REQUIRED_WHEN_ARIA_HIDDEN.msg, () => {
+          <div aria-hidden="true"/>;
+        });
+      });
+    });
+
+    describe('when set to `false`', () =>{
+      it('does not warn when applied to an interactive element with `tabIndex="-1"`', () => {
+        doNotExpectWarning(assertions.props['aria-hidden'].TABINDEX_REQUIRED_WHEN_ARIA_HIDDEN.msg, () => {
+          <a aria-hidden="false" tabIndex="-1"/>;
+        });
+      });
+    });
+  });
 });
 
 describe('tags', () => {
@@ -132,6 +180,22 @@ describe('tags', () => {
   });
 
   describe('a', () => {
+    describe('placeholder links without href', () => {
+      it('does not warn', () => {
+        doNotExpectWarning(assertions.tags.a.HASH_HREF_NEEDS_BUTTON.msg, () => {
+          <a class="foo" />;
+        });
+      });
+    });
+
+    describe('placeholder links without tabindex', () => {
+      it('does not warn', () => {
+        doNotExpectWarning(assertions.tags.a.TABINDEX_NEEDS_BUTTON.msg, () => {
+          <a class="foo" />;
+        });
+      });
+    });
+
     describe('with [href="#"]', () => {
       it('warns', () => {
         expectWarning(assertions.tags.a.HASH_HREF_NEEDS_BUTTON.msg, () => {
@@ -194,9 +258,27 @@ describe('labels', () => {
     });
   });
 
-  it('does not warn when the ARIA role is presentation', () => {
+  it('does not warn when `role="presentation"`', () => {
     doNotExpectWarning(assertions.render.NO_LABEL.msg, () => {
-      <span role="presentation" />;
+      <img role="presentation" />;
+    });
+  });
+
+  it('does not warn when `role="none"`', () => {
+    doNotExpectWarning(assertions.render.NO_LABEL.msg, () => {
+      <img role="none" />;
+    });
+  });
+
+  it('does not warn when `aria-hidden="true"`', () => {
+    doNotExpectWarning(assertions.render.NO_LABEL.msg, () => {
+      <button aria-hidden="true" />;
+    });
+  });
+
+  it('warns when `aria-hidden="false"`', () => {
+    expectWarning(assertions.render.NO_LABEL.msg, () => {
+      <button aria-hidden="false" />;
     });
   });
 
diff --git a/lib/assertions.js b/lib/assertions.js
index a6534a5..aa76ef4 100644
--- a/lib/assertions.js
+++ b/lib/assertions.js
@@ -21,6 +21,12 @@ var INTERACTIVE = {
   }
 };
 
+const presentationRoles = new Set(['presentation', 'none']);
+
+var isHiddenFromAT = (props) => {
+  return props['aria-hidden'] == 'true';
+};
+
 var hasAlt = (props) => {
   return typeof props.alt === 'string';
 };
@@ -105,11 +111,21 @@ var hasChildTextNode = (props, children, failureCB) => {
   return hasText;
 };
 
+exports.mobileExclusions = [
+  'NO_TABINDEX',
+  'BUTTON_ROLE_SPACE',
+  'BUTTON_ROLE_ENTER',
+  'TABINDEX_REQUIRED_WHEN_ARIA_HIDDEN'
+];
+
 exports.props = {
   onClick: {
     NO_ROLE: {
       msg: 'You have a click handler on a non-interactive element but no `role` DOM property. It will be unclear what this element is supposed to do to a screen-reader user. http://www.w3.org/TR/wai-aria/roles#role_definitions',
       test (tagName, props, children) {
+        if (isHiddenFromAT(props))
+          return true;
+
         return !(!isInteractive(tagName, props) && !props.role);
       }
     },
@@ -117,6 +133,9 @@ exports.props = {
     NO_TABINDEX: {
       msg: 'You have a click handler on a non-interactive element but no `tabIndex` DOM property. The element will not be navigable or interactive by keyboard users. http://www.w3.org/TR/wai-aria-practices/#focus_tabindex',
       test (tagName, props, children) {
+        if (isHiddenFromAT(props))
+          return true;
+
         return !(
           !isInteractive(tagName, props) &&
           props.tabIndex == null // tabIndex={0} is valid
@@ -127,6 +146,9 @@ exports.props = {
     BUTTON_ROLE_SPACE: {
       msg: 'You have `role="button"` but did not define an `onKeyDown` handler. Add it, and have the "Space" key do the same thing as an `onClick` handler.',
       test (tagName, props, children) {
+        if (isHiddenFromAT(props))
+          return true;
+
         return !(props.role === 'button' && !props.onKeyDown);
       }
     },
@@ -134,10 +156,25 @@ exports.props = {
     BUTTON_ROLE_ENTER: {
       msg: 'You have `role="button"` but did not define an `onKeyDown` handler. Add it, and have the "Enter" key do the same thing as an `onClick` handler.',
       test (tagName, props, children) {
+        if (isHiddenFromAT(props))
+          return true;
+
         return !(props.role === 'button' && !props.onKeyDown);
       }
     }
+  },
 
+  'aria-hidden': {
+    'TABINDEX_REQUIRED_WHEN_ARIA_HIDDEN': {
+      msg: 'You have `aria-hidden="true"` applied to an interactive element but have not removed it from the tab flow. This could result in a hidden tab stop for users of screen readers.',
+      test (tagName, props, children) {
+        return !(
+          (isInteractive(tagName, props) || (tagName == 'a' && !props.href)) &&
+          props['aria-hidden'] == 'true' &&
+          props.tabIndex != '-1'
+        );
+      }
+    }
   }
 };
 
@@ -146,13 +183,19 @@ exports.tags = {
     HASH_HREF_NEEDS_BUTTON: {
       msg: 'You have an anchor with `href="#"` and no `role` DOM property. Add `role="button"` or better yet, use a `<button/>`.',
       test (tagName, props, children) {
+        if (isHiddenFromAT(props))
+          return true;
+
         return !(!props.role && props.href === '#');
       }
     },
     TABINDEX_NEEDS_BUTTON: {
       msg: 'You have an anchor with a tabIndex, no `href` and no `role` DOM property. Add `role="button"` or better yet, use a `<button/>`.',
       test (tagName, props, children) {
-        return !(!props.role && props.tabIndex !== null && !props.href);
+        if (isHiddenFromAT(props))
+          return true;
+
+        return !(!props.role && props.tabIndex != null && !props.href);
       }
     }
   },
@@ -161,6 +204,9 @@ exports.tags = {
     MISSING_ALT: {
       msg: 'You forgot an `alt` DOM property on an image. Screen-reader users will not know what it is.',
       test (tagName, props, children) {
+        if (isHiddenFromAT(props))
+          return true;
+
         return hasAlt(props);
       }
     },
@@ -169,6 +215,9 @@ exports.tags = {
       // TODO: have some way to set localization strings to match against
       msg: 'Screen-readers already announce `img` tags as an image, you don\'t need to use the word "image" in the description',
       test (tagName, props, children) {
+        if (isHiddenFromAT(props))
+          return true;
+
         return !(hasAlt(props) && props.alt.match('image'));
       }
     }
@@ -179,22 +228,17 @@ exports.render = {
   NO_LABEL: {
     msg: 'You have an unlabled element or control. Add `aria-label` or `aria-labelled-by` attribute, or put some text in the element.',
     test (tagName, props, children, failureCB) {
-      var labelRequired = (
-          isInteractive(tagName, props) ||
-          props.role && props.role != 'presentation'
-        );
+      if (isHiddenFromAT(props) || presentationRoles.has(props.role))
+        return;
 
-      if (!labelRequired)
+      if (!(isInteractive(tagName, props) || props.role))
         return;
 
       var failed = !(
-        labelRequired &&
-        (
-          props['aria-label'] ||
-          props['aria-labelled-by'] ||
-          (tagName === 'img' && props.alt) ||
-          hasChildTextNode(props, children, failureCB)
-        )
+        props['aria-label'] ||
+        props['aria-labelled-by'] ||
+        (tagName === 'img' && props.alt) ||
+        hasChildTextNode(props, children, failureCB)
       );
 
       if (failed)
diff --git a/lib/index.js b/lib/index.js
index 95539d7..015ca93 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -1,17 +1,11 @@
 var assertions = require('./assertions');
 var after = require('./after');
 
-const mobileExclusions = [
-  'NO_TABINDEX',
-  'BUTTON_ROLE_SPACE',
-  'BUTTON_ROLE_ENTER'
-];
-
 var shouldRunTest = (testName, options) => {
   var exclude = options.exclude || [];
 
   if (options.device == 'mobile') {
-    exclude = new Set(exclude.concat(mobileExclusions));
+    exclude = new Set(exclude.concat(assertions.mobileExclusions));
     exclude = [...exclude];
   }