Repository: ember-template-lint/ember-template-lint
Pull request #628 (2019-01-24 04:34:25)

feat: #625 ([A11Y] no-abstract-roles)


Patch:
diff --git a/docs/rule/no-abstract-roles.md b/docs/rule/no-abstract-roles.md
new file mode 100644
index 00000000..eb3348eb
--- /dev/null
+++ b/docs/rule/no-abstract-roles.md
@@ -0,0 +1,33 @@
+## no-abstract-roles
+
+The HTML attribute `role` must never have the following values:
+
+* `command`
+* `composite`
+* `input`
+* `landmark`
+* `range`
+* `roletype`
+* `section`
+* `sectionhead`
+* `select`
+* `structure`
+* `widget`
+* `window`
+
+### `<* role>`
+
+This rule **forbids** the following:
+
+```hbs
+<div role="window"> Hello, world! </div>
+```
+
+This rule **allows** the following:
+
+```hbs
+<div role="button"> Push it </div>
+```
+
+### References
+* See [https://www.w3.org/TR/wai-aria-1.0/roles#abstract_roles](https://www.w3.org/TR/wai-aria-1.0/roles#abstract_roles)
diff --git a/docs/rule/no-negated-condition.md b/docs/rule/no-negated-condition.md
index 3a67047b..4e629f02 100644
--- a/docs/rule/no-negated-condition.md
+++ b/docs/rule/no-negated-condition.md
@@ -33,6 +33,14 @@ This rule **forbids** the following:
 {{/unless}}
 ```
 
+```hbs
+{{#unless (not condition)}}
+  ...
+{{else}}
+  ...
+{{/unless}}
+```
+
 And similar examples with non-block forms like:
 
 ```hbs
diff --git a/docs/rule/simple-unless.md b/docs/rule/simple-unless.md
index 3c0181ce..b4fd23b4 100644
--- a/docs/rule/simple-unless.md
+++ b/docs/rule/simple-unless.md
@@ -1,22 +1,19 @@
 ## simple-unless
 
-This rule strongly advises against `{{unless}}` blocks in the following situations:
-
-* With other block helpers (e.g. `{{else}}`, `{{else if}}`)
-* With template helpers in the condition
+This rule strongly advises against `{{unless}}` blocks used in conjunction with other block helpers (e.g. `{{else}}`, `{{else if}}`), and template helpers.
 
 Common solutions are to use an `{{if}}` block, or refactor potentially confusing logic within the template.
 
 This rule **forbids** the following:
 
 ``` hbs
-{{! `unless` condition has a template helper }}
+{{! `if` template helper}}
 
 {{unless (if true) "This is not recommended"}}
 ```
 
 ``` hbs
-{{! `unless` statement has an `else` block }}
+{{! `else` block}}
 
 {{#unless bandwagoner}}
   Go Niners!
@@ -25,16 +22,6 @@ This rule **forbids** the following:
 {{/unless}}
 ```
 
-``` hbs
-{{! conditional statement has an `else unless` block }}
-
-{{#if condition}}
-  Hello
-{{else unless otherCondition}}
-  World
-{{/unless}}
-```
-
 This rule **allows** the following:
 
 ``` hbs
@@ -45,12 +32,6 @@ This rule **allows** the following:
 {{/if}}
 ```
 
-``` hbs
-{{#unless condition}}
-  Hello World
-{{/unless}}
-```
-
 ### Configuration
 
 The following values are valid configuration:
diff --git a/docs/rules.md b/docs/rules.md
index 0234df4c..f75ae42b 100644
--- a/docs/rules.md
+++ b/docs/rules.md
@@ -9,6 +9,7 @@
 * [linebreak-style](rule/linebreak-style.md)
 * [link-href-attributes](rule/link-href-attributes.md)
 * [link-rel-noopener](rule/link-rel-noopener.md)
+* [no-abstract-roles](rule/no-abstract-roles.md)
 * [no-action-modifiers](rule/no-action-modifiers.md)
 * [no-attrs-in-components](rule/no-attrs-in-components.md)
 * [no-bare-strings](rule/no-bare-strings.md)
diff --git a/lib/helpers/ast-node-info.js b/lib/helpers/ast-node-info.js
index 504a53e9..2c1aa212 100644
--- a/lib/helpers/ast-node-info.js
+++ b/lib/helpers/ast-node-info.js
@@ -48,14 +48,6 @@ function isBlockStatement(node) {
   return node.type === 'BlockStatement';
 }
 
-function isIf(node) {
-  return node.path && node.path.original === 'if';
-}
-
-function isUnless(node) {
-  return node.path && node.path.original === 'unless';
-}
-
 function hasAttribute(node, attributeName) {
   var attribute = findAttribute(node, attributeName);
 
@@ -106,8 +98,6 @@ module.exports = {
   hasAttribute: hasAttribute,
   hasChildren: hasChildren,
   isBlockStatement: isBlockStatement,
-  isIf: isIf,
-  isUnless: isUnless,
   isCommentStatement: isCommentStatement,
   isConcatStatement: isConcatStatement,
   isMustacheCommentStatement: isMustacheCommentStatement,
diff --git a/lib/rules/index.js b/lib/rules/index.js
index 296afac7..d623335e 100644
--- a/lib/rules/index.js
+++ b/lib/rules/index.js
@@ -13,6 +13,7 @@ module.exports = {
   'linebreak-style': require('./lint-linebreak-style'),
   'link-href-attributes': require('./lint-link-href-attributes'),
   'link-rel-noopener': require('./lint-link-rel-noopener'),
+  'no-abstract-roles': require('./lint-no-abstract-roles'),
   'no-action-modifiers': require('./lint-no-action-modifiers'),
   'no-attrs-in-components': require('./lint-no-attrs-in-components'),
   'no-bare-strings': require('./lint-no-bare-strings'),
diff --git a/lib/rules/lint-no-abstract-roles.js b/lib/rules/lint-no-abstract-roles.js
new file mode 100644
index 00000000..9e046fdc
--- /dev/null
+++ b/lib/rules/lint-no-abstract-roles.js
@@ -0,0 +1,58 @@
+'use strict';
+
+const AstNodeInfo = require('../helpers/ast-node-info');
+const Rule = require('./base');
+
+// From https://www.w3.org/TR/wai-aria-1.0/roles#abstract_roles
+const PROHIBITED_ROLE_VALUES = [
+  'command',
+  'composite',
+  'input',
+  'landmark',
+  'range',
+  'roletype',
+  'section',
+  'sectionhead',
+  'select',
+  'structure',
+  'widget',
+  'window',
+];
+
+function attributeTextValue(attrNode) {
+  if (!attrNode) {
+    return;
+  }
+  if (attrNode.value && attrNode.value.type === 'TextNode') {
+    return attrNode.value.chars;
+  }
+}
+
+module.exports = class NoAbstractRoles extends Rule {
+  logNode({ node, message }) {
+    return this.log({
+      message,
+      line: node.loc && node.loc.start.line,
+      column: node.loc && node.loc.start.column,
+      source: this.sourceForNode(node),
+    });
+  }
+  visitor() {
+    return {
+      ElementNode(node) {
+        const roleAttr = AstNodeInfo.findAttribute(node, 'role');
+        if (roleAttr) {
+          const roleAttrValue = attributeTextValue(roleAttr);
+          if (PROHIBITED_ROLE_VALUES.includes(roleAttrValue)) {
+            this.logNode({
+              message:
+                roleAttrValue +
+                ' is an abstract role, and is not a valid value for the role attribute.',
+              node,
+            });
+          }
+        }
+      },
+    };
+  }
+};
diff --git a/lib/rules/lint-no-negated-condition.js b/lib/rules/lint-no-negated-condition.js
index eec005f2..5091b5cc 100644
--- a/lib/rules/lint-no-negated-condition.js
+++ b/lib/rules/lint-no-negated-condition.js
@@ -10,27 +10,33 @@ const ERROR_MESSAGE_USE_UNLESS = 'Use `unless` instead of `if !condition`.';
 module.exports = class NoNegatedCondition extends Rule {
   visitor() {
     function checkNode(node) {
-      const isIf = AstNodeInfo.isIf(node);
-      const isUnless = AstNodeInfo.isUnless(node);
+      const isIf = node.path && node.path.original === 'if';
+      const isUnless = node.path && node.path.original === 'unless';
       if (!isIf && !isUnless) {
         // Not a conditional statement.
         return;
       }
 
       if (AstNodeInfo.isBlockStatement(node)) {
-        if (this.sourceForNode(node).startsWith('{{else ')) {
-          // We only care about the beginning of the overall `if` / `unless` statement so ignore the `else` parts.
-          return;
+        if (node.inverse && node.inverse.body.length > 0) {
+          // Conditional block statement has an `else` / `else if`
+
+          const firstNode = findFirstNonTextNonCommentNode(node.inverse.body);
+          if (firstNode && AstNodeInfo.isBlockStatement(firstNode)) {
+            // If there's BlockStatement directly inside the body of the `else`,
+            // we need to mark it so we can ignore it later,
+            // to avoid potentially suggesting an `unless` after an `else`.
+            firstNode.isAtBeginningOfElseBody = true;
+
+            if (isIf && firstNode.path.original === 'if') {
+              // Ignore `if ... else if ...` statements as there's nothing wrong with them.
+              return;
+            }
+          }
         }
 
-        if (
-          node.inverse &&
-          node.inverse.body.length > 0 &&
-          AstNodeInfo.isBlockStatement(node.inverse.body[0]) &&
-          isIf &&
-          AstNodeInfo.isIf(node.inverse.body[0])
-        ) {
-          // Ignore `if ... else if ...` statements as there may be no way to avoid negated conditions inside them.
+        if (isIf && node.isAtBeginningOfElseBody) {
+          // Ignore `if` statements directly following an `else` due to complexity.
           return;
         }
       }
@@ -41,7 +47,6 @@ module.exports = class NoNegatedCondition extends Rule {
         !AstNodeInfo.isPathExpression(node.params[0].path) ||
         node.params[0].path.original !== 'not'
       ) {
-        // No negation present.
         return;
       }
 
@@ -78,6 +83,19 @@ module.exports = class NoNegatedCondition extends Rule {
   }
 };
 
+function findFirstNonTextNonCommentNode(nodes) {
+  for (let i = 0; i < nodes.length; i++) {
+    const currentNode = nodes[i];
+    if (
+      !AstNodeInfo.isTextNode(currentNode) &&
+      !AstNodeInfo.isMustacheCommentStatement(currentNode)
+    ) {
+      return currentNode;
+    }
+  }
+  return null;
+}
+
 module.exports.ERROR_MESSAGE_FLIP_IF = ERROR_MESSAGE_FLIP_IF;
 module.exports.ERROR_MESSAGE_USE_IF = ERROR_MESSAGE_USE_IF;
 module.exports.ERROR_MESSAGE_USE_UNLESS = ERROR_MESSAGE_USE_UNLESS;
diff --git a/lib/rules/lint-simple-unless.js b/lib/rules/lint-simple-unless.js
index 6786cdd9..dd8b8997 100644
--- a/lib/rules/lint-simple-unless.js
+++ b/lib/rules/lint-simple-unless.js
@@ -1,7 +1,6 @@
 'use strict';
 
 const Rule = require('./base');
-const AstNodeInfo = require('../helpers/ast-node-info');
 const createErrorMessage = require('../helpers/create-error-message');
 
 const messages = {
@@ -70,19 +69,26 @@ module.exports = class LintSimpleUnless extends Rule {
       },
 
       BlockStatement(node) {
-        const nodeInverse = node.inverse;
+        let nodeInverse = node.inverse;
+        let nodePathOriginal = node.path.original;
 
         if (nodeInverse) {
-          if (AstNodeInfo.isUnless(node)) {
-            if (nodeInverse.body[0] && AstNodeInfo.isIf(nodeInverse.body[0])) {
+          if (nodePathOriginal === 'unless') {
+            if (
+              nodeInverse.body[0] &&
+              nodeInverse.body[0].path &&
+              nodeInverse.body[0].path.original === 'if'
+            ) {
               this._followingElseIfBlock(node);
             } else {
               this._followingElseBlock(node);
             }
-          } else if (this._isElseUnlessBlock(nodeInverse.body[0])) {
-            this._asElseUnlessBlock(node);
+          } else if (nodePathOriginal === 'if' && nodeInverse.body[0]) {
+            if (nodeInverse.body[0].path && nodeInverse.body[0].path.original === 'unless') {
+              this._asElseUnlessBlock(node);
+            }
           }
-        } else if (AstNodeInfo.isUnless(node) && node.params[0].path) {
+        } else if (nodePathOriginal === 'unless' && node.params[0].path) {
           this._withHelper(node);
         }
       },
@@ -155,15 +161,6 @@ module.exports = class LintSimpleUnless extends Rule {
     } while (containsSubexpression);
   }
 
-  _isElseUnlessBlock(node) {
-    return (
-      node &&
-      node.path &&
-      node.path.original === 'unless' &&
-      this.sourceForNode(node).startsWith('{{else ')
-    );
-  }
-
   _logMessage(message, line, column, source) {
     return this.log({
       message,


Test patch:
diff --git a/test/unit/rules/lint-duplicate-attributes-test.js b/test/unit/rules/lint-duplicate-attributes.js
similarity index 100%
rename from test/unit/rules/lint-duplicate-attributes-test.js
rename to test/unit/rules/lint-duplicate-attributes.js
diff --git a/test/unit/rules/lint-no-abstract-roles-test.js b/test/unit/rules/lint-no-abstract-roles-test.js
new file mode 100644
index 00000000..b408b529
--- /dev/null
+++ b/test/unit/rules/lint-no-abstract-roles-test.js
@@ -0,0 +1,153 @@
+'use strict';
+
+const generateRuleTests = require('../../helpers/rule-test-harness');
+
+const errMsg = function(roleName) {
+  return roleName + ' is an abstract role, and is not a valid value for the role attribute.';
+};
+
+generateRuleTests({
+  name: 'no-abstract-roles',
+
+  config: true,
+
+  good: [
+    '<img alt="" role="none" src="zoey.jpg">',
+    '<img alt="" role="presentation" src="zoey.jpg">',
+  ],
+
+  bad: [
+    {
+      template: '<img role="command">',
+
+      result: {
+        message: errMsg('command'),
+        moduleId: 'layout.hbs',
+        source: '<img role="command">',
+        line: 1,
+        column: 0,
+      },
+    },
+    {
+      template: '<img role="composite">',
+
+      result: {
+        message: errMsg('composite'),
+        moduleId: 'layout.hbs',
+        source: '<img role="composite">',
+        line: 1,
+        column: 0,
+      },
+    },
+    {
+      template: '<input role="input">',
+
+      result: {
+        message: errMsg('input'),
+        moduleId: 'layout.hbs',
+        source: '<input role="input">',
+        line: 1,
+        column: 0,
+      },
+    },
+    {
+      template: '<img role="landmark">',
+
+      result: {
+        message: errMsg('landmark'),
+        moduleId: 'layout.hbs',
+        source: '<img role="landmark">',
+        line: 1,
+        column: 0,
+      },
+    },
+    {
+      template: '<input role="range">',
+
+      result: {
+        message: errMsg('range'),
+        moduleId: 'layout.hbs',
+        source: '<input role="range">',
+        line: 1,
+        column: 0,
+      },
+    },
+    {
+      template: '<img role="roletype">',
+
+      result: {
+        message: errMsg('roletype'),
+        moduleId: 'layout.hbs',
+        source: '<img role="roletype">',
+        line: 1,
+        column: 0,
+      },
+    },
+    {
+      template: '<img role="section">',
+
+      result: {
+        message: errMsg('section'),
+        moduleId: 'layout.hbs',
+        source: '<img role="section">',
+        line: 1,
+        column: 0,
+      },
+    },
+    {
+      template: '<img role="sectionhead">',
+
+      result: {
+        message: errMsg('sectionhead'),
+        moduleId: 'layout.hbs',
+        source: '<img role="sectionhead">',
+        line: 1,
+        column: 0,
+      },
+    },
+    {
+      template: '<select role="select"></select>',
+
+      result: {
+        message: errMsg('select'),
+        moduleId: 'layout.hbs',
+        source: '<select role="select"></select>',
+        line: 1,
+        column: 0,
+      },
+    },
+    {
+      template: '<div role="structure"></div>',
+
+      result: {
+        message: errMsg('structure'),
+        moduleId: 'layout.hbs',
+        source: '<div role="structure"></div>',
+        line: 1,
+        column: 0,
+      },
+    },
+    {
+      template: '<img role="widget">',
+
+      result: {
+        message: errMsg('widget'),
+        moduleId: 'layout.hbs',
+        source: '<img role="widget">',
+        line: 1,
+        column: 0,
+      },
+    },
+    {
+      template: '<img role="window">',
+
+      result: {
+        message: errMsg('window'),
+        moduleId: 'layout.hbs',
+        source: '<img role="window">',
+        line: 1,
+        column: 0,
+      },
+    },
+  ],
+});
diff --git a/test/unit/rules/lint-no-negated-condition-test.js b/test/unit/rules/lint-no-negated-condition.js
similarity index 87%
rename from test/unit/rules/lint-no-negated-condition-test.js
rename to test/unit/rules/lint-no-negated-condition.js
index 67f3d969..acaad51a 100644
--- a/test/unit/rules/lint-no-negated-condition-test.js
+++ b/test/unit/rules/lint-no-negated-condition.js
@@ -26,7 +26,6 @@ generateRuleTests({
 
     // if ... else if ...
     '{{#if condition}}<img>{{else if condition}}<img>{{/if}}',
-    '{{#if condition}}<img>{{else if (not condition2)}}<img>{{/if}}', // we ignore `if ... else if ...`
     '{{#if (not condition)}}<img>{{else if (not condition2)}}<img>{{/if}}', // we ignore `if ... else if ...`
 
     // if ... else if ... else ...
@@ -49,6 +48,19 @@ generateRuleTests({
     '{{#unless condition}}<img>{{else if condition}}<img>{{else}}<img>{{/unless}}',
     '{{#unless (or c1 c2)}}<img>{{else if (or c1 c2)}}<img>{{else}}<img>{{/unless}}',
 
+    // This is valid since we don't want to suggest `unless` directly after an `else`:
+    `{{#if condition}}
+     {{else}}
+       {{! some comment }}
+       {{#if (not condition)}}<img>{{/if}}
+     {{/if}}`,
+
+    // This is valid since we can't yet handle `if` statements directly after an `else`:
+    `{{#if (not condition)}}
+     {{else}}
+       {{#if (not condition)}}<img>{{else}}<img>{{/if}}
+     {{/if}}`,
+
     // ******************************************
     // MustacheStatement
     // ******************************************
@@ -82,14 +94,6 @@ generateRuleTests({
     // if ... else ...
     '{{input class=(if condition "some-class" "other-class")}}',
     '{{input class=(if (or c1 c2) "some-class" "other-class")}}',
-
-    // unless ...
-    '{{input class=(unless condition "some-class")}}',
-    '{{input class=(unless (or c1 c2) "some-class")}}',
-
-    // unless ... else ...
-    '{{input class=(unless condition "some-class" "other-class")}}',
-    '{{input class=(unless (or c1 c2) "some-class" "other-class")}}',
   ],
 
   bad: [
@@ -177,33 +181,6 @@ generateRuleTests({
       },
     },
 
-    // Nested inside the body of an `else` block (with preceding comment):
-    {
-      template:
-        '{{#if condition}}{{else}}{{! some comment }}{{#if (not condition)}}<img>{{/if}}{{/if}}',
-
-      result: {
-        message: ERROR_MESSAGE_USE_UNLESS,
-        moduleId: 'layout.hbs',
-        source: '{{#if (not condition)}}<img>{{/if}}',
-        line: 1,
-        column: 44,
-      },
-    },
-
-    // Nested inside the body of an `else` block (without preceding comment):
-    {
-      template: '{{#if condition}}{{else}}{{#if (not condition)}}<img>{{/if}}{{/if}}',
-
-      result: {
-        message: ERROR_MESSAGE_USE_UNLESS,
-        moduleId: 'layout.hbs',
-        source: '{{#if (not condition)}}<img>{{/if}}',
-        line: 1,
-        column: 25,
-      },
-    },
-
     // ******************************************
     // MustacheStatement
     // ******************************************
diff --git a/test/unit/rules/lint-simple-unless-test.js b/test/unit/rules/lint-simple-unless-test.js
index bb3a9078..e5d84ac0 100644
--- a/test/unit/rules/lint-simple-unless-test.js
+++ b/test/unit/rules/lint-simple-unless-test.js
@@ -16,8 +16,6 @@ generateRuleTests({
     '<div class="{{if foo \'foo\'}}"></div>',
     '{{unrelated-mustache-without-params}}',
     '{{#if foo}}{{else}}{{/if}}',
-    '{{#if foo}}{{else}}{{#unless bar}}{{/unless}}{{/if}}',
-    '{{#if foo}}{{else}}{{unless bar someProperty}}{{/if}}',
     '{{#unless (or foo bar)}}order whiskey{{/unless}}',
     '{{#unless (eq (or foo bar) baz)}}order whiskey{{/unless}}',
     ['{{#unless hamburger}}', '  HOT DOG!', '{{/unless}}'].join('\n'),