Conversation
🏎️ Benchmark Comparison
Full mitata output |
There was a problem hiding this comment.
Pull request overview
Adds a new template accessibility rule to disallow placeholder / non-navigational href values on <a> elements, complementing the existing template-link-href-attributes (presence-only) rule.
Changes:
- Implement
template-no-invalid-link-hrefto flag empty/whitespacehref, bare#/#!, andjavascript:URLs (static-only; skips dynamic values). - Add RuleTester coverage for both GJS/GTS (
<template>) and HBS parsing modes. - Add rule documentation and list the rule in the README rule table; add a peer-parity audit fixture.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/rules/template-no-invalid-link-href.js |
New rule implementation for detecting placeholder/unsafe static href values on <a>. |
tests/lib/rules/template-no-invalid-link-href.js |
Unit tests covering valid/invalid static href cases plus dynamic-skip behavior. |
tests/audit/anchor-is-valid-href-only/peer-parity.js |
Audit fixture translating peer-plugin cases for behavioral comparison. |
docs/rules/template-no-invalid-link-href.md |
Rule documentation with examples and references. |
README.md |
Adds the rule to the documented rule list/table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
johanrd
force-pushed
the
feat/template-no-invalid-link-href
branch
2 times, most recently
from
5ed004c to
e5aa482
Compare
johanrd
force-pushed
the
feat/template-no-invalid-link-href
branch
from
cd248b9 to
d2c4dad
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'use strict'; | ||
|
|
||
| const { getStaticAttrValue } = require('../../../lib/utils/static-attr-value'); | ||
|
|
||
| describe('getStaticAttrValue', () => { | ||
| it('returns empty string for null/undefined (valueless attribute)', () => { | ||
| expect(getStaticAttrValue(null)).toBe(''); | ||
| expect(getStaticAttrValue(undefined)).toBe(''); |
There was a problem hiding this comment.
This test file relies on a global expect without importing it. If the repo’s test runner doesn’t provide Jest/Vitest-style globals, this will fail at runtime. Prefer using Node’s built-in assert (or the repo’s standard assertion library) so the test is self-contained and consistent with the rest of the suite.
| return extractLiteral(value.path); | ||
| } | ||
| if (value.type === 'GlimmerConcatStatement') { | ||
| const parts = value.parts || []; |
There was a problem hiding this comment.
Defaulting value.parts to an empty array means a GlimmerConcatStatement with missing/invalid parts will resolve to the empty string ('') instead of undefined. That can cause callers to treat an otherwise-unresolvable value as statically known and (for href validation) potentially flag it as an empty href. Consider returning undefined when parts is not an array (e.g. !Array.isArray(value.parts)), and only joining when parts is a valid array.
| const parts = value.parts || []; | |
| if (!Array.isArray(value.parts)) { | |
| return undefined; | |
| } | |
| const parts = value.parts; |
|
|
||
| <!-- end auto-generated rule header --> | ||
|
|
||
| Disallow link elements — `<a>` and `<area>` — whose `href` value is a commonly-misused placeholder (e.g. `href="#"`, `href=""`, `href="javascript:..."`). Both carry URL semantics per HTML §4.5.1 / §4.8.14, so the same validity rules apply on each. |
There was a problem hiding this comment.
The doc references specific HTML section numbers ("HTML §4.5.1 / §4.8.14"), which don’t align well with the Living Standard’s current structure and can become stale/misleading. Prefer linking directly to the relevant Living Standard anchors for both elements (e.g. the <a> element and the <area> element sections) and drop the numeric section references.
| | [template-no-heading-inside-button](docs/rules/template-no-heading-inside-button.md) | disallow heading elements inside button elements | 📋 | | | | ||
| | [template-no-invalid-aria-attributes](docs/rules/template-no-invalid-aria-attributes.md) | disallow invalid aria-* attributes | 📋 | | | | ||
| | [template-no-invalid-interactive](docs/rules/template-no-invalid-interactive.md) | disallow non-interactive elements with interactive handlers | 📋 | | | | ||
| | [template-no-invalid-link-href](docs/rules/template-no-invalid-link-href.md) | disallow invalid href values on anchor elements | | | | |
There was a problem hiding this comment.
The README description says "anchor elements", but the rule (and its docs/tests) also applies to <area>. Update the wording to reflect both, e.g. "disallow invalid href values on <a>/<area> elements" (or "link elements").
| | [template-no-invalid-link-href](docs/rules/template-no-invalid-link-href.md) | disallow invalid href values on anchor elements | | | | | |
| | [template-no-invalid-link-href](docs/rules/template-no-invalid-link-href.md) | disallow invalid href values on `<a>`/`<area>` elements | | | | |
…clarify README to include <area>
…stable anchor URLs
Note
This is part of a series where Claude has audited
eslint-plugin-emberagainst jsx-a11y, vuejs-accessibility, angular-eslint, lit-a11y and html-validate,ember-template-lint, and the HTML and WCAG specs.hrefon<a>/<area>to have "a value that is a valid URL potentially surrounded by spaces." Placeholders likehref="#",href="", andhref="javascript:..."parse as valid URLs (so they don't violate the HTML spec per se) but fake a clickable link at the expense of keyboard/screen-reader semantics — the element announces as a link but doesn't navigate. This is an accessibility and UX anti-pattern documented by MDN ("Anchor elements are often abused as fake buttons by setting their href to#orjavascript:void(0)... These bogus href values cause unexpected behavior... Use a<button>instead.").template-link-href-attributesonly checks for the presence ofhref. It acceptshref="#"andhref="javascript:void(0)"as valid.The rule's premise is a pragmatic a11y/UX check backed by MDN guidance and peer-plugin prior art (jsx-a11y, lit-a11y), not a literal HTML-spec violation.
Fix: add
template-no-invalid-link-href. Validates the href VALUE, complementing the existing presence check.Flags
Allows
Prior art
Verified each peer in source:
anchor-is-validhref="",href="#",href="javascript:void(0)". Tests atanchor-is-valid-test.js:286-289.anchor-is-validallowHashdefaults totrue— does NOT flaghref="#"by default.