From a9c4ae90b7e31349deb29211a110b42fba1c6a70 Mon Sep 17 00:00:00 2001
From: Zama Khan Mohammed <mohammedzamakhan@gmail.com>
Date: Fri, 1 Mar 2019 20:30:25 -0600
Subject: [PATCH] fix(rule):  don't check keyup events for some elements (#772)

---
 package.json                                  |   1 +
 src/templateClickEventsHaveKeyEventsRule.ts   |  21 ++
 src/util/attributesComparator.ts              |  24 +++
 src/util/getAttributeValue.ts                 |  13 +-
 src/util/getLiteralValue.ts                   |   8 +
 src/util/isHiddenFromScreenReader.ts          |  24 +++
 src/util/isInteractiveElement.ts              |  94 +++++++++
 src/util/isPresentationRole.ts                |   7 +
 ...mplateClickEventsHaveKeyEventsRule.spec.ts | 187 +++++++++++++++++-
 yarn.lock                                     |   6 +
 10 files changed, 377 insertions(+), 8 deletions(-)
 create mode 100644 src/util/attributesComparator.ts
 create mode 100644 src/util/getLiteralValue.ts
 create mode 100644 src/util/isHiddenFromScreenReader.ts
 create mode 100644 src/util/isInteractiveElement.ts
 create mode 100644 src/util/isPresentationRole.ts

