Extract rule: template-require-input-label#2607
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Review: template-require-input-label
General Correctness
1. The original skips Input and Textarea (capitalized) in strict mode (gjs/gts). The PR does NOT implement this strict-mode skip. In gjs/gts, <Input> and <Textarea> could be user-defined components that happen to share the name, not necessarily the Ember built-in. The original explicitly skips these in strict mode to avoid false positives. The PR should add this check, particularly because it uses templateMode: 'both'.
2. Error messages differ from original. The original uses "form elements require a valid associated label." and "form elements should not have multiple labels." The PR uses "Input elements should have an associated label." and "Input element has multiple labelling mechanisms." These are cosmetic differences but worth noting.
3. hasValidLabelParent uses an elementStack approach instead of the original's path.parents() traversal. The logic is equivalent: walk up the parent chain looking for a label (or custom label tag), and for label specifically, require it to have more than one child (text + input). This is a valid adaptation since ESLint visitors don't have the same path API as ember-template-lint.
4. The original supports regex patterns in labelTags config. The PR only supports string arrays. If users relied on regex patterns (e.g., /^my-label-.*/), this would be a regression. The schema only allows string items, so regex isn't possible. This is probably fine for most use cases but worth documenting.
5. The MustacheStatement handler for curly {{input}}/{{textarea}} is a good addition. The original also handles this. The PR correctly checks for type="hidden" and id pair on these. However, the PR doesn't check for aria-label or aria-labelledby on curly component invocations, while the original also only checks id for MustacheStatement. So this matches.
6. The GlimmerElementNode:exit handler correctly pops the element stack. Good.
7. Missing check: ...attributes spread detection. The PR checks hasAttr(node, '...attributes'), but ...attributes is a splattributes syntax, not a regular attribute name. Verify that node.attributes actually includes an entry with name === '...attributes' in the Glimmer AST. If it uses a different representation, this check might not work. The tests include ...attributes cases that are expected to be valid, so this should be verified by running tests.
Scope Analysis (gjs/gts)
The GlimmerMustacheStatement handler checks node.path?.original for 'input' and 'textarea'. In gjs/gts, these names could be shadowed by local JS imports. For example:
import { input } from 'some-custom-lib';
<template>{{input type="text"}}</template>Here, input refers to the imported value, not Ember's built-in {{input}} helper. The rule should ideally check context.sourceCode.getScope(node) to verify input/textarea aren't locally bound. However, in practice, the curly {{input}} helper is a legacy Ember pattern primarily used in .hbs files, and its usage in gjs/gts is uncommon. The risk of false positives is low but exists.
For the GlimmerElementNode handler, it checks lowercase tag names (input, textarea, select) which are HTML elements and cannot be shadowed. The capitalized variants (Input, Textarea) are component invocations that CAN be shadowed in gjs/gts -- and as noted in point 1, the original explicitly skips these in strict mode.
Summary
Key issues: (1) Missing strict-mode skip for capitalized Input/Textarea components in gjs/gts (behavioral divergence from original that could cause false positives). (2) No regex support in labelTags config (minor). (3) Consider scope analysis for curly {{input}}/{{textarea}} in gjs/gts, though low risk.
🤖 Automated review comparing with ember-template-lint source
4d79927 to
8e893bd
Compare
Split from #2371.