Skip to content

Commit 246e1c5

Browse files
marcorothkoddssonkhiga8
committed
Linter: Implement html-details-has-summary rule
Co-Authored-By: Kristján Oddsson <hi@koddsson.com> Co-Authored-By: Kate Higa <khiga8@github.com>
1 parent 5978d9f commit 246e1c5

File tree

7 files changed

+264
-3
lines changed

7 files changed

+264
-3
lines changed

‎javascript/packages/linter/docs/rules/README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ This page contains documentation for all Herb Linter rules.
5050
- [`html-attribute-values-require-quotes`](./html-attribute-values-require-quotes.md) - Requires quotes around attribute values
5151
- [`html-avoid-both-disabled-and-aria-disabled`](./html-avoid-both-disabled-and-aria-disabled.md) - Avoid using both `disabled` and `aria-disabled` attributes
5252
- [`html-body-only-elements`](./html-body-only-elements.md) - Require content elements inside `<body>`.
53+
- [`html-details-has-summary`](./html-details-has-summary.md) - Require `<summary>` in `<details>` elements
5354
- [`html-boolean-attributes-no-value`](./html-boolean-attributes-no-value.md) - Prevents values on boolean attributes
5455
- [`html-head-only-elements`](./html-head-only-elements.md) - Require head-scoped elements inside `<head>`.
5556
- [`html-iframe-has-title`](./html-iframe-has-title.md) - `iframe` elements must have a `title` attribute

