From 246e1c5781130a6186d03e4ffff5e8e23d773cd7 Mon Sep 17 00:00:00 2001 From: Marco Roth Date: Thu, 12 Mar 2026 01:33:18 +0100 Subject: [PATCH] Linter: Implement `html-details-has-summary` rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Kristján Oddsson Co-Authored-By: Kate Higa --- .../packages/linter/docs/rules/README.md | 1 + .../docs/rules/html-details-has-summary.md | 46 ++++++ javascript/packages/linter/src/rules.ts | 2 + .../src/rules/html-details-has-summary.ts | 69 +++++++++ javascript/packages/linter/src/rules/index.ts | 1 + .../test/__snapshots__/cli.test.ts.snap | 6 +- .../rules/html-details-has-summary.test.ts | 142 ++++++++++++++++++ 7 files changed, 264 insertions(+), 3 deletions(-) create mode 100644 javascript/packages/linter/docs/rules/html-details-has-summary.md create mode 100644 javascript/packages/linter/src/rules/html-details-has-summary.ts create mode 100644 javascript/packages/linter/test/rules/html-details-has-summary.test.ts diff --git a/javascript/packages/linter/docs/rules/README.md b/javascript/packages/linter/docs/rules/README.md index d16440e79..f45f963bd 100644 --- a/javascript/packages/linter/docs/rules/README.md +++ b/javascript/packages/linter/docs/rules/README.md @@ -50,6 +50,7 @@ This page contains documentation for all Herb Linter rules. - [`html-attribute-values-require-quotes`](./html-attribute-values-require-quotes.md) - Requires quotes around attribute values - [`html-avoid-both-disabled-and-aria-disabled`](./html-avoid-both-disabled-and-aria-disabled.md) - Avoid using both `disabled` and `aria-disabled` attributes - [`html-body-only-elements`](./html-body-only-elements.md) - Require content elements inside ``. +- [`html-details-has-summary`](./html-details-has-summary.md) - Require `` in `
` elements - [`html-boolean-attributes-no-value`](./html-boolean-attributes-no-value.md) - Prevents values on boolean attributes - [`html-head-only-elements`](./html-head-only-elements.md) - Require head-scoped elements inside ``. - [`html-iframe-has-title`](./html-iframe-has-title.md) - `iframe` elements must have a `title` attribute diff --git a/javascript/packages/linter/docs/rules/html-details-has-summary.md b/javascript/packages/linter/docs/rules/html-details-has-summary.md new file mode 100644 index 000000000..a69f99ccb --- /dev/null +++ b/javascript/packages/linter/docs/rules/html-details-has-summary.md @@ -0,0 +1,46 @@ +# Linter Rule: `
` elements must have a `` child + +**Rule:** `html-details-has-summary` + +## Description + +Ensure that all `
` elements have a direct `` child element that describes what the disclosure widget will expand. + +## Rationale + +The `` element provides a visible label for the `
` disclosure widget, hinting to the user what they'll be expanding. Screen reader users rely on `` elements to understand the purpose of the expandable content. If a developer omits the ``, the user agent adds a default one with no meaningful context. The `` must be a direct child of `
` to function correctly — nesting it inside another element will not work as intended. + +## Examples + +### ✅ Good + +```erb +
+ Expand me! + I do have a summary tag! +
+ +
+ I do have a summary tag! + Expand me! +
+``` + +### 🚫 Bad + +```erb +
+ I don't have a summary tag! +
+ +
+
Expand me!
+ The summary tag needs to be a direct child of the details tag. +
+``` + +## References + +- [HTML: `details` element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details) +- [HTML: `summary` element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/summary) +- [erblint-github: GitHub::Accessibility::DetailsHasSummary](https://github.com/github/erblint-github/pull/23) diff --git a/javascript/packages/linter/src/rules.ts b/javascript/packages/linter/src/rules.ts index ad415815a..eece3b463 100644 --- a/javascript/packages/linter/src/rules.ts +++ b/javascript/packages/linter/src/rules.ts @@ -51,6 +51,7 @@ import { HTMLAttributeEqualsSpacingRule } from "./rules/html-attribute-equals-sp import { HTMLAttributeValuesRequireQuotesRule } from "./rules/html-attribute-values-require-quotes.js" import { HTMLAvoidBothDisabledAndAriaDisabledRule } from "./rules/html-avoid-both-disabled-and-aria-disabled.js" import { HTMLBodyOnlyElementsRule } from "./rules/html-body-only-elements.js" +import { HTMLDetailsHasSummaryRule } from "./rules/html-details-has-summary.js" import { HTMLBooleanAttributesNoValueRule } from "./rules/html-boolean-attributes-no-value.js" import { HTMLHeadOnlyElementsRule } from "./rules/html-head-only-elements.js" import { HTMLIframeHasTitleRule } from "./rules/html-iframe-has-title.js" @@ -133,6 +134,7 @@ export const rules: RuleClass[] = [ HTMLAttributeValuesRequireQuotesRule, HTMLAvoidBothDisabledAndAriaDisabledRule, HTMLBodyOnlyElementsRule, + HTMLDetailsHasSummaryRule, HTMLBooleanAttributesNoValueRule, HTMLHeadOnlyElementsRule, HTMLIframeHasTitleRule, diff --git a/javascript/packages/linter/src/rules/html-details-has-summary.ts b/javascript/packages/linter/src/rules/html-details-has-summary.ts new file mode 100644 index 000000000..b725adf0c --- /dev/null +++ b/javascript/packages/linter/src/rules/html-details-has-summary.ts @@ -0,0 +1,69 @@ +import { BaseRuleVisitor } from "./rule-utils.js" +import { getTagLocalName, isHTMLElementNode } from "@herb-tools/core" + +import { ParserRule } from "../types.js" +import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js" +import type { HTMLElementNode, ParseResult, ParserOptions } from "@herb-tools/core" + +class DetailsHasSummaryVisitor extends BaseRuleVisitor { + visitHTMLElementNode(node: HTMLElementNode): void { + this.checkDetailsElement(node) + super.visitHTMLElementNode(node) + } + + private checkDetailsElement(node: HTMLElementNode): void { + const tagName = getTagLocalName(node) + + if (tagName !== "details") { + return + } + + if (!this.hasDirectSummaryChild(node)) { + this.addOffense( + "`
` element must have a direct `` child element.", + node.location, + ) + } + } + + private hasDirectSummaryChild(node: HTMLElementNode): boolean { + if (!node.body || node.body.length === 0) { + return false + } + + for (const child of node.body) { + if (isHTMLElementNode(child)) { + const childTagName = getTagLocalName(child) + + if (childTagName === "summary") { + return true + } + } + } + + return false + } +} + +export class HTMLDetailsHasSummaryRule extends ParserRule { + static ruleName = "html-details-has-summary" + + get defaultConfig(): FullRuleConfig { + return { + enabled: true, + severity: "warning" + } + } + + get parserOptions(): Partial { + return { + action_view_helpers: true, + } + } + + check(result: ParseResult, context?: Partial): UnboundLintOffense[] { + const visitor = new DetailsHasSummaryVisitor(this.ruleName, context) + visitor.visit(result.value) + return visitor.offenses + } +} diff --git a/javascript/packages/linter/src/rules/index.ts b/javascript/packages/linter/src/rules/index.ts index 2dbfaf59c..e713701d9 100644 --- a/javascript/packages/linter/src/rules/index.ts +++ b/javascript/packages/linter/src/rules/index.ts @@ -49,6 +49,7 @@ export * from "./html-attribute-equals-spacing.js" export * from "./html-attribute-values-require-quotes.js" export * from "./html-avoid-both-disabled-and-aria-disabled.js" export * from "./html-body-only-elements.js" +export * from "./html-details-has-summary.js" export * from "./html-boolean-attributes-no-value.js" export * from "./html-head-only-elements.js" export * from "./html-iframe-has-title.js" diff --git a/javascript/packages/linter/test/__snapshots__/cli.test.ts.snap b/javascript/packages/linter/test/__snapshots__/cli.test.ts.snap index 49749b298..b92e00ec2 100644 --- a/javascript/packages/linter/test/__snapshots__/cli.test.ts.snap +++ b/javascript/packages/linter/test/__snapshots__/cli.test.ts.snap @@ -787,7 +787,7 @@ exports[`CLI Output Formatting > formats JSON output correctly for bad file 1`] "summary": { "filesChecked": 1, "filesWithOffenses": 1, - "ruleCount": 69, + "ruleCount": 70, "totalErrors": 2, "totalHints": 0, "totalIgnored": 0, @@ -808,7 +808,7 @@ exports[`CLI Output Formatting > formats JSON output correctly for clean file 1` "summary": { "filesChecked": 1, "filesWithOffenses": 0, - "ruleCount": 69, + "ruleCount": 70, "totalErrors": 0, "totalHints": 0, "totalIgnored": 0, @@ -881,7 +881,7 @@ exports[`CLI Output Formatting > formats JSON output correctly for file with err "summary": { "filesChecked": 1, "filesWithOffenses": 1, - "ruleCount": 69, + "ruleCount": 70, "totalErrors": 2, "totalHints": 0, "totalIgnored": 0, diff --git a/javascript/packages/linter/test/rules/html-details-has-summary.test.ts b/javascript/packages/linter/test/rules/html-details-has-summary.test.ts new file mode 100644 index 000000000..275a868eb --- /dev/null +++ b/javascript/packages/linter/test/rules/html-details-has-summary.test.ts @@ -0,0 +1,142 @@ +import { describe, test } from "vitest" +import { HTMLDetailsHasSummaryRule } from "../../src/rules/html-details-has-summary.js" +import { createLinterTest } from "../helpers/linter-test-helper.js" + +const { expectNoOffenses, expectWarning, assertOffenses } = createLinterTest(HTMLDetailsHasSummaryRule) + +describe("html-details-has-summary", () => { + test("passes when details has a direct summary child", () => { + expectNoOffenses('
Expand me!

Content

') + }) + + test("passes when summary is not the first child", () => { + expectNoOffenses('

Surprise text!

Expand me!
') + }) + + test("fails when details has no summary", () => { + expectWarning("`
` element must have a direct `` child element.") + + assertOffenses("
I don't have a summary element
") + }) + + test("fails when summary is not a direct child of details", () => { + expectWarning("`
` element must have a direct `` child element.") + + assertOffenses("
Expand me!
") + }) + + test("ignores non-details elements", () => { + expectNoOffenses('

No summary needed here

') + }) + + test("handles multiple details elements independently", () => { + expectWarning("`
` element must have a direct `` child element.") + + assertOffenses('
OK

Missing summary

') + }) + + test("passes for details with summary and other content", () => { + expectNoOffenses('
Expand me!
') + }) + + test("fails for empty details element", () => { + expectWarning("`
` element must have a direct `` child element.") + + assertOffenses('
') + }) + + test("fails when details is rendered with tag.details and no summary", () => { + expectWarning("`
` element must have a direct `` child element.") + + assertOffenses(` +
+ <%= tag.div "Not a summary" %> +
+ `) + }) + + test("fails when details has content_tag but not a summary", () => { + expectWarning("`
` element must have a direct `` child element.") + + assertOffenses(` +
+ <%= content_tag(:div) { "Not a summary" } %> +
+ `) + }) + + test("fails when details only contains ERB output", () => { + expectWarning("`
` element must have a direct `` child element.") + + assertOffenses(` +
+ <%= tag.p "Some content" %> +
+ `) + }) + + test("fails when details only contains content_tag with do block", () => { + expectWarning("`
` element must have a direct `` child element.") + + assertOffenses(` +
+ <%= content_tag(:div) do %> + Not a summary + <% end %> +
+ `) + }) + + test("fails when tag.details has no summary", () => { + expectWarning("`
` element must have a direct `` child element.") + + assertOffenses(` + <%= tag.details do %> + Some content without summary + <% end %> + `) + }) + + test("fails when content_tag(:details) has no summary", () => { + expectWarning("`
` element must have a direct `` child element.") + + assertOffenses(`<%= content_tag(:details) { "Some content" } %>`) + }) + + test("fails when content_tag(:details) with do block has no summary", () => { + expectWarning("`
` element must have a direct `` child element.") + + assertOffenses(` + <%= content_tag(:details) do %> + Some content without summary + <% end %> + `) + }) + + test("passes when content_tag(:details) has content_tag(:summary)", () => { + expectNoOffenses(` + <%= content_tag(:details) do %> + <%= content_tag(:summary) { "Expand me!" } %> + Some content + <% end %> + `) + }) + + test("passes when tag.details has a summary child", () => { + expectNoOffenses(` + <%= tag.details do %> + Expand me! + Some content + <% end %> + `) + }) + + test("passes when tag.details has tag.summary child", () => { + expectNoOffenses(` + <%= tag.details do %> + <%= tag.summary "Expand me!" %> + Some content + <% end %> + `) + }) +})