fix(forms): improve aria attribute management#128
Open
coryrylan wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughForms state controllers now explicitly clear or set internals aria values (null or string), TypeSubmitController replaces the native-button controller and is wired into button mixin/tests, FormControlInstance is exported and validators/mixins updated to use it, and tests for form behavior and defaults are added or adjusted. Core internal barrel exports and an InterestEvent type are updated. ChangesState Controllers and Form Behavior
Core exports and types
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.4.16)projects/internals/metadata/static/tests.jsonFile contains syntax errors that prevent linting: Line 1: String values must be double quoted.; Line 1: String values must be double quoted.; Line 1: End of file expected; Line 2: End of file expected; Line 2: String values must be double quoted.; Line 3: String values must be double quoted.; Line 3: End of file expected Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/forms/src/mixins/control.ts (1)
443-449:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBreaking change: valueAsNumber now warns instead of throwing.
The
valueAsNumbersetter behavior changed from throwing aFormControlErrorto emitting aconsole.warnwhen attempting to set a number value on a non-number-typed control. This reduces strictness and could mask programming errors where code incorrectly attempts numeric assignment on string-typed controls.Additionally, the JSDoc comment for
valueAsNumber(lines 156-159) does not document this warning behavior or the conditions under which it occurs.📝 Suggested JSDoc enhancement
/** * The current value parsed as a number. + * + * `@remarks` + * Setting this property on a non-number-typed control will log a warning and no-op. */ valueAsNumber: number;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/forms/src/mixins/control.ts` around lines 443 - 449, The setter valueAsNumber in the FormControl mixin (method: set valueAsNumber) currently calls console.warn on type mismatch; revert it to throw a FormControlError when attempting to set a numeric value on a non-number-typed control to restore previous strict behavior, and update the JSDoc for valueAsNumber to document that it throws FormControlError when the underlying generic type is not number and describe the exact condition checked (typeof this._value !== 'number' || typeof value !== 'number'). Locate set valueAsNumber and the JSDoc block for valueAsNumber and replace the console.warn path with: throw new FormControlError(this.localName, 'cannot set number value on non-number type'), and expand the JSDoc to state the throwing behavior and the condition under which it will throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/forms/src/mixins/slider.test.ts`:
- Around line 72-97: The test's fixture cleanup can leak when an assertion fails
because removeFixture(customFixture) is only called at the end; wrap the async
test body so that after awaiting createFixture(...) and assigning customFixture
you run your assertions inside a try block and call removeFixture(customFixture)
in a finally block (so the fixture is always removed). Locate the test named
"should support custom slider defaults" and modify the structure around
createFixture, customFixture, and removeFixture to ensure
removeFixture(customFixture) executes in finally; keep existing assertions and
operations on customElement (min, max, step, value, getInternals checks,
FormData check, valueAsNumber change and formResetCallback) inside the try.
- Around line 23-40: CustomSliderDefaultsTestElement (which extends
SliderFormControlMixin<typeof HTMLElement>(HTMLElement)) is missing a
requestUpdate method stub like SliderTestElement has, so any mixin calls to
requestUpdate (e.g., when setting valueAsNumber) can throw; add a minimal
requestUpdate(): Promise<void> stub to CustomSliderDefaultsTestElement that
returns Promise.resolve() (or mirrors the signature used by the mixin) so the
mixin’s calls to requestUpdate succeed during tests.
---
Outside diff comments:
In `@projects/forms/src/mixins/control.ts`:
- Around line 443-449: The setter valueAsNumber in the FormControl mixin
(method: set valueAsNumber) currently calls console.warn on type mismatch;
revert it to throw a FormControlError when attempting to set a numeric value on
a non-number-typed control to restore previous strict behavior, and update the
JSDoc for valueAsNumber to document that it throws FormControlError when the
underlying generic type is not number and describe the exact condition checked
(typeof this._value !== 'number' || typeof value !== 'number'). Locate set
valueAsNumber and the JSDoc block for valueAsNumber and replace the console.warn
path with: throw new FormControlError(this.localName, 'cannot set number value
on non-number type'), and expand the JSDoc to state the throwing behavior and
the condition under which it will throw.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 79040383-af17-4367-84a4-e197e77912eb
📒 Files selected for processing (15)
projects/forms/src/internal/controllers/state-current.controller.test.tsprojects/forms/src/internal/controllers/state-current.controller.tsprojects/forms/src/internal/controllers/state-expanded.controller.test.tsprojects/forms/src/internal/controllers/state-expanded.controller.tsprojects/forms/src/internal/controllers/state-pressed.controller.test.tsprojects/forms/src/internal/controllers/state-pressed.controller.tsprojects/forms/src/internal/controllers/state-selected.controller.test.tsprojects/forms/src/internal/controllers/state-selected.controller.tsprojects/forms/src/internal/types.tsprojects/forms/src/mixins/checkbox.test.tsprojects/forms/src/mixins/control.test.tsprojects/forms/src/mixins/control.tsprojects/forms/src/mixins/slider.test.tsprojects/forms/src/validators/index.test.tsprojects/forms/src/validators/index.ts
- Modified state controllers to correctly manage aria attributes when values are null or undefined. - Improved test cases for state controllers to ensure proper aria attribute handling. - Introduced custom slider defaults in tests to validate default behavior. Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-cleanup-forms
branch
from
d03d352 to
4e13663
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/forms/src/internal/controllers/type-submit.controller.ts (1)
59-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock submit/reset when host is read-only in the click path.
hostUpdated()detaches listeners forreadOnly, but#onSubmitClickonly guardsdisabled. During update lag, a read-only host can still submit/reset. Add a directreadOnlyguard here too.Suggested patch
`#onSubmitClick` = (event: Event) => { - if (this.host.disabled) { + if (this.host.disabled || this.host.readOnly) { stopEvent(event); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/forms/src/internal/controllers/type-submit.controller.ts` around lines 59 - 69, The click handler `#onSubmitClick` only checks this.host.disabled but not this.host.readOnly, allowing submits/resets during update lag; add a guard at the start of `#onSubmitClick` to treat readOnly like disabled (call stopEvent(event) and return) before proceeding to call `#requestSubmit`(this.host.form) or this.host.form.reset() so read-only hosts cannot submit or reset.projects/forms/src/internal/controllers/type-submit.controller.test.ts (1)
62-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
elementIsStableafter fixture setup and state changes to avoid flaky timing.These tests use
createFixturebut skip theelementIsStablepattern required for unit tests, which can make listener/state assertions race-prone.As per coding guidelines, "
**/*.test.ts: Follow unit testing patterns from/projects/site/src/docs/internal/guidelines/testing-unit.mdincluding createFixture and elementIsStable patterns".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/forms/src/internal/controllers/type-submit.controller.test.ts` around lines 62 - 159, Tests use createFixture but do not await elementIsStable, causing race conditions; after each createFixture(...) call and after mutating element state (e.g., element.type = 'submit', element.disabled = true, element.readOnly = true, element.sync(), and after element.remove()) insert await elementIsStable(fixture) (or await elementIsStable(element) where appropriate) before dispatching events or asserting; update all test cases (those referencing createFixture, element.sync(), element.dispatchEvent, element.remove()) to await stability so listeners/state are settled before assertions.Source: Coding guidelines
♻️ Duplicate comments (2)
projects/forms/src/mixins/slider.test.ts (2)
72-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuarantee fixture cleanup with
try/finally.At Line 73, cleanup currently depends on reaching Line 96; a failed assertion leaks DOM state into later tests.
Proposed fix
it('should support custom slider defaults', async () => { const customFixture = await createFixture(html` <form> <ui-custom-slider-defaults-test-element name="level"></ui-custom-slider-defaults-test-element> </form> `); - const customElement = customFixture.querySelector<CustomSliderDefaultsTestElement>( - 'ui-custom-slider-defaults-test-element' - )!; - const customForm = customFixture.querySelector<HTMLFormElement>('form')!; - - expect(customElement.min).toBe(10); - expect(customElement.max).toBe(20); - expect(customElement.step).toBe(2); - expect(customElement.value).toBe(14); - expect(getInternals(customElement).ariaValueMin).toBe('10'); - expect(getInternals(customElement).ariaValueMax).toBe('20'); - expect(getInternals(customElement).ariaValueNow).toBe('14'); - expect(new FormData(customForm).get('level')).toBe('14'); - - customElement.valueAsNumber = 18; - customElement.formResetCallback(); - expect(customElement.valueAsNumber).toBe(14); - - removeFixture(customFixture); + try { + const customElement = customFixture.querySelector<CustomSliderDefaultsTestElement>( + 'ui-custom-slider-defaults-test-element' + )!; + const customForm = customFixture.querySelector<HTMLFormElement>('form')!; + + expect(customElement.min).toBe(10); + expect(customElement.max).toBe(20); + expect(customElement.step).toBe(2); + expect(customElement.value).toBe(14); + expect(getInternals(customElement).ariaValueMin).toBe('10'); + expect(getInternals(customElement).ariaValueMax).toBe('20'); + expect(getInternals(customElement).ariaValueNow).toBe('14'); + expect(new FormData(customForm).get('level')).toBe('14'); + + customElement.valueAsNumber = 18; + customElement.formResetCallback(); + expect(customElement.valueAsNumber).toBe(14); + } finally { + removeFixture(customFixture); + } });As per coding guidelines, tests should follow unit testing fixture patterns from
/projects/site/src/docs/internal/guidelines/testing-unit.md(reliable fixture lifecycle handling).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/forms/src/mixins/slider.test.ts` around lines 72 - 97, The test "should support custom slider defaults" leaks DOM when an assertion fails because removeFixture(customFixture) is only called at the end; wrap the fixture usage in a try/finally so removeFixture(customFixture) always runs. Locate the test function (the it block using createFixture, customFixture, customElement, customForm) and change the flow to create the fixture, then run assertions and interactions inside a try block and call removeFixture(customFixture) in the finally block to guarantee cleanup regardless of failures.Source: Coding guidelines
23-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
requestUpdate()to the custom slider test host.At Line 23,
CustomSliderDefaultsTestElementomitsrequestUpdate, while this mixin test path mutates reactive state and can hit a missing-method runtime failure.Proposed fix
class CustomSliderDefaultsTestElement extends SliderFormControlMixin<typeof HTMLElement>(HTMLElement) { @@ static readonly sliderDefaults = { max: 20, min: 10, step: 2, value: 14 }; + + requestUpdate() {} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/forms/src/mixins/slider.test.ts` around lines 23 - 40, CustomSliderDefaultsTestElement (the test host that extends SliderFormControlMixin) is missing the requestUpdate method used by the mixin; add a requestUpdate implementation to CustomSliderDefaultsTestElement (a simple no-op or a Promise-returning no-op is fine) so reactive state mutations in the SliderFormControlMixin won't throw at runtime; keep the method on the class alongside the existing static metadata and sliderDefaults so the test host satisfies the mixin's expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/forms/src/mixins/control.ts`:
- Line 447: The current code creates a FormControlError instance only to log its
.message; replace that with a simpler form: either build the message string
directly using this.localName (e.g. `${this.localName}: cannot set number value
on non-number type`) and pass it to console.warn, or pass the FormControlError
object itself to console.warn to retain stack/error metadata. Update the call at
the console.warn currently constructing new FormControlError(this.localName,
'cannot set number value on non-number type').message accordingly.
---
Outside diff comments:
In `@projects/forms/src/internal/controllers/type-submit.controller.test.ts`:
- Around line 62-159: Tests use createFixture but do not await elementIsStable,
causing race conditions; after each createFixture(...) call and after mutating
element state (e.g., element.type = 'submit', element.disabled = true,
element.readOnly = true, element.sync(), and after element.remove()) insert
await elementIsStable(fixture) (or await elementIsStable(element) where
appropriate) before dispatching events or asserting; update all test cases
(those referencing createFixture, element.sync(), element.dispatchEvent,
element.remove()) to await stability so listeners/state are settled before
assertions.
In `@projects/forms/src/internal/controllers/type-submit.controller.ts`:
- Around line 59-69: The click handler `#onSubmitClick` only checks
this.host.disabled but not this.host.readOnly, allowing submits/resets during
update lag; add a guard at the start of `#onSubmitClick` to treat readOnly like
disabled (call stopEvent(event) and return) before proceeding to call
`#requestSubmit`(this.host.form) or this.host.form.reset() so read-only hosts
cannot submit or reset.
---
Duplicate comments:
In `@projects/forms/src/mixins/slider.test.ts`:
- Around line 72-97: The test "should support custom slider defaults" leaks DOM
when an assertion fails because removeFixture(customFixture) is only called at
the end; wrap the fixture usage in a try/finally so removeFixture(customFixture)
always runs. Locate the test function (the it block using createFixture,
customFixture, customElement, customForm) and change the flow to create the
fixture, then run assertions and interactions inside a try block and call
removeFixture(customFixture) in the finally block to guarantee cleanup
regardless of failures.
- Around line 23-40: CustomSliderDefaultsTestElement (the test host that extends
SliderFormControlMixin) is missing the requestUpdate method used by the mixin;
add a requestUpdate implementation to CustomSliderDefaultsTestElement (a simple
no-op or a Promise-returning no-op is fine) so reactive state mutations in the
SliderFormControlMixin won't throw at runtime; keep the method on the class
alongside the existing static metadata and sliderDefaults so the test host
satisfies the mixin's expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 90bc599f-8774-4acd-8bd8-d5953969cf4b
📒 Files selected for processing (18)
projects/forms/src/internal/controllers/state-current.controller.test.tsprojects/forms/src/internal/controllers/state-current.controller.tsprojects/forms/src/internal/controllers/state-expanded.controller.test.tsprojects/forms/src/internal/controllers/state-expanded.controller.tsprojects/forms/src/internal/controllers/state-pressed.controller.test.tsprojects/forms/src/internal/controllers/state-pressed.controller.tsprojects/forms/src/internal/controllers/state-selected.controller.test.tsprojects/forms/src/internal/controllers/state-selected.controller.tsprojects/forms/src/internal/controllers/type-submit.controller.test.tsprojects/forms/src/internal/controllers/type-submit.controller.tsprojects/forms/src/internal/types.tsprojects/forms/src/mixins/button.tsprojects/forms/src/mixins/checkbox.test.tsprojects/forms/src/mixins/control.test.tsprojects/forms/src/mixins/control.tsprojects/forms/src/mixins/slider.test.tsprojects/forms/src/validators/index.test.tsprojects/forms/src/validators/index.ts
| this.value = value as T; | ||
| } else { | ||
| throw new FormControlError(this.localName, 'cannot set number value on non-number type'); | ||
| console.warn(new FormControlError(this.localName, 'cannot set number value on non-number type').message); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider simplifying the warning message construction.
The code instantiates a FormControlError object solely to extract its .message property. Consider either constructing the message string directly or passing the error object itself to console.warn.
♻️ Proposed refactor options
Option 1: Construct message directly
- console.warn(new FormControlError(this.localName, 'cannot set number value on non-number type').message);
+ console.warn(`(${this.localName}): cannot set number value on non-number type`);Option 2: Pass error object
- console.warn(new FormControlError(this.localName, 'cannot set number value on non-number type').message);
+ console.warn(new FormControlError(this.localName, 'cannot set number value on non-number type'));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.warn(new FormControlError(this.localName, 'cannot set number value on non-number type').message); | |
| console.warn(`(${this.localName}): cannot set number value on non-number type`); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/forms/src/mixins/control.ts` at line 447, The current code creates a
FormControlError instance only to log its .message; replace that with a simpler
form: either build the message string directly using this.localName (e.g.
`${this.localName}: cannot set number value on non-number type`) and pass it to
console.warn, or pass the FormControlError object itself to console.warn to
retain stack/error metadata. Update the call at the console.warn currently
constructing new FormControlError(this.localName, 'cannot set number value on
non-number type').message accordingly.
Signed-off-by: Cory Rylan <crylan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/internal/types/index.ts`:
- Around line 116-118: The InterestEvent type exports source as HTMLElement
which is too narrow; update the exported type declaration for InterestEvent so
its source property is typed as Element (not HTMLElement) to match MDN and avoid
forcing callers to cast; locate the InterestEvent type in the file (symbol:
InterestEvent) and change the source property's type accordingly, ensuring
related imports/types remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1ecbdc62-b34b-4612-9e81-a93297ac1acb
📒 Files selected for processing (23)
projects/core/src/internal/controllers/state-active.controller.test.tsprojects/core/src/internal/controllers/state-active.controller.tsprojects/core/src/internal/controllers/state-current.controller.test.tsprojects/core/src/internal/controllers/state-current.controller.tsprojects/core/src/internal/controllers/state-disabled.controller.test.tsprojects/core/src/internal/controllers/state-disabled.controller.tsprojects/core/src/internal/controllers/state-pressed.controller.test.tsprojects/core/src/internal/controllers/state-pressed.controller.tsprojects/core/src/internal/controllers/type-button.controller.examples.tsprojects/core/src/internal/controllers/type-button.controller.test.tsprojects/core/src/internal/controllers/type-button.controller.tsprojects/core/src/internal/controllers/type-command.controller.test.tsprojects/core/src/internal/controllers/type-command.controller.tsprojects/core/src/internal/controllers/type-interest.controller.test.tsprojects/core/src/internal/controllers/type-interest.controller.tsprojects/core/src/internal/controllers/type-native-popover-trigger.controller.test.tsprojects/core/src/internal/controllers/type-native-popover-trigger.controller.tsprojects/core/src/internal/controllers/type-native-popover.controller.tsprojects/core/src/internal/controllers/type-submit.controller.test.tsprojects/core/src/internal/controllers/type-submit.controller.tsprojects/core/src/internal/index.tsprojects/core/src/internal/types/index.tsprojects/internals/metadata/static/tests.json
💤 Files with no reviewable changes (20)
- projects/core/src/internal/controllers/type-native-popover-trigger.controller.test.ts
- projects/core/src/internal/controllers/state-current.controller.test.ts
- projects/core/src/internal/controllers/state-pressed.controller.test.ts
- projects/core/src/internal/controllers/type-submit.controller.test.ts
- projects/core/src/internal/controllers/state-current.controller.ts
- projects/core/src/internal/controllers/type-submit.controller.ts
- projects/core/src/internal/controllers/type-button.controller.ts
- projects/core/src/internal/controllers/type-command.controller.ts
- projects/core/src/internal/controllers/type-native-popover-trigger.controller.ts
- projects/core/src/internal/controllers/type-button.controller.test.ts
- projects/core/src/internal/controllers/type-button.controller.examples.ts
- projects/core/src/internal/controllers/state-disabled.controller.test.ts
- projects/core/src/internal/controllers/type-interest.controller.ts
- projects/core/src/internal/controllers/type-command.controller.test.ts
- projects/core/src/internal/controllers/state-disabled.controller.ts
- projects/core/src/internal/controllers/state-pressed.controller.ts
- projects/core/src/internal/controllers/state-active.controller.ts
- projects/core/src/internal/controllers/type-interest.controller.test.ts
- projects/core/src/internal/index.ts
- projects/core/src/internal/controllers/state-active.controller.test.ts
johnyanarella
approved these changes
Jun 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features