‎javascript/packages/linter/docs/rules/html-details-has-summary.md‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
# Linter Rule: `<details>` elements must have a `<summary>` child
2+
3+
**Rule:** `html-details-has-summary`
4+
5+
## Description
6+
7+
Ensure that all `<details>` elements have a direct `<summary>` child element that describes what the disclosure widget will expand.
8+
9+
## Rationale
10+
11+
The `<summary>` element provides a visible label for the `<details>` disclosure widget, hinting to the user what they'll be expanding. Screen reader users rely on `<summary>` elements to understand the purpose of the expandable content. If a developer omits the `<summary>`, the user agent adds a default one with no meaningful context. The `<summary>` must be a direct child of `<details>` to function correctly — nesting it inside another element will not work as intended.
12+
13+
## Examples
14+
15+
### ✅ Good
16+
17+
```erb
18+
<details>
19+
<summary>Expand me!</summary>
20+
I do have a summary tag!
21+
</details>
22+
23+
<details>
24+
I do have a summary tag!
25+
<summary>Expand me!</summary>
26+
</details>
27+
```
28+
29+
### 🚫 Bad
30+
31+
```erb
32+
<details>
33+
I don't have a summary tag!
34+
</details>
35+
36+
<details>
37+
<div><summary>Expand me!</summary></div>
38+
The summary tag needs to be a direct child of the details tag.
39+
</details>
40+
```
41+
42+
## References
43+
44+
- [HTML: `details` element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details)
45+
- [HTML: `summary` element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/summary)
46+
- [erblint-github: GitHub::Accessibility::DetailsHasSummary](https://github.com/github/erblint-github/pull/23)

‎javascript/packages/linter/src/rules.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ import { HTMLAttributeEqualsSpacingRule } from "./rules/html-attribute-equals-sp
5151
import { HTMLAttributeValuesRequireQuotesRule } from "./rules/html-attribute-values-require-quotes.js"
5252
import { HTMLAvoidBothDisabledAndAriaDisabledRule } from "./rules/html-avoid-both-disabled-and-aria-disabled.js"
5353
import { HTMLBodyOnlyElementsRule } from "./rules/html-body-only-elements.js"
54+
import { HTMLDetailsHasSummaryRule } from "./rules/html-details-has-summary.js"
5455
import { HTMLBooleanAttributesNoValueRule } from "./rules/html-boolean-attributes-no-value.js"
5556
import { HTMLHeadOnlyElementsRule } from "./rules/html-head-only-elements.js"
5657
import { HTMLIframeHasTitleRule } from "./rules/html-iframe-has-title.js"
@@ -133,6 +134,7 @@ export const rules: RuleClass[] = [
133134
HTMLAttributeValuesRequireQuotesRule,
134135
HTMLAvoidBothDisabledAndAriaDisabledRule,
135136
HTMLBodyOnlyElementsRule,
137+
HTMLDetailsHasSummaryRule,
136138
HTMLBooleanAttributesNoValueRule,
137139
HTMLHeadOnlyElementsRule,
138140
HTMLIframeHasTitleRule,

‎javascript/packages/linter/src/rules/html-details-has-summary.ts‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
import { BaseRuleVisitor } from "./rule-utils.js"
2+
import { getTagLocalName, isHTMLElementNode } from "@herb-tools/core"
3+
4+
import { ParserRule } from "../types.js"
5+
import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js"
6+
import type { HTMLElementNode, ParseResult, ParserOptions } from "@herb-tools/core"
7+
8+
class DetailsHasSummaryVisitor extends BaseRuleVisitor {
9+
visitHTMLElementNode(node: HTMLElementNode): void {
10+
this.checkDetailsElement(node)
11+
super.visitHTMLElementNode(node)
12+
}
13+
14+
private checkDetailsElement(node: HTMLElementNode): void {
15+
const tagName = getTagLocalName(node)
16+
17+
if (tagName !== "details") {
18+
return
19+
}
20+
21+
if (!this.hasDirectSummaryChild(node)) {
22+
this.addOffense(
23+
"`<details>` element must have a direct `<summary>` child element.",
24+
node.location,
25+
)
26+
}
27+
}
28+
29+
private hasDirectSummaryChild(node: HTMLElementNode): boolean {
30+
if (!node.body || node.body.length === 0) {
31+
return false
32+
}
33+
34+
for (const child of node.body) {
35+
if (isHTMLElementNode(child)) {
36+
const childTagName = getTagLocalName(child)
37+
38+
if (childTagName === "summary") {
39+
return true
40+
}
41+
}
42+
}
43+
44+
return false
45+
}
46+
}
47+
48+
export class HTMLDetailsHasSummaryRule extends ParserRule {
49+
static ruleName = "html-details-has-summary"
50+
51+
get defaultConfig(): FullRuleConfig {
52+
return {
53+
enabled: true,
54+
severity: "warning"
55+
}
56+
}
57+
58+
get parserOptions(): Partial<ParserOptions> {
59+
return {
60+
action_view_helpers: true,
61+
}
62+
}
63+
64+
check(result: ParseResult, context?: Partial<LintContext>): UnboundLintOffense[] {
65+
const visitor = new DetailsHasSummaryVisitor(this.ruleName, context)
66+
visitor.visit(result.value)
67+
return visitor.offenses
68+
}
69+
}

‎javascript/packages/linter/src/rules/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ export * from "./html-attribute-equals-spacing.js"
4949
export * from "./html-attribute-values-require-quotes.js"
5050
export * from "./html-avoid-both-disabled-and-aria-disabled.js"
5151
export * from "./html-body-only-elements.js"
52+
export * from "./html-details-has-summary.js"
5253
export * from "./html-boolean-attributes-no-value.js"
5354
export * from "./html-head-only-elements.js"
5455
export * from "./html-iframe-has-title.js"

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

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎javascript/packages/linter/test/rules/html-details-has-summary.test.ts‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
import { describe, test } from "vitest"
2+
import { HTMLDetailsHasSummaryRule } from "../../src/rules/html-details-has-summary.js"
3+
import { createLinterTest } from "../helpers/linter-test-helper.js"
4+
5+
const { expectNoOffenses, expectWarning, assertOffenses } = createLinterTest(HTMLDetailsHasSummaryRule)
6+
7+
describe("html-details-has-summary", () => {
8+
test("passes when details has a direct summary child", () => {
9+
expectNoOffenses('<details><summary>Expand me!</summary><p>Content</p></details>')
10+
})
11+
12+
test("passes when summary is not the first child", () => {
13+
expectNoOffenses('<details><p>Surprise text!</p><summary>Expand me!</summary></details>')
14+
})
15+
16+
test("fails when details has no summary", () => {
17+
expectWarning("`<details>` element must have a direct `<summary>` child element.")
18+
19+
assertOffenses("<details>I don't have a summary element</details>")
20+
})
21+
22+
test("fails when summary is not a direct child of details", () => {
23+
expectWarning("`<details>` element must have a direct `<summary>` child element.")
24+
25+
assertOffenses("<details><div><summary>Expand me!</summary></div></details>")
26+
})
27+
28+
test("ignores non-details elements", () => {
29+
expectNoOffenses('<div><p>No summary needed here</p></div>')
30+
})
31+
32+
test("handles multiple details elements independently", () => {
33+
expectWarning("`<details>` element must have a direct `<summary>` child element.")
34+
35+
assertOffenses('<details><summary>OK</summary></details><details><p>Missing summary</p></details>')
36+
})
37+
38+
test("passes for details with summary and other content", () => {
39+
expectNoOffenses('<details><summary>Expand me!</summary><button>Surprise button!</button></details>')
40+
})
41+
42+
test("fails for empty details element", () => {
43+
expectWarning("`<details>` element must have a direct `<summary>` child element.")
44+
45+
assertOffenses('<details></details>')
46+
})
47+
48+
test("fails when details is rendered with tag.details and no summary", () => {
49+
expectWarning("`<details>` element must have a direct `<summary>` child element.")
50+
51+
assertOffenses(`
52+
<details>
53+
<%= tag.div "Not a summary" %>
54+
</details>
55+
`)
56+
})
57+
58+
test("fails when details has content_tag but not a summary", () => {
59+
expectWarning("`<details>` element must have a direct `<summary>` child element.")
60+
61+
assertOffenses(`
62+
<details>
63+
<%= content_tag(:div) { "Not a summary" } %>
64+
</details>
65+
`)
66+
})
67+
68+
test("fails when details only contains ERB output", () => {
69+
expectWarning("`<details>` element must have a direct `<summary>` child element.")
70+
71+
assertOffenses(`
72+
<details>
73+
<%= tag.p "Some content" %>
74+
</details>
75+
`)
76+
})
77+
78+
test("fails when details only contains content_tag with do block", () => {
79+
expectWarning("`<details>` element must have a direct `<summary>` child element.")
80+
81+
assertOffenses(`
82+
<details>
83+
<%= content_tag(:div) do %>
84+
Not a summary
85+
<% end %>
86+
</details>
87+
`)
88+
})
89+
90+
test("fails when tag.details has no summary", () => {
91+
expectWarning("`<details>` element must have a direct `<summary>` child element.")
92+
93+
assertOffenses(`
94+
<%= tag.details do %>
95+
Some content without summary
96+
<% end %>
97+
`)
98+
})
99+
100+
test("fails when content_tag(:details) has no summary", () => {
101+
expectWarning("`<details>` element must have a direct `<summary>` child element.")
102+
103+
assertOffenses(`<%= content_tag(:details) { "Some content" } %>`)
104+
})
105+
106+
test("fails when content_tag(:details) with do block has no summary", () => {
107+
expectWarning("`<details>` element must have a direct `<summary>` child element.")
108+
109+
assertOffenses(`
110+
<%= content_tag(:details) do %>
111+
Some content without summary
112+
<% end %>
113+
`)
114+
})
115+
116+
test("passes when content_tag(:details) has content_tag(:summary)", () => {
117+
expectNoOffenses(`
118+
<%= content_tag(:details) do %>
119+
<%= content_tag(:summary) { "Expand me!" } %>
120+
Some content
121+
<% end %>
122+
`)
123+
})
124+
125+
test("passes when tag.details has a summary child", () => {
126+
expectNoOffenses(`
127+
<%= tag.details do %>
128+
<summary>Expand me!</summary>
129+
Some content
130+
<% end %>
131+
`)
132+
})
133+
134+
test("passes when tag.details has tag.summary child", () => {
135+
expectNoOffenses(`
136+
<%= tag.details do %>
137+
<%= tag.summary "Expand me!" %>
138+
Some content
139+
<% end %>
140+
`)
141+
})
142+
})

0 commit comments

Comments
 (0)