Skip to content
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

fix(components/forms): set radio group 'aria-owns' to satisfy accessibility rules #671

Merged
merged 10 commits into from
Oct 21, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
<legend id="radio-group-label">Reactive Radio button options:</legend>
<ul class="sky-list-unstyled">
<li *ngFor="let value of options">
<sky-radio [disabled]="value.disabled" [value]="value">
<sky-radio [disabled]="value.disabled" [id]="value.id" [value]="value">
<sky-radio-label> Option {{ value.name }} </sky-radio-label>
</sky-radio>
</li>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export class SkyRadioGroupReactiveFixtureComponent implements OnInit {

public initialValue: unknown = null;

public options = [
public options: { name: string; disabled: boolean; id?: string }[] = [
{ name: 'Lillith Corharvest', disabled: false },
{ name: 'Harima Kenji', disabled: false },
{ name: 'Harry Mckenzie', disabled: false },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ <h1 id="radio-group-label">Radio label</h1>
[label]="label1"
[value]="value1"
[(ngModel)]="selectedValue"
(checkedChange)="onCheckedChange($event)"
(disabledChange)="onDisabledChange($event)"
Blackbaud-SteveBrush marked this conversation as resolved.
Show resolved Hide resolved
>
<sky-radio-label> My label </sky-radio-label>
</sky-radio>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { AfterViewInit, Component, ViewChild } from '@angular/core';
import { Component, ViewChild } from '@angular/core';

import { SkyRadioComponent } from '../radio.component';

@Component({
templateUrl: './radio.component.fixture.html',
})
export class SkyRadioTestComponent implements AfterViewInit {
export class SkyRadioTestComponent {
public selectedValue = '1';
public disabled1 = false;

Expand All @@ -23,13 +23,17 @@ export class SkyRadioTestComponent implements AfterViewInit {
@ViewChild(SkyRadioComponent)
public checkboxComponent!: SkyRadioComponent;

public ngAfterViewInit() {
this.checkboxComponent.disabledChange.subscribe((value) => {
this.onDisabledChange(value);
});
// eslint-disable-next-line @typescript-eslint/no-unused-vars
public onCheckedChange(checked: boolean): void {
Blackbaud-PaulCrowder marked this conversation as resolved.
Show resolved Hide resolved
/* */
}

public onDisabledChange(value: boolean): void {}
public onClick(): void {
/* */
}

public onClick() {}
// eslint-disable-next-line @typescript-eslint/no-unused-vars
public onDisabledChange(disabled: boolean): void {
Blackbaud-PaulCrowder marked this conversation as resolved.
Show resolved Hide resolved
/* */
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { Injectable } from '@angular/core';

import { BehaviorSubject, Observable } from 'rxjs';

/**
* Tracks the element IDs for all radios within a radio group.
* @internal
*/
@Injectable()
export class SkyRadioGroupIdService {
public get radioIds(): Observable<string[]> {
return this.#radioIdsObs;
Blackbaud-ErikaMcVey marked this conversation as resolved.
Show resolved Hide resolved
}

#radioIds: string[] = [];
#radioIds$: BehaviorSubject<string[]>;
#radioIdsObs: Observable<string[]>;

constructor() {
this.#radioIds$ = new BehaviorSubject<string[]>([]);
this.#radioIdsObs = this.#radioIds$.asObservable();
}

/**
* Associates a radio input's ID with its parent radio group.
*/
public registerId(id: string): void {
if (this.#radioIds.indexOf(id) === -1) {
this.#radioIds.push(id);
this.#emitRadioIds();
}
}

/**
* Updates a radio input's ID with its parent radio group.
*/
public updateId(oldId: string, newId: string): void {
const index = this.#radioIds.indexOf(oldId);
if (index > -1) {
this.#radioIds[index] = newId;
this.#emitRadioIds();
}
}

/**
* Disassociates a radio input's ID with its parent radio group.
*/
public unregisterId(id: string): void {
const index = this.#radioIds.indexOf(id);
if (index > -1) {
this.#radioIds.splice(index);
this.#emitRadioIds();
}
}

#emitRadioIds(): void {
this.#radioIds$.next(this.#radioIds);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
role="radiogroup"
[attr.aria-label]="ariaLabel"
[attr.aria-labelledby]="ariaLabelledBy"
[attr.aria-owns]="ariaOwns"
[attr.aria-required]="required ? true : null"
[attr.required]="required ? '' : null"
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,33 @@ describe('Radio group component (reactive)', function () {
await fixture.whenStable();
await expectAsync(fixture.nativeElement).toBeAccessible();
});

it('should set aria-owns as a space-separated list of radio ids', () => {
fixture.detectChanges();

const radioGroupEl: HTMLDivElement =
fixture.nativeElement.querySelector('.sky-radio-group');

expect(radioGroupEl.getAttribute('aria-owns')).toMatch(
Blackbaud-PaulCrowder marked this conversation as resolved.
Show resolved Hide resolved
/sky-radio-sky-radio-[0-9]+-input sky-radio-sky-radio-[0-9]+-input sky-radio-sky-radio-[0-9]+-input/
);
});

it('should update aria-owns if radio IDs change', () => {
Blackbaud-ErikaMcVey marked this conversation as resolved.
Show resolved Hide resolved
fixture.detectChanges();

const radioGroupEl: HTMLDivElement =
fixture.nativeElement.querySelector('.sky-radio-group');

const originalAriaOwns = radioGroupEl.getAttribute('aria-owns');

fixture.componentInstance.options[0].id = 'foobar';
fixture.detectChanges();

const newAriaOwns = radioGroupEl.getAttribute('aria-owns');

expect(originalAriaOwns).not.toEqual(newAriaOwns);
Blackbaud-PaulCrowder marked this conversation as resolved.
Show resolved Hide resolved
});
});

describe('Radio group component (template-driven)', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
ChangeDetectorRef,
Component,
ContentChildren,
Host,
Input,
OnDestroy,
Optional,
Expand All @@ -17,6 +18,7 @@ import { takeUntil } from 'rxjs/operators';

import { SkyFormsUtility } from '../shared/forms-utility';

import { SkyRadioGroupIdService } from './radio-group-id.service';
import { SkyRadioComponent } from './radio.component';
import { SkyRadioChange } from './types/radio-change';

Expand All @@ -31,6 +33,7 @@ let nextUniqueId = 0;
@Component({
selector: 'sky-radio-group',
templateUrl: './radio-group.component.html',
providers: [SkyRadioGroupIdService],
})
export class SkyRadioGroupComponent
implements AfterContentInit, AfterViewInit, OnDestroy
Expand Down Expand Up @@ -135,6 +138,15 @@ export class SkyRadioGroupComponent
return this.#_tabIndex;
}

/**
* Our radio components are usually implemented using an unordered list. This is an
* accessibility violation because the unordered list has an implicit role which
* interrupts the 'radiogroup' and 'radio' relationship. To correct this, we can set the
* radio group's 'aria-owns' attribute to a space-separated list of radio IDs.
* @see https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/radio_role
*/
public ariaOwns: string | undefined;

@ContentChildren(SkyRadioComponent, { descendants: true })
public radios: QueryList<SkyRadioComponent> | undefined;

Expand All @@ -151,18 +163,28 @@ export class SkyRadioGroupComponent
#_tabIndex: number | undefined;

#changeDetector: ChangeDetectorRef;
#radioGroupIdSvc: SkyRadioGroupIdService;
#ngControl: NgControl | undefined;

constructor(
changeDetector: ChangeDetectorRef,
@Host() radioGroupIdSvc: SkyRadioGroupIdService,
Blackbaud-PaulCrowder marked this conversation as resolved.
Show resolved Hide resolved
@Self() @Optional() ngControl: NgControl
) {
if (ngControl) {
ngControl.valueAccessor = this;
}
this.#changeDetector = changeDetector;
this.#radioGroupIdSvc = radioGroupIdSvc;
this.#ngControl = ngControl;
this.name = this.#defaultName;

this.#radioGroupIdSvc.radioIds
.pipe(takeUntil(this.#ngUnsubscribe))
.subscribe((ids) => {
this.ariaOwns = ids.join(' ') || undefined;
this.#changeDetector.markForCheck();
});
}

public ngAfterContentInit(): void {
Expand Down Expand Up @@ -248,8 +270,11 @@ export class SkyRadioGroupComponent
this.#onTouched = fn;
}

/* istanbul ignore next */
// eslint-disable-next-line @typescript-eslint/no-empty-function
#onChange: (value: any) => void = () => {};

/* istanbul ignore next */
// eslint-disable-next-line @typescript-eslint/no-empty-function
#onTouched: () => any = () => {};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,20 @@ describe('Radio component', function () {
expect(onDisabledChangeSpy).toHaveBeenCalledTimes(1);
});

it('should emit when radio is checked', async () => {
const onCheckedChangeSpy = spyOn(testComponent, 'onCheckedChange');
const radios: NodeListOf<HTMLInputElement> =
fixture.nativeElement.querySelectorAll('input');

// Select the second radio.
radios.item(1).click();
fixture.detectChanges();
await fixture.whenStable();

expect(onCheckedChangeSpy).toHaveBeenCalledWith(false);
Blackbaud-PaulCrowder marked this conversation as resolved.
Show resolved Hide resolved
expect(onCheckedChangeSpy).toHaveBeenCalledTimes(1);
});

it('should update the ngModel properly when radio button is changed', fakeAsync(function () {
const radioElement = fixture.debugElement.queryAll(
By.directive(SkyRadioComponent)
Expand Down
31 changes: 25 additions & 6 deletions libs/components/forms/src/lib/modules/radio/radio.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import {
Component,
Input,
OnDestroy,
OnInit,
Optional,
Output,
Provider,
forwardRef,
Expand All @@ -14,6 +16,7 @@ import { BehaviorSubject, Observable, Subject } from 'rxjs';

import { SkyFormsUtility } from '../shared/forms-utility';

import { SkyRadioGroupIdService } from './radio-group-id.service';
import { SkyRadioChange } from './types/radio-change';

/**
Expand Down Expand Up @@ -43,7 +46,9 @@ const SKY_RADIO_CONTROL_VALUE_ACCESSOR: Provider = {
providers: [SKY_RADIO_CONTROL_VALUE_ACCESSOR],
changeDetection: ChangeDetectionStrategy.OnPush,
})
export class SkyRadioComponent implements OnDestroy, ControlValueAccessor {
export class SkyRadioComponent
implements OnInit, OnDestroy, ControlValueAccessor
{
/**
* Fires when users focus off a radio button.
*/
Expand Down Expand Up @@ -97,10 +102,14 @@ export class SkyRadioComponent implements OnDestroy, ControlValueAccessor {
*/
@Input()
public set id(value: string | undefined) {
if (value) {
this.inputId = `sky-radio-${value}-input`;
} else {
this.inputId = `sky-radio-${this.#defaultId}-input`;
const oldId = this.inputId;
const newId = value
? `sky-radio-${value}-input`
: `sky-radio-${this.#defaultId}-input`;

if (oldId !== newId) {
this.inputId = newId;
this.#radioGroupIdSvc?.updateId(oldId, newId);
}
}

Expand Down Expand Up @@ -284,9 +293,14 @@ export class SkyRadioComponent implements OnDestroy, ControlValueAccessor {
#_value: any;

#changeDetector: ChangeDetectorRef;
#radioGroupIdSvc: SkyRadioGroupIdService | undefined;

constructor(changeDetector: ChangeDetectorRef) {
constructor(
changeDetector: ChangeDetectorRef,
@Optional() radioGroupIdService?: SkyRadioGroupIdService
) {
this.#changeDetector = changeDetector;
this.#radioGroupIdSvc = radioGroupIdService;
this.#change = new Subject<SkyRadioChange>();
this.#changeObs = this.#change.asObservable();
this.#checkedChange = new BehaviorSubject<boolean>(this.checked);
Expand All @@ -297,7 +311,12 @@ export class SkyRadioComponent implements OnDestroy, ControlValueAccessor {
this.id = this.#defaultId;
}

public ngOnInit(): void {
this.#radioGroupIdSvc?.registerId(this.inputId);
}

public ngOnDestroy(): void {
this.#radioGroupIdSvc?.unregisterId(this.inputId);
this.#removeUniqueSelectionListener();
this.#change.complete();
this.#checkedChange.complete();
Expand Down