Skip to content

Conversation

@OmerGronich
Copy link

@OmerGronich OmerGronich commented Jan 16, 2026

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:

  • Four new directives: ngSpinButton, ngSpinButtonInput, ngSpinButtonIncrement, ngSpinButtonDecrement
  • Full keyboard navigation:
    • Arrow Up/Down: increment/decrement by step
    • Page Up/Down: increment/decrement by large step (configurable)
    • Home/End: jump to min/max values
  • Value wrapping support (max→min and min→max)
  • Proper ARIA attributes: aria-valuenow, aria-valuetext, aria-valuemin, aria-valuemax, aria-disabled, aria-readonly, aria-invalid
  • Support for both native input elements and span elements
  • Development mode validation warnings
  • Two example implementations:
    • Guest counter with increment/decrement buttons
    • Time field with hour/minute/period spinbuttons
  • Comprehensive test coverage

Future Enhancements:

  • Documentation guide
  • Additional examples (date picker fields, numeric stepper)
  • Indeterminate state support (undefined value)
  • aria-required attribute support

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
@OmerGronich OmerGronich marked this pull request as ready for review January 20, 2026 18:11
@pullapprove pullapprove bot requested a review from crisbeto January 20, 2026 18:11
@angular angular deleted a comment from google-cla bot Jan 27, 2026
Comment on lines 117 to 119
if (!this._hasFocused()) {
this._pattern.setDefaultState();
}
Copy link
Member

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

Copy link
Author

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({
Copy link
Member

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

Suggested change
readonly _pattern: SpinButtonPattern = new SpinButtonPattern({
readonly pattern: SpinButtonPattern = new SpinButtonPattern({

Copy link
Author

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 {
Copy link
Member

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?

Copy link
Author

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

}

/** Called when the input receives focus. */
_onFocus(): void {
Copy link
Member

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 ?

Copy link
Author

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be protected ?

Copy link
Author

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 {}
Copy link
Member

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 ?

Copy link
Author

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.

OmerGronich and others added 7 commits January 28, 2026 19:33
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

detected: feature PR contains a feature commit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants