-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat(aria/spinbutton): add aria spinbutton component #32663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
8f419d2 to
19a53fd
Compare
249c6f2 to
76f3abf
Compare
Implements a spinbutton ARIA primitive as a compound component following the W3C APG spinbutton pattern. The implementation includes: - SpinButtonPattern class with value management, keyboard handling, and wrap/clamp behavior - SpinButton parent directive for container and state management - SpinButtonInput directive for the focusable element (supports both input and span elements) - SpinButtonIncrement/Decrement button directives - Comprehensive test coverage - Two dev-app examples: APG hotel guest counter and time field segments
76f3abf to
8a1a79f
Compare
src/aria/spinbutton/spinbutton.ts
Outdated
| if (!this._hasFocused()) { | ||
| this._pattern.setDefaultState(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this ? setDefaultState is empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| private _hasFocused = signal(false); | ||
|
|
||
| /** The UI pattern instance for this spinbutton. */ | ||
| readonly _pattern: SpinButtonPattern = new SpinButtonPattern({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's not private
| readonly _pattern: SpinButtonPattern = new SpinButtonPattern({ | |
| readonly pattern: SpinButtonPattern = new SpinButtonPattern({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that all other angular aria primitives use readonly _pattern. Should I keep _pattern here for consistency?
| } | ||
|
|
||
| // @public | ||
| export class SpinButtonPattern { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really belong in the public API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other pattern classes also exported in the private API golden, I assumed this one should be exported too
src/aria/spinbutton/spinbutton.ts
Outdated
| } | ||
|
|
||
| /** Called when the input receives focus. */ | ||
| _onFocus(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be private ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| } | ||
|
|
||
| /** Handles pointerdown events for the spinbutton. */ | ||
| onPointerdown(_event: PointerEvent): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be protected ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the other pattern classes, onPointerdown is public everywhere. Happy to add protected if you'd like to start that convention here, just wanted to flag the inconsistency.
| } | ||
|
|
||
| /** Sets the spinbutton to its default initial state. */ | ||
| setDefaultState(): void {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is an implementation missing here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think setDefaultState() is expected as a core method, see UI Pattern rules. For SpinButton, the state is fully determined by signal inputs (value, min, max), so no additional initialization is needed. Other patterns (e.g., Listbox, Tree) use this to set a default active item from dynamic children.
I saw that a few other patterns leave it as a noop too
| override setDefaultState(): void {} |
| override setDefaultState(): void {} |
Not sure if I should leave it like this or remove it entirely.
Co-authored-by: Matthieu Riegler <kyro38@gmail.com>
Co-authored-by: Matthieu Riegler <kyro38@gmail.com>
Co-authored-by: Matthieu Riegler <kyro38@gmail.com>
Co-authored-by: Matthieu Riegler <kyro38@gmail.com>
Co-authored-by: Matthieu Riegler <kyro38@gmail.com>
Co-authored-by: Matthieu Riegler <kyro38@gmail.com>
- Remove unnecessary setDefaultState effect and related code - Clarify setDefaultState JSDoc is intentionally empty - Use native ARIA binding syntax in increment and input - Update API golden
2cc8550 to
f72df1b
Compare
This PR implements the ARIA spinbutton pattern based on WAI-ARIA guidelines.
WAI-ARIA Pattern Reference:
https://www.w3.org/WAI/ARIA/apg/patterns/spinbutton/
Completed:
ngSpinButton,ngSpinButtonInput,ngSpinButtonIncrement,ngSpinButtonDecrementaria-valuenow,aria-valuetext,aria-valuemin,aria-valuemax,aria-disabled,aria-readonly,aria-invalidFuture Enhancements:
aria-requiredattribute support