Skip to content

Extract rule: template-require-iframe-title#2606

Merged
NullVoxPopuli merged 1 commit intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-require-iframe-title
Mar 20, 2026
Merged

Extract rule: template-require-iframe-title#2606
NullVoxPopuli merged 1 commit intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-require-iframe-title

Conversation

@NullVoxPopuli
Copy link
Contributor

Split from #2371.

Copy link

@NullVoxPopuli-ai-agent NullVoxPopuli-ai-agent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: template-require-iframe-title

General Correctness

1. Duplicate title detection differs from original. The original reports BOTH the original iframe (with a "This title is not unique" message) and the duplicate iframe (with a more detailed message including the duplicate value). The PR only reports the second (duplicate) iframe, not the first. This means the first iframe with a duplicated title never gets flagged, which diverges from the original behavior. The original also includes numbered references (#1, #2) in its messages.

2. Error messages simplified. The original has distinct messages for duplicates ("This title is not unique. #N" for the first, and a detailed message for the duplicate). The PR uses the same missingTitle messageId for all cases. This loses context -- the user won't know if the issue is a missing title, empty title, or duplicate title. Consider adding separate messageIds like duplicateTitle.

3. The knownTitles array is instance-level state in the create() closure. This works for a single file, but note that the original uses this.knownTitles = [] in the visitor() method, resetting per-file. The PR's approach of declaring knownTitles in the create() function achieves the same per-file scoping since create() is called per file. Correct.

4. aria-hidden value check. The original checks for the presence of the aria-hidden attribute (any value). The PR does the same with node.attributes?.some((a) => a.name === 'aria-hidden'). However, technically aria-hidden="false" means the element is NOT hidden. Both the original and the PR treat any aria-hidden attribute as hidden regardless of value. This is a pre-existing issue in the original, so matching is fine.

5. Tests are solid with good coverage including duplicate titles, boolean false values, empty strings, and hidden iframes.

Scope Analysis (gjs/gts)

This rule only checks GlimmerElementNode for node.tag === 'iframe' -- HTML element inspection. No scope analysis needed.

Summary

The main concern is the divergent behavior for duplicate title detection -- the original flags both the first and second iframe when a title is duplicated, while the PR only flags the second. Also, the single missingTitle message is used for all error conditions (missing, empty, duplicate, boolean false), losing diagnostic specificity.

🤖 Automated review comparing with ember-template-lint source

@NullVoxPopuli NullVoxPopuli force-pushed the nvp/template-lint-extract-rule-template-require-iframe-title branch from bf38786 to 199794d Compare March 20, 2026 14:21
@NullVoxPopuli NullVoxPopuli merged commit 096d133 into ember-cli:master Mar 20, 2026
11 checks passed
@NullVoxPopuli NullVoxPopuli deleted the nvp/template-lint-extract-rule-template-require-iframe-title branch March 20, 2026 15:47
@github-actions github-actions bot mentioned this pull request Mar 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants