Skip to content

refactor(checkbox,switch,radio): migrate to signals and OnPush - #17604

Open
mddragnev wants to merge 47 commits into
masterfrom
mdragnev/checkbox-signals-migration
Open

mddragnev wants to merge 47 commits into
masterfrom
mdragnev/checkbox-signals-migration

Conversation

@mddragnev

@mddragnev mddragnev commented Sep 14, 2026 •

Copy link
Copy Markdown
Member

Related:
#17635 #17633 #17634

Description

Checkbox, switch, radio and CheckboxBaseDirective

  • Internal state is signal-backed. checked, disabled, readonly, indeterminate, invalid, focused, required, id, labelId, value, name, tabindex, labelPosition, disableRipple, ariaLabelledBy and ariaLabel read and write signal()s behind their existing accessors.
  • Derived state is memoized with computed(), and the templates read the backing signals directly.
  • CheckboxBaseDirective stays abstract. disabled is now an abstract accessor that each control implements, so the radio can combine its own disabled input with the group's form control state.
  • The destroy$/takeUntil pair around ngControl.statusChanges is replaced with takeUntilDestroyed(). destroy$ was never completed, so it never actually fired.

Radio group

  • State is signal-backed: value, selected, name, required, invalid, disabled and alignment. The --before and --disabled host classes are computed() over the registered buttons, and every host binding and listener lives in host: {} metadata.
  • Buttons register synchronously in the radio's ngOnInit and get the group's name, required, disabled state and selection immediately. This replaces the state-writing effect() and the Promise.resolve() push.
  • Form control wiring runs once, in ngAfterContentInit. Signal Forms keep required in sync at runtime.
  • Each radio's tab order is a computed() over the group's checked button (roving tabindex). This replaces ngDoCheck, which wrote tabIndex on every change detection pass and fought the template binding.
  • Radios call the group directly for selection, blur, keyup and value changes. The group no longer subscribes to radio events.

Public API is unchanged. Every @Input()/@Output() keeps its name, alias, type and transform, so radio.checked = true, group.value = x and [checked]="x" behave exactly as before. There is no input()/output()/model() conversion. The only removals are @hidden @internal members: the radio's blurRadio and groupDisabled, and the group's cssClass, ngDoCheck and ngOnDestroy.

Bug fixes (IgxRadioGroupDirective)

  • Arrow-key navigation follows the rendered order of the radio buttons, including buttons inserted in the middle of an @for or created with ViewContainerRef.createComponent(). This applies in the browser only; elsewhere the order of registration is kept.
  • selected is cleared when value is null or matches no radio button.
  • Only the checked radio is in the tab order from the first render, and every radio gets its own tabindex back when the value is cleared.
  • Radio buttons removed from the group are no longer kept subscribed to for the group's lifetime.

Deliberately left alone

  • required stays a getter rather than becoming a computed(). It falls back to nativeElement.hasAttribute('required'), which isn't part of the reactive graph, so memoizing it would go stale and silently drop the fallback.
  • labelId and ariaLabelledBy keep their snapshot semantics: they capture id once on initialization rather than deriving from it.
  • The group still pushes name, required, invalid and checked into its radios instead of the radios deriving them. Deriving would change what radio.name or radio.checked return after a consumer sets them directly.

Motivation / Context

Part of the ongoing move to signals and away from zone-based change detection.

Eager (CheckAlways) meant these components were re-checked on every change detection pass across the whole application, and the radio group relied on ngDoCheck and markForCheck() to keep its children in sync. Backing the state with signals is what makes OnPush and zoneless safe here.

The constraint was no public API changes and no breaking changes, which is why the @Input()/@Output() decorators and accessor shapes are preserved rather than converted to signal inputs.

Type of Change (check all that apply):

  • Bug fix
  • New functionality
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (no functional changes)
  • Documentation
  • Demos
  • CI/CD
  • Tests
  • Changelog
  • Skills/Agents

Component(s) / Area(s) Affected:

IgxCheckboxComponent, IgxSwitchComponent, IgxRadioComponent, IgxRadioGroupDirective, CheckboxBaseDirective

How Has This Been Tested?

  • Unit tests
  • Manual testing
  • Automated e2e tests

Test Configuration:

  • Angular version: 22.1.1
  • Browser(s): Chrome Headless 152
  • OS: macOS

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them
  • Accessibility (ARIA, keyboard navigation, focus management) has been verified

mddragnev and others added 7 commits September 14, 2026 13:28
Replace the plain backing fields of the shared checkbox/switch/radio base
directive with Angular signals, grouped at the top of the class so that each
JSDoc block documents the public accessor rather than the backing field.

The public API is unchanged: every member keeps its @input()/@output()
decorator and plain property shape, so `checkbox.checked = true` and
[checked]="x" behave exactly as before.

Also:
- replace the destroy$/takeUntil cleanup around ngControl.statusChanges with
  takeUntilDestroyed(); destroy$ was never completed, so it never fired
- move the @HostBinding/@HostListener declarations into the decorator's host
  metadata
- expose destroyRef so a parent can scope subscriptions to a single instance

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Switch from ChangeDetectionStrategy.Eager to OnPush, now that the state the
component renders is signal-backed and marks the view dirty on every write.
Also moves the @HostBinding declarations into the decorator's host metadata.

The public API is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Switch from ChangeDetectionStrategy.Eager to OnPush, now that the state the
component renders is signal-backed and marks the view dirty on every write.
Also moves the @HostBinding declarations into the decorator's host metadata.

The public API is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Switch from ChangeDetectionStrategy.Eager to OnPush, now that the state the
component renders is signal-backed and marks the view dirty on every write.
Also moves the @HostBinding/@HostListener declarations into the decorator's
host metadata.

The public API is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Convert the ariaChecked and labelClass getters to memoized computed() signals
and read the backing signals directly from the templates.

Both getters ran once per binding on every dirty change-detection pass, with
labelClass allocating a new string each time. As computed() they recompute
only when their dependencies change: measured 10 -> 0 recomputations over 10
dirty passes with labelPosition unchanged.

The public ariaChecked/labelClass getters are removed. Both were marked
@hidden @internal and were referenced only by these three templates.

required deliberately stays a getter: it falls back to
nativeElement.hasAttribute('required'), which is not part of the reactive
graph, so a computed() would go stale and drop the fallback.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_setRadioButtonEvents subscribed to each button's change/blurRadio/keyup
streams but only tore them down when the whole group was destroyed, so
cycling buttons through a structural directive accumulated subscriptions for
the lifetime of the group.

The existing takeUntil(button.destroy$) never fired - destroy$ was declared on
CheckboxBaseDirective but never completed - so the intended per-button cleanup
was inert. Scope the subscriptions to the button's own DestroyRef instead, and
cover it with a regression test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Document the OnPush switch for the checkbox, switch and radio components
under Behavioral Changes, and the radio group's per-button subscription
cleanup under Bug Fixes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

DOM reordering can leave keyboard navigation stale, and memoization changes writable cssClass behavior.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)

Comment thread projects/igniteui-angular/radio/src/radio/radio-group/radio-group.directive.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Null-valued radio selection can remain stale, and the changelog section order needs correction.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Null assignment can leave a radio button selected

projects/​igniteui-angular/​radio/​src/​radio/​radio-group/​radio-group.directive.ts:160

value = null does not always clear selected: when _value is already null (for example, after assigning selected to a radio whose value is null), this guard skips _selectRadioButton() and leaves that radio selected even though null is defined as the cleared state. Run the synchronization for null even when the stored value is unchanged.

Low severity Keep Unreleased as the leading changelog section

CHANGELOG.md:5

Unreleased was the leading changelog section, but this inserts a versioned section above it. That reverses the established chronology and separates these entries from the other pending changes; keep Unreleased first, moving this section below it only as part of a release cut (otherwise merge these entries into it).

@mddragnev
mddragnev marked this pull request as ready for review October 5, 2026 14:13
@mddragnev
mddragnev requested a review from rkaraivanov October 5, 2026 14:14
@viktorkombov viktorkombov added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Oct 6, 2026
@viktorkombov

Copy link
Copy Markdown
Contributor

I tested the PR in a zone-based and a zoneless app. The tests pass and the grid, tree and select still work fine with these controls. However, I found a few issues that seem to be regressions:

  1. Two radios checked. <igx-radio-group value="Large"> with @for (item of items(); track $index). Change items from ['Small', 'Medium', 'Large'] to ['Medium', 'Large', 'XL']. Both Large and XL are checked.
  2. An effect reverts the user's click. effect(() => checkbox.checked = accepted()), where accepted never changes. Click the checkbox and it unchecks itself. Same with group.value. The setters read their own signals, wrapping them in untracked() fixes it.
  3. selected = null doesn't clear the value. Render A, B, C with @for, select C, remove it from the list and set group.selected = null. group.value is still 'C', and C is checked again when added back.
  4. Slow with many radios. 2,000 radios in one group take about 9 s to render, vs 0.6 s on master (dev build with ng serve, headless Chrome 151, same machine for both).
  5. change subscribers see the old value. Subscribe to a radio's change after init and click it. In the handler group.value is still the previous value.
  6. aria-required stays true. A radio with its own form control that has a non-required validator (e.g. one that always fails), inside <igx-radio-group [required]="required()">. Set required to false and the input keeps aria-required="true".
  7. The radio's own control wins over the group. A radio whose form control has Validators.required is required, even though the group isn't.
  8. Focus ring stays after Tab. Put a link in an unchecked radio's label, click the checked radio and press Tab. Focus goes to the link, but the checked radio keeps its focus ring.

Also, subclasses that redeclare these properties as fields, use cdr or call super.ngDoCheck() don't compile anymore, but the changelog says the public API is unchanged.

All of them are in a sample in the attached patch. Apply it on this branch with git apply checkbox-radio-review-sample.patch, run npm start and open /checkbox-radio-review. A red status line means the issue is showing.

checkbox-radio-review-sample.patch

@viktorkombov viktorkombov added awaiting author When the reviewer/verifier has some confusions about the scenario/behavior and wait author's action and removed 💥 status: in-test PRs currently being tested labels Oct 6, 2026
The radio emitted change before the group updated, so subscribers saw
the previous group value. Update the group state first, then notify
the radio (change, onChange), then the group, like a native radio.
A keyup from a focusable element in the label, such as a link, bubbled
to the radio host and marked the checked radio as focused. Only treat
keyups from the radio's input or host as keyboard focus.
@mddragnev

mddragnev commented Oct 7, 2026 •

Copy link
Copy Markdown
Member Author

I tested the PR in a zone-based and a zoneless app. The tests pass and the grid, tree and select still work fine with these controls. However, I found a few issues that seem to be regressions:

  1. Two radios checked. <igx-radio-group value="Large"> with @for (item of items(); track $index). Change items from ['Small', 'Medium', 'Large'] to ['Medium', 'Large', 'XL']. Both Large and XL are checked.
  2. An effect reverts the user's click. effect(() => checkbox.checked = accepted()), where accepted never changes. Click the checkbox and it unchecks itself. Same with group.value. The setters read their own signals, wrapping them in untracked() fixes it.
  3. selected = null doesn't clear the value. Render A, B, C with @for, select C, remove it from the list and set group.selected = null. group.value is still 'C', and C is checked again when added back.
  4. Slow with many radios. 2,000 radios in one group take about 9 s to render, vs 0.6 s on master (dev build with ng serve, headless Chrome 151, same machine for both).
  5. change subscribers see the old value. Subscribe to a radio's change after init and click it. In the handler group.value is still the previous value.
  6. aria-required stays true. A radio with its own form control that has a non-required validator (e.g. one that always fails), inside <igx-radio-group [required]="required()">. Set required to false and the input keeps aria-required="true".
  7. The radio's own control wins over the group. A radio whose form control has Validators.required is required, even though the group isn't.
  8. Focus ring stays after Tab. Put a link in an unchecked radio's label, click the checked radio and press Tab. Focus goes to the link, but the checked radio keeps its focus ring.

Also, subclasses that redeclare these properties as fields, use cdr or call super.ngDoCheck() don't compile anymore, but the changelog says the public API is unchanged.

All of them are in a sample in the attached patch. Apply it on this branch with git apply checkbox-radio-review-sample.patch, run npm start and open /checkbox-radio-review. A red status line means the issue is showing.

checkbox-radio-review-sample.patch

@viktorkombov
2. Angular discourages using effects to push state into components, however, I fixed it because it is valid code and somebody might do it.
3. I'm not fully convinced that selected = null should clear the group's value. I see selected as only "which radio button is currently checked", so when no button is checked, selected = null would be a no-op, and you'd set value = null to drop a pending value. But master clears the value in this case, so I've kept that behavior to avoid a regression.

I've updated the CHANGELOG with a Breaking Changes section about subclasses that no longer compile.
The other issues are fixed.

@mddragnev mddragnev added ❌ status: awaiting-test PRs awaiting manual verification and removed awaiting author When the reviewer/verifier has some confusions about the scenario/behavior and wait author's action labels Oct 7, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

4 participants