Linter: Make html-anchor-require-href Action View Helper aware

Author: marcoroth

javascript/packages/linter/docs/rules/html-anchor-require-href.md

@@ -24,6 +24,10 @@ Anchor tags without an `href` attribute are not focusable via keyboard navigatio
 <a>Go to Page</a>
 ```
 
+```erb
+<a href="#">Go to Page</a>
+```
+
 ```erb
 <a data-action="click->doSomething">I'm a fake link</a>
 ```

javascript/packages/linter/src/rules/html-anchor-require-href.ts

@@ -1,28 +1,28 @@
 import { BaseRuleVisitor } from "./rule-utils.js"
-import { getAttribute, getStaticAttributeValue, getTagLocalName } from "@herb-tools/core"
+import { getAttribute, getStaticAttributeValue, getTagLocalName, isERBOpenTagNode, isHTMLOpenTagNode, isRubyLiteralNode, filterHTMLAttributeNodes, findAttributeByName } from "@herb-tools/core"
 
 import { ParserRule } from "../types.js"
 import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js"
-import type { HTMLOpenTagNode, ParseResult } from "@herb-tools/core"
+import type { HTMLElementNode, HTMLAttributeNode, ParseResult, ParserOptions } from "@herb-tools/core"
 
 class AnchorRequireHrefVisitor extends BaseRuleVisitor {
-  visitHTMLOpenTagNode(node: HTMLOpenTagNode): void {
+  visitHTMLElementNode(node: HTMLElementNode): void {
     this.checkATag(node)
-    super.visitHTMLOpenTagNode(node)
+    super.visitHTMLElementNode(node)
   }
 
-  private checkATag(node: HTMLOpenTagNode): void {
+  private checkATag(node: HTMLElementNode): void {
     const tagName = getTagLocalName(node)
 
     if (tagName !== "a") {
       return
     }
 
-    const hrefAttribute = getAttribute(node, "href")
+    const hrefAttribute = this.getHrefAttribute(node)
 
     if (!hrefAttribute) {
       this.addOffense(
-        "Add an `href` attribute to `<a>` to ensure it is focusable and accessible. Links should go somewhere, you probably want to use a `<button>` instead.",
+        "Add an `href` attribute to `<a>` to ensure it is focusable and accessible. Links should navigate somewhere. If you need a clickable element without navigation, use a `<button>` instead.",
         node.tag_name!.location,
       )
 
@@ -33,10 +33,43 @@ class AnchorRequireHrefVisitor extends BaseRuleVisitor {
 
     if (hrefValue === "#") {
       this.addOffense(
-        "Add an `href` attribute to `<a>` to ensure it is focusable and accessible. Links should go somewhere, you probably want to use a `<button>` instead.",
-        node.tag_name!.location,
+        'Avoid `href="#"` on `<a>`. `href="#"` does not navigate anywhere, scrolls the page to the top, and adds `#` to the URL. If you need a clickable element without navigation, use a `<button>` instead.',
+        hrefAttribute.location,
       )
+
+      return
+    }
+
+    if (this.hasNilHrefValue(hrefAttribute)) {
+      this.addOffense(
+        "Avoid passing `nil` as the URL for `link_to`. Links should navigate somewhere. If you need a clickable element without navigation, use a `<button>` instead.",
+        hrefAttribute.location,
+      )
+    }
+  }
+
+  private hasNilHrefValue(hrefAttribute: HTMLAttributeNode): boolean {
+    const valueNode = hrefAttribute.value
+
+    if (!valueNode) return false
+
+    return valueNode.children.some(child =>
+      isRubyLiteralNode(child) && child.content === "url_for(nil)"
+    )
+  }
+
+  private getHrefAttribute(node: HTMLElementNode): HTMLAttributeNode | null {
+    const openTag = node.open_tag
+
+    if (isHTMLOpenTagNode(openTag)) {
+      return getAttribute(openTag, "href")
+    }
+
+    if (isERBOpenTagNode(openTag)) {
+      return findAttributeByName(filterHTMLAttributeNodes(openTag.children), "href")
     }
+
+    return null
   }
 }
 
@@ -50,6 +83,12 @@ export class HTMLAnchorRequireHrefRule extends ParserRule {
     }
   }
 
