fix(forms): improve aria attribute management#128
Open
coryrylan wants to merge 1 commit into
Open
Conversation
- 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>
📝 WalkthroughWalkthroughThe PR refactors aria attribute handling across state controllers to consistently set internals to ChangesState Controllers and Type System Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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
Comment on lines
+72
to
+97
| 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); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Guard fixture cleanup with try/finally in the async test.
If any assertion fails before Line 96, customFixture is never removed, which can leak DOM state into later tests.
As per coding guidelines, tests should follow stable fixture lifecycle patterns from /projects/site/src/docs/internal/guidelines/testing-unit.md (including reliable fixture handling).
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);
+ }
});📝 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
| 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); | |
| }); | |
| 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> | |
| `); | |
| 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); | |
| } | |
| }); |
🤖 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'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.
Source: Coding guidelines
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
1 participant
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