fix: warn when implicit label has no visible text#1864
fix: warn when implicit label has no visible text#1864ShivaniNR wants to merge 2 commits intohtmlhint:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the input-requires-label rule to ensure that labels with nested inputs contain descriptive, non-whitespace text. While the changes introduce a text listener and updated tests, a logic bug was identified where the use of a single boolean flag incorrectly tracks text presence across multiple or nested labels. The reviewer provides actionable feedback and code suggestions to replace this logic with a state stack, ensuring correct validation for each label's scope.
| nested: boolean | ||
| }> = [] | ||
| let labelDepth = 0 | ||
| let labelHasText = false |
There was a problem hiding this comment.
Using a single boolean labelHasText and a global loop over inputTags in the tagend listener is problematic for two reasons:
- False Positives: When an empty label is encountered, the current code resets the
nestedflag for all inputs found so far in the document, even those correctly nested in previous valid labels. - Nested Labels: A nested label will reset the
labelHasTextstate for its parent label, potentially causing the parent label to be treated as empty even if it contains text.
To fix this, use a stack to track the state (text presence and starting input index) for each label depth.
| let labelHasText = false | |
| const labelStateStack: Array<{ hasText: boolean; inputStartIndex: number }> = [] |
There was a problem hiding this comment.
Done. Replaced let labelHasText = false with const labelStateStack: Array<{ hasText: boolean; inputStartIndex: number }> = [] to track state per label depth
| // implicit label (no `for`): nesting labels the input | ||
| // a self-closing <label/> opens no scope and emits no tagend | ||
| labelDepth++ | ||
| labelHasText = false |
There was a problem hiding this comment.
Push the initial state for the new label context onto the stack.
labelStateStack.push({
hasText: false,
inputStartIndex: inputTags.length,
})There was a problem hiding this comment.
Done. Now pushing { hasText: false, inputStartIndex: inputTags.length } on label open instead of resetting a global boolean.
| if (!labelHasText) { | ||
| inputTags.forEach((input) => { | ||
| if (input.nested) { | ||
| input.nested = false | ||
| } | ||
| }) | ||
| } | ||
| labelDepth-- |
There was a problem hiding this comment.
Pop the state for the current label and only reset the nested flag for inputs that were added within this specific label's scope. Also, propagate the text presence to the parent label if applicable.
const state = labelStateStack.pop()
if (state && !state.hasText) {
for (let i = state.inputStartIndex; i < inputTags.length; i++) {
inputTags[i].nested = false
}
} else if (state && state.hasText && labelStateStack.length > 0) {
labelStateStack[labelStateStack.length - 1].hasText = true
}
labelDepth--There was a problem hiding this comment.
Done. The tagend listener now pops the stack, only resets nested for inputs within the current label's scope (from inputStartIndex), and propagates hasText to the parent label. Also added two tests covering the multi-label and nested-label edge cases.
|
|
||
| parser.addListener('text', (event) => { | ||
| if (labelDepth > 0 && event.raw && !/^\s*$/.test(event.raw)) { | ||
| labelHasText = true |
There was a problem hiding this comment.
Update the text presence flag for the current label context in the stack.
| labelHasText = true | |
| labelStateStack[labelStateStack.length - 1].hasText = true |
| const labelTags = []; | ||
| const inputTags = []; | ||
| let labelDepth = 0; | ||
| let labelHasText = false; |
There was a problem hiding this comment.
This compiled file contains the same logic bug as the source. It resets the nested flag for all inputs globally when any label is empty. Use a state stack instead.
| let labelHasText = false; | |
| const labelStateStack = []; |
| } | ||
| else if (!event.close) { | ||
| labelDepth++; | ||
| labelHasText = false; |
There was a problem hiding this comment.
Initialize the state for the current label context.
| labelHasText = false; | |
| labelStateStack.push({ hasText: false, inputStartIndex: inputTags.length }); |
| if (!labelHasText) { | ||
| inputTags.forEach((input) => { | ||
| if (input.nested) { | ||
| input.nested = false; | ||
| } | ||
| }); | ||
| } | ||
| labelDepth--; |
There was a problem hiding this comment.
Only reset inputs belonging to the current label context and propagate text presence to the parent.
const state = labelStateStack.pop();
if (state && !state.hasText) {
for (let i = state.inputStartIndex; i < inputTags.length; i++) {
inputTags[i].nested = false;
}
}
else if (state && state.hasText && labelStateStack.length > 0) {
labelStateStack[labelStateStack.length - 1].hasText = true;
}
labelDepth--;| }); | ||
| parser.addListener('text', (event) => { | ||
| if (labelDepth > 0 && event.raw && !/^\s*$/.test(event.raw)) { | ||
| labelHasText = true; |
There was a problem hiding this comment.
Update the state stack when text is found.
| labelHasText = true; | |
| labelStateStack[labelStateStack.length - 1].hasText = true; |
1 participant
Short description of what this resolves
Closes #1860
An implicit label wrapping an input (
<label><input /></label>) without any visible text is inaccessible; screen readers find the input but have no name to announce. After #1852 added implicit label support, this case was silently accepted.Proposed changes
labelHasTextflag that resets when an implicit<label>openstextevent listener that sets the flag when non-whitespace text is found inside the label</label>close, if no text was found, nested inputs are marked as unlabeled, so the existing warning fires