+  get parserOptions(): Partial<ParserOptions> {
+    return {
+      action_view_helpers: true,
+    }
+  }
+
   check(result: ParseResult, context?: Partial<LintContext>): UnboundLintOffense[] {
     const visitor = new AnchorRequireHrefVisitor(this.ruleName, context)
 

javascript/packages/linter/test/__snapshots__/cli.test.ts.snap

@@ -4,40 +4,42 @@ import { createLinterTest } from "../helpers/linter-test-helper.js"
 
 const { expectNoOffenses, expectError, assertOffenses } = createLinterTest(HTMLAnchorRequireHrefRule)
 
-const MESSAGE = "Add an `href` attribute to `<a>` to ensure it is focusable and accessible. Links should go somewhere, you probably want to use a `<button>` instead."
+const MISSING_HREF_MESSAGE = "Add an `href` attribute to `<a>` to ensure it is focusable and accessible. Links should navigate somewhere. If you need a clickable element without navigation, use a `<button>` instead."
+const HASH_HREF_MESSAGE = 'Avoid `href="#"` on `<a>`. `href="#"` does not navigate anywhere, scrolls the page to the top, and adds `#` to the URL. If you need a clickable element without navigation, use a `<button>` instead.'
+const NIL_HREF_MESSAGE = "Avoid passing `nil` as the URL for `link_to`. Links should navigate somewhere. If you need a clickable element without navigation, use a `<button>` instead."
 
 describe("html-anchor-require-href", () => {
   test("passes for a with href attribute", () => {
     expectNoOffenses('<a href="http://example.com">My link</a>')
   })
 
   test("fails for a without href attribute", () => {
-    expectError(MESSAGE)
+    expectError(MISSING_HREF_MESSAGE)
 
     assertOffenses("<a>My link</a>")
   })
 
   test("fails for multiple a tags without href", () => {
-    expectError(MESSAGE)
-    expectError(MESSAGE)
+    expectError(MISSING_HREF_MESSAGE)
+    expectError(MISSING_HREF_MESSAGE)
 
     assertOffenses("<a>My link</a><a>My other link</a>")
   })
 
   test("fails for a with href='#'", () => {
-    expectError(MESSAGE)
+    expectError(HASH_HREF_MESSAGE)
 
     assertOffenses('<a href="#">My link</a>')
   })
 
   test("fails for a with name attribute and no href", () => {
-    expectError(MESSAGE)
+    expectError(MISSING_HREF_MESSAGE)
 
     assertOffenses('<a name="section1"></a>')
   })
 
   test("fails for a with id attribute and no href", () => {
-    expectError(MESSAGE)
+    expectError(MISSING_HREF_MESSAGE)
 
     assertOffenses('<a id="content">anchor target</a>')
   })
@@ -59,13 +61,13 @@ describe("html-anchor-require-href", () => {
   })
 
   test("fails for a with other attributes but no href", () => {
-    expectError(MESSAGE)
+    expectError(MISSING_HREF_MESSAGE)
 
     assertOffenses('<a class="btn">My link</a>')
   })
 
   test("fails for a with href='#' and other attributes", () => {
-    expectError(MESSAGE)
+    expectError(HASH_HREF_MESSAGE)
 
     assertOffenses('<a href="#" data-action="click->doSomething">My link</a>')
   })
@@ -75,8 +77,46 @@ describe("html-anchor-require-href", () => {
   })
 
   test("handles mixed case a tags", () => {
-    expectError(MESSAGE)
+    expectError(MISSING_HREF_MESSAGE)
 
     assertOffenses("<A>My link</A>")
   })
+
+  test("fails for link_to with href='#'", () => {
+    expectError(HASH_HREF_MESSAGE)
+
+    assertOffenses('<%= link_to "Click me", "#" %>')
+  })
+
+  test("fails for link_to with href='#' and html options", () => {
+    expectError(HASH_HREF_MESSAGE)
+
+    assertOffenses('<%= link_to "Click me", "#", class: "example" %>')
+  })
+
+  test("fails for link_to with href='#' block form", () => {
+    expectError(HASH_HREF_MESSAGE)
+
+    assertOffenses('<%= link_to "#" do %>Click me<% end %>')
+  })
+
+  test("passes for link_to with path helper", () => {
+    expectNoOffenses('<%= link_to "Click me", root_path %>')
+  })
+
+  test("passes for link_to with string url", () => {
+    expectNoOffenses('<%= link_to "Click me", "http://example.com" %>')
+  })
+
+  test("fails for link_to with nil url", () => {
+    expectError(NIL_HREF_MESSAGE)
+
+    assertOffenses('<%= link_to "Profile", nil %>')
+  })
+
+  test("fails for link_to with nil url and html options", () => {
+    expectError(NIL_HREF_MESSAGE)
+
+    assertOffenses('<%= link_to "Profile", nil, class: "btn" %>')
+  })
 })