diff --git a/package.json b/package.json
index 148e93656..b78b7bbda 100644
--- a/package.json
+++ b/package.json
@@ -102,6 +102,7 @@
   "dependencies": {
     "app-root-path": "^2.1.0",
     "aria-query": "^3.0.0",
+    "axobject-query": "^2.0.2",
     "css-selector-tokenizer": "^0.7.0",
     "cssauron": "^1.4.0",
     "damerau-levenshtein": "^1.0.4",
diff --git a/src/templateClickEventsHaveKeyEventsRule.ts b/src/templateClickEventsHaveKeyEventsRule.ts
index fd078e79d..289d28cdc 100644
--- a/src/templateClickEventsHaveKeyEventsRule.ts
+++ b/src/templateClickEventsHaveKeyEventsRule.ts
@@ -1,8 +1,14 @@
 import { ElementAst } from '@angular/compiler';
+import { dom } from 'aria-query';
 import { IRuleMetadata, RuleFailure, Rules } from 'tslint/lib';
 import { SourceFile } from 'typescript/lib/typescript';
 import { NgWalker } from './angular/ngWalker';
 import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor';
+import { isInteractiveElement } from './util/isInteractiveElement';
+import { isPresentationRole } from './util/isPresentationRole';
+import { isHiddenFromScreenReader } from './util/isHiddenFromScreenReader';
+
+const domElements = new Set(dom.keys());
 
 export class Rule extends Rules.AbstractRule {
   static readonly metadata: IRuleMetadata = {
@@ -37,6 +43,21 @@ class TemplateClickEventsHaveKeyEventsVisitor extends BasicTemplateAstVisitor {
     if (!hasClick) {
       return;
     }
+
+    if (!domElements.has(el.name)) {
+      // Do not test components, as we do not know what
+      // low-level DOM element this maps to.
+      return;
+    }
+
+    if (isPresentationRole(el) || isHiddenFromScreenReader(el)) {
+      return;
+    }
+
+    if (isInteractiveElement(el)) {
+      return;
+    }
+
     const hasKeyEvent = el.outputs.some(output => output.name === 'keyup' || output.name === 'keydown' || output.name === 'keypress');
 
     if (hasKeyEvent) {
diff --git a/src/util/attributesComparator.ts b/src/util/attributesComparator.ts
new file mode 100644
index 000000000..6e6037c32
--- /dev/null
+++ b/src/util/attributesComparator.ts
@@ -0,0 +1,24 @@
+import { ElementAst, AttrAst, BoundElementPropertyAst } from '@angular/compiler';
+import { getAttributeValue } from './getAttributeValue';
+import { getLiteralValue } from './getLiteralValue';
+
+export function attributesComparator(baseAttributes: any = [], el: ElementAst): boolean {
+  const attributes: Array<AttrAst | BoundElementPropertyAst> = [...el.attrs, ...el.inputs];
+  return baseAttributes.every(
+    (baseAttr): boolean =>
+      attributes.some(
+        (attribute): boolean => {
+          if (el.name === 'a' && attribute.name === 'routerLink') {
+            return true;
+          }
+          if (baseAttr.name !== attribute.name) {
+            return false;
+          }
+          if (baseAttr.value && baseAttr.value !== getLiteralValue(getAttributeValue(el, baseAttr.name))) {
+            return false;
+          }
+          return true;
+        }
+      )
+  );
+}
diff --git a/src/util/getAttributeValue.ts b/src/util/getAttributeValue.ts
index fdfd2c9ac..296837114 100644
--- a/src/util/getAttributeValue.ts
+++ b/src/util/getAttributeValue.ts
@@ -1,4 +1,5 @@
-import { ElementAst } from '@angular/compiler';
+import { ElementAst, ASTWithSource, LiteralPrimitive } from '@angular/compiler';
+import { PROPERTY } from './isHiddenFromScreenReader';
 
 export const getAttributeValue = (element: ElementAst, property: string) => {
   const attr = element.attrs.find(attr => attr.name === property);
@@ -6,7 +7,13 @@ export const getAttributeValue = (element: ElementAst, property: string) => {
   if (attr) {
     return attr.value;
   }
-  if (input) {
-    return (<any>input.value).ast.value;
+  if (!input || !(input.value instanceof ASTWithSource)) {
+    return undefined;
   }
+
+  if (input.value.ast instanceof LiteralPrimitive) {
+    return input.value.ast.value;
+  }
+
+  return PROPERTY;
 };
diff --git a/src/util/getLiteralValue.ts b/src/util/getLiteralValue.ts
new file mode 100644
index 000000000..1653af21b
--- /dev/null
+++ b/src/util/getLiteralValue.ts
@@ -0,0 +1,8 @@
+export const getLiteralValue = value => {
+  if (value === 'true') {
+    return true;
+  } else if (value === 'false') {
+    return false;
+  }
+  return value;
+};
diff --git a/src/util/isHiddenFromScreenReader.ts b/src/util/isHiddenFromScreenReader.ts
new file mode 100644
index 000000000..347402caa
--- /dev/null
+++ b/src/util/isHiddenFromScreenReader.ts
@@ -0,0 +1,24 @@
+import { ElementAst } from '@angular/compiler';
+import { getAttributeValue } from './getAttributeValue';
+import { getLiteralValue } from './getLiteralValue';
+
+export const PROPERTY = ['PROPERTY'];
+/**
+ * Returns boolean indicating that the aria-hidden prop
+ * is present or the value is true. Will also return true if
+ * there is an input with type='hidden'.
+ *
+ * <div aria-hidden /> is equivalent to the DOM as <div aria-hidden=true />.
+ */
+export const isHiddenFromScreenReader = (el: ElementAst) => {
+  if (el.name.toUpperCase() === 'INPUT') {
+    const hidden = getAttributeValue(el, 'type');
+
+    if (hidden && hidden.toUpperCase() === 'HIDDEN') {
+      return true;
+    }
+  }
+
+  const ariaHidden = getLiteralValue(getAttributeValue(el, 'aria-hidden'));
+  return ariaHidden === PROPERTY || ariaHidden === true;
+};
diff --git a/src/util/isInteractiveElement.ts b/src/util/isInteractiveElement.ts
new file mode 100644
index 000000000..a29d3a45f
--- /dev/null
+++ b/src/util/isInteractiveElement.ts
@@ -0,0 +1,94 @@
+import { dom, elementRoles, roles } from 'aria-query';
+
+import { AXObjects, elementAXObjects } from 'axobject-query';
+
+import { attributesComparator } from './attributesComparator';
+import { ElementAst } from '@angular/compiler';
+
+const domKeys = <string[]>Array.from(dom.keys());
+const roleKeys: any = <string[]>Array.from(roles.keys());
+const elementRoleEntries = Array.from(elementRoles);
+
+const nonInteractiveRoles = new Set(
+  roleKeys.filter(name => {
+    const role = roles.get(name);
+    return !role.abstract && !role.superClass.some(classes => classes.indexOf('widget') !== 0);
+  })
+);
+
+const interactiveRoles: any = new Set(
+  [
+    ...roleKeys,
+    // 'toolbar' does not descend from widget, but it does support
+    // aria-activedescendant, thus in practice we treat it as a widget.
+    'toolbar'
+  ].filter(name => {
+    const role = roles.get(name);
+    return !role.abstract && role.superClass.some(classes => classes.indexOf('widget') !== 0);
+  })
+);
+
+const nonInteractiveElementRoleSchemas = elementRoleEntries.reduce((accumulator: any, [elementSchema, roleSet]: any) => {
+  if (Array.from(roleSet).every((role): boolean => nonInteractiveRoles.has(role))) {
+    accumulator.push(elementSchema);
+  }
+  return accumulator;
+}, []);
+
+const interactiveElementRoleSchemas = elementRoleEntries.reduce((accumulator: any, [elementSchema, roleSet]: any) => {
+  if (Array.from(roleSet).some((role): boolean => interactiveRoles.has(role))) {
+    accumulator.push(elementSchema);
+  }
+  return accumulator;
+}, []);
+
+const interactiveAXObjects = new Set(Array.from(AXObjects.keys()).filter(name => AXObjects.get(name).type === 'widget'));
+
+const interactiveElementAXObjectSchemas = Array.from(elementAXObjects).reduce((accumulator: any, [elementSchema, AXObjectSet]: any) => {
+  if (Array.from(AXObjectSet).every((role): boolean => interactiveAXObjects.has(role))) {
+    accumulator.push(elementSchema);
+  }
+  return accumulator;
+}, []);
+
+function checkIsInteractiveElement(el: ElementAst): boolean {
+  function elementSchemaMatcher(elementSchema) {
+    return el.name === elementSchema.name && attributesComparator(elementSchema.attributes, el);
+  }
+  // Check in elementRoles for inherent interactive role associations for
+  // this element.
+  const isInherentInteractiveElement = interactiveElementRoleSchemas.some(elementSchemaMatcher);
+
+  if (isInherentInteractiveElement) {
+    return true;
+  }
+  // Check in elementRoles for inherent non-interactive role associations for
+  // this element.
+  const isInherentNonInteractiveElement = nonInteractiveElementRoleSchemas.some(elementSchemaMatcher);
+
+  if (isInherentNonInteractiveElement) {
+    return false;
+  }
+  // Check in elementAXObjects for AX Tree associations for this element.
+  const isInteractiveAXElement = interactiveElementAXObjectSchemas.some(elementSchemaMatcher);
+
+  if (isInteractiveAXElement) {
+    return true;
+  }
+
+  return false;
+}
+
+/**
+ * Returns boolean indicating whether the given element is
+ * interactive on the DOM or not. Usually used when an element
+ * has a dynamic handler on it and we need to discern whether or not
+ * it's intention is to be interacted with on the DOM.
+ */
+export const isInteractiveElement = (el: ElementAst): boolean => {
+  if (domKeys.indexOf(el.name) === -1) {
+    return false;
+  }
+
+  return checkIsInteractiveElement(el);
+};
diff --git a/src/util/isPresentationRole.ts b/src/util/isPresentationRole.ts
new file mode 100644
index 000000000..2c50c6111
--- /dev/null
+++ b/src/util/isPresentationRole.ts
@@ -0,0 +1,7 @@
+import { getAttributeValue } from './getAttributeValue';
+import { ElementAst } from '@angular/compiler';
+import { PROPERTY } from './isHiddenFromScreenReader';
+
+const presentationRoles = new Set(['presentation', 'none', PROPERTY]);
+
+export const isPresentationRole = (el: ElementAst) => presentationRoles.has(getAttributeValue(el, 'role'));
diff --git a/test/templateClickEventsHaveKeyEventsRule.spec.ts b/test/templateClickEventsHaveKeyEventsRule.spec.ts
index 544233668..840f9657a 100644
--- a/test/templateClickEventsHaveKeyEventsRule.spec.ts
+++ b/test/templateClickEventsHaveKeyEventsRule.spec.ts
@@ -24,17 +24,194 @@ describe(ruleName, () => {
         source
       });
     });
+
+    it('should fail when click is not accompanied with key events on non interactive element', () => {
+      const source = `
+        @Component({
+          template: \`
+            <header (click)="onClick()"></header>
+            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+          \`
+        })
+        class Bar {}
+      `;
+      assertAnnotated({
+        message: FAILURE_STRING,
+        ruleName,
+        source
+      });
+    });
+
+    it('should fail when click is not accompanied with key events on interactive element without attributes that make them interactive', () => {
+      const source = `
+        @Component({
+          template: \`
+            <a (click)="onClick()"></a>
+            ~~~~~~~~~~~~~~~~~~~~~~~
+          \`
+        })
+        class Bar {}
+      `;
+      assertAnnotated({
+        message: FAILURE_STRING,
+        ruleName,
+        source
+      });
+    });
+
+    it('should fail when click is not accompanied with key events and has aria-hidden attribute as false', () => {
+      const source = `
+        @Component({
+          template: \`
+            <div (click)="onClick()" aria-hidden="false"></div>
+            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+          \`
+        })
+        class Bar {}
+      `;
+      assertAnnotated({
+        message: FAILURE_STRING,
+        ruleName,
+        source
+      });
+    });
+
+    it('should fail when click is not accompanied with key events and has aria-hidden input as false', () => {
+      const source = `
+        @Component({
+          template: \`
+            <div (click)="onClick()" [attr.aria-hidden]="'false'"></div>
+            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+          \`
+        })
+        class Bar {}
+      `;
+      assertAnnotated({
+        message: FAILURE_STRING,
+        ruleName,
+        source
+      });
+    });
+
+    it('should fail when click is not accompanied with key events and has role other than presentational', () => {
+      const source = `
+        @Component({
+          template: \`
+            <div (click)="onClick()" role="header"></div>
+            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+          \`
+        })
+        class Bar {}
+      `;
+      assertAnnotated({
+        message: FAILURE_STRING,
+        ruleName,
+        source
+      });
+    });
+
+    it('should fail when click is not accompanied with key events and has aria-hidden attribute as false', () => {
+      const source = `
+        @Component({
+          template: \`
+            <div (click)="onClick()" role="aside"></div>
+            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+          \`
+        })
+        class Bar {}
+      `;
+      assertAnnotated({
+        message: FAILURE_STRING,
+        ruleName,
+        source
+      });
+    });
+
+    it('should fail when click is not accompanied with key events and has aria-hidden input as false', () => {
+      const source = `
+        @Component({
+          template: \`
+            <div (click)="onClick()" [attr.role]="'header'"></div>
+            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+          \`
+        })
+        class Bar {}
+      `;
+      assertAnnotated({
+        message: FAILURE_STRING,
+        ruleName,
+        source
+      });
+    });
   });
 
   describe('success', () => {
-    it('should work find when click events are associated with key events', () => {
+    it('should work when click events are associated with key events', () => {
+      const source = `
+        @Component({
+          template: \`
+            <div (click)="onClick()" (keyup)="onKeyup()"></div>
+          \`
+        })
+        class Bar {}
+      `;
+      assertSuccess(ruleName, source);
+    });
+
+    it('should work when click events are passed to custom element', () => {
+      const source = `
+        @Component({
+          template: \`
+            <cui-button (click)="onClick()"></cui-button>
+          \`
+        })
+        class Bar {}
+      `;
+      assertSuccess(ruleName, source);
+    });
+
+    it('should work when element has aria-hidden', () => {
+      const source = `
+        @Component({
+          template: \`
+            <div (click)="onClick()" aria-hidden="true"></div>
+            <div (click)="onClick()" [attr.aria-hidden]="true"></div>
+            <div (click)="onClick()" [attr.aria-hidden]="ariaHidden"></div>
+          \`
+        })
+        class Bar {}
+      `;
+      assertSuccess(ruleName, source);
+    });
+
+    it('should work when element has presentation role', () => {
+      const source = `
+        @Component({
+          template: \`
+            <div (click)="onClick()" role="presentation"></div>
+            <div (click)="onClick()" [attr.role]="'none'"></div>
+            <div (click)="onClick()" [attr.role]="roleName"></div>
+          \`
+        })
+        class Bar {}
+      `;
+      assertSuccess(ruleName, source);
+    });
+
+    it('should work when element is interactive', () => {
       const source = `
         @Component({
           template: \`
-          <div (click)="onClick()" (keyup)="onKeyup()"></div>
-          <div (click)="onClick()" (keydown)="onKeyDown()"></div>
-          <div (click)="onClick()" (keypress)="onKeyPress()"></div>
-          <div (click)="onClick()" (keyup)="onKeyup()" (keydown)="onKeyDown()" (keypress)="onKeyPress()"></div>
+            <input (click)="onClick()">
+            <button (click)="onClick()"></button>
+            <textarea (click)="onClick()"></textarea>
+            <select (click)="onClick()">
+              <option (click)="onClick()"></option>
+            </select>
+            <textarea (click)="onClick()"></textarea>
+            <a href="#" (click)="onClick()"></a>
+            <a [attr.href]="href" class="anchor" (click)="onClick()"></a>
+            <a [routerLink]="'route'" (click)="onClick()"></a>
           \`
         })
         class Bar {}
diff --git a/yarn.lock b/yarn.lock
index 31cb05ec5..7f162089b 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -367,6 +367,12 @@ aws4@^1.6.0:
   version "1.7.0"
   resolved "https://registry.yarnpkg.com/aws4/-/aws4-1.7.0.tgz#d4d0e9b9dbfca77bf08eeb0a8a471550fe39e289"
 
+axobject-query@^2.0.2:
+  version "2.0.2"
+  resolved "https://registry.yarnpkg.com/axobject-query/-/axobject-query-2.0.2.tgz#ea187abe5b9002b377f925d8bf7d1c561adf38f9"
+  dependencies:
+    ast-types-flow "0.0.7"
+
 babel-code-frame@^6.22.0:
   version "6.22.0"
   resolved "https://registry.yarnpkg.com/babel-code-frame/-/babel-code-frame-6.22.0.tgz#027620bee567a88c32561574e7fd0801d33118e4"