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(selection-list): improve accessibility of selection list #10137

Merged
merged 1 commit into from
Feb 28, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion src/cdk/a11y/key-manager/list-key-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@ import {Subject} from 'rxjs/Subject';


class FakeFocusable {
constructor(private _label = '') { }
/** Whether the item is disabled or not. */
disabled = false;
/** Test property that can be used to test the `skipPredicate` functionality. */
skipItem = false;
constructor(private _label = '') {}
focus(_focusOrigin?: FocusOrigin) {}
getLabel() { return this._label; }
}
Expand Down Expand Up @@ -480,6 +483,31 @@ describe('Key managers', () => {
});
});

describe('skip predicate', () => {

it('should skip disabled items by default', () => {
itemList.items[1].disabled = true;

expect(keyManager.activeItemIndex).toBe(0);

keyManager.onKeydown(fakeKeyEvents.downArrow);

expect(keyManager.activeItemIndex).toBe(2);
});

it('should be able to skip items with a custom predicate', () => {
keyManager.skipPredicate(item => item.skipItem);

itemList.items[1].skipItem = true;

expect(keyManager.activeItemIndex).toBe(0);

keyManager.onKeydown(fakeKeyEvents.downArrow);

expect(keyManager.activeItemIndex).toBe(2);
});
});

describe('typeahead mode', () => {
const debounceInterval = 300;

Expand Down
24 changes: 21 additions & 3 deletions src/cdk/a11y/key-manager/list-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
private _vertical = true;
private _horizontal: 'ltr' | 'rtl' | null;

/**
* Predicate function that can be used to check whether an item should be skipped
* by the key manager. By default, disabled items are skipped.
*/
private _skipPredicateFn = (item: T) => item.disabled;
Copy link
Member

Choose a reason for hiding this comment

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

Call this one skipPredicate?

Copy link
Member Author

Choose a reason for hiding this comment

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

This would conflict with the function name that sets the predicate. We could name it _skipPredicate, but having the Fn suffix feels appropriate IMO.


// Buffer for the letters that the user has pressed when the typeahead option is turned on.
private _pressedLetters: string[] = [];

Expand All @@ -72,6 +78,16 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
/** Stream that emits whenever the active item of the list manager changes. */
change = new Subject<number>();

/**
* Sets the predicate function that determines which items should be skipped by the
* list key manager.
* @param predicate Function that determines whether the given item should be skipped.
*/
skipPredicate(predicate: (item: T) => boolean): this {
this._skipPredicateFn = predicate;
return this;
}

/**
* Turns on wrapping mode, which ensures that the active item will wrap to
* the other end of list when there are no more items in the given direction.
Expand Down Expand Up @@ -128,7 +144,9 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
const index = (this._activeItemIndex + i) % items.length;
const item = items[index];

if (!item.disabled && item.getLabel!().toUpperCase().trim().indexOf(inputString) === 0) {
if (!this._skipPredicateFn(item) &&
item.getLabel!().toUpperCase().trim().indexOf(inputString) === 0) {

this.setActiveItem(index);
break;
}
Expand Down Expand Up @@ -274,7 +292,7 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
const index = (this._activeItemIndex + (delta * i) + items.length) % items.length;
const item = items[index];

if (!item.disabled) {
if (!this._skipPredicateFn(item)) {
this.setActiveItem(index);
return;
}
Expand All @@ -301,7 +319,7 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
return;
}

while (items[index].disabled) {
while (this._skipPredicateFn(items[index])) {
index += fallbackDelta;

if (!items[index]) {
Expand Down
2 changes: 1 addition & 1 deletion src/cdk/testing/event-objects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export function createKeyboardEvent(type: string, keyCode: number, target?: Elem
}

/** Creates a fake event object with any desired event type. */
export function createFakeEvent(type: string, canBubble = true, cancelable = true) {
export function createFakeEvent(type: string, canBubble = false, cancelable = true) {
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should leave this one as true, because most of the events bubble. We can opt-out of it for the ones that don't.

Copy link
Member Author

@devversion devversion Feb 25, 2018

Choose a reason for hiding this comment

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

We can also check for the type and just default to false if the event is either focus | blur.

Copy link
Member

Choose a reason for hiding this comment

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

We could, but there might be more that don't bubble. We open ourselves up to having to maintain a list of events that don't bubble.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's why I think that we should be explicit the other way. If some tests insist on event bubbling, which is very unlikely, then that test should explicitly turn on bubbling.

Bubbling will most likely causing more side-effects, so disabling it by default in tests might be better. That's at least what I noticed while working with the focus event right now.

const event = document.createEvent('Event');
event.initEvent(type, canBubble, cancelable);
return event;
Expand Down
11 changes: 1 addition & 10 deletions src/lib/list/_list-theme.scss
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,8 @@
background-color: mat-color($background, disabled-list-option);
}

.mat-list-option,
.mat-nav-list .mat-list-item {
outline: none;

&:hover, &.mat-list-item-focus {
background: mat-color($background, 'hover');
}
}

.mat-list-option {
outline: none;

&:hover, &.mat-list-item-focus {
background: mat-color($background, 'hover');
}
Expand Down
3 changes: 1 addition & 2 deletions src/lib/list/list-option.html
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
<div class="mat-list-item-content"
[class.mat-list-item-content-reverse]="checkboxPosition == 'after'"
[class.mat-list-item-disabled]="disabled">
[class.mat-list-item-content-reverse]="checkboxPosition == 'after'">

<div mat-ripple
class="mat-list-item-ripple"
Expand Down
15 changes: 5 additions & 10 deletions src/lib/list/list.scss
Original file line number Diff line number Diff line change
Expand Up @@ -237,24 +237,19 @@ $mat-list-item-inset-divider-offset: 72px;
}
}


.mat-nav-list {
a {
text-decoration: none;
color: inherit;
}

.mat-list-item-content {
.mat-list-item {
cursor: pointer;

&:hover, &.mat-list-item-focus {
outline: none;
}
outline: none;
}
}

.mat-list-option {
&:not(.mat-list-item-disabled) {
cursor: pointer;
}
.mat-list-option:not(.mat-list-item-disabled) {
cursor: pointer;
outline: none;
}
15 changes: 5 additions & 10 deletions src/lib/list/selection-list.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,30 +251,25 @@ describe('MatSelectionList without forms', () => {
});

it('should focus next item when press DOWN ARROW', () => {
let testListItem = listOptions[2].nativeElement as HTMLElement;
let DOWN_EVENT: KeyboardEvent =
createKeyboardEvent('keydown', DOWN_ARROW, testListItem);
let manager = selectionList.componentInstance._keyManager;
const manager = selectionList.componentInstance._keyManager;

dispatchFakeEvent(listOptions[2].nativeElement, 'focus');
expect(manager.activeItemIndex).toEqual(2);

selectionList.componentInstance._keydown(DOWN_EVENT);

selectionList.componentInstance._keydown(createKeyboardEvent('keydown', DOWN_ARROW));
fixture.detectChanges();

expect(manager.activeItemIndex).toEqual(3);
});

it('should focus the first non-disabled item when pressing HOME', () => {
it('should be able to focus the first item when pressing HOME', () => {
const manager = selectionList.componentInstance._keyManager;
expect(manager.activeItemIndex).toBe(-1);

const event = dispatchKeyboardEvent(selectionList.nativeElement, 'keydown', HOME);
fixture.detectChanges();

// Note that the first item is disabled so we expect the second one to be focused.
expect(manager.activeItemIndex).toBe(1);
expect(manager.activeItemIndex).toBe(0);
expect(event.defaultPrevented).toBe(true);
});

Expand Down Expand Up @@ -425,7 +420,7 @@ describe('MatSelectionList without forms', () => {
fixture.detectChanges();

expect(selectionList.componentInstance.tabIndex)
.toBe(-1, 'Expected the tabIndex to be set to "-1" if selection list is disabled.');
.toBe(3, 'Expected the tabIndex to be still set to "3".');
});
});

Expand Down
32 changes: 18 additions & 14 deletions src/lib/list/selection-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,18 @@ import {
import {
CanDisable,
CanDisableRipple,
HasTabIndex,
MatLine,
MatLineSetter,
mixinDisabled,
mixinDisableRipple,
mixinTabIndex,
} from '@angular/material/core';
import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '@angular/forms';
import {Subscription} from 'rxjs/Subscription';


/** @docs-private */
export class MatSelectionListBase {}
export const _MatSelectionListMixinBase =
mixinTabIndex(mixinDisableRipple(mixinDisabled(MatSelectionListBase)));
export const _MatSelectionListMixinBase = mixinDisableRipple(mixinDisabled(MatSelectionListBase));

/** @docs-private */
export class MatListOptionBase {}
Expand Down Expand Up @@ -294,7 +291,7 @@ export class MatListOption extends _MatListOptionMixinBase
changeDetection: ChangeDetectionStrategy.OnPush
})
export class MatSelectionList extends _MatSelectionListMixinBase implements FocusableOption,
CanDisable, CanDisableRipple, HasTabIndex, AfterContentInit, ControlValueAccessor, OnDestroy {
CanDisable, CanDisableRipple, AfterContentInit, ControlValueAccessor, OnDestroy {

/** The FocusKeyManager which handles focus. */
_keyManager: FocusKeyManager<MatListOption>;
Expand All @@ -306,6 +303,9 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements Focu
@Output() readonly selectionChange: EventEmitter<MatSelectionListChange> =
new EventEmitter<MatSelectionListChange>();

/** Tabindex of the selection list. */
@Input() tabIndex: number = 0;

/** The currently selected options. */
selectedOptions: SelectionModel<MatListOption> = new SelectionModel<MatListOption>(true);

Expand All @@ -327,7 +327,12 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements Focu
}

ngAfterContentInit(): void {
this._keyManager = new FocusKeyManager<MatListOption>(this.options).withWrap().withTypeAhead();
this._keyManager = new FocusKeyManager<MatListOption>(this.options)
.withWrap()
.withTypeAhead()
// Allow disabled items to be focusable. For accessibility reasons, there must be a way for
// screenreader users, that allows reading the different options of the list.
.skipPredicate(() => false);

if (this._tempValues) {
this._setOptionsFromValues(this._tempValues);
Expand All @@ -354,7 +359,7 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements Focu
this._modelChanges.unsubscribe();
}

/** Focus the selection-list. */
/** Focuses the last active list option. */
focus() {
this._element.nativeElement.focus();
}
Expand Down Expand Up @@ -392,16 +397,15 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements Focu

/** Passes relevant key presses to our key manager. */
_keydown(event: KeyboardEvent) {
if (this.disabled) {
Copy link
Member

Choose a reason for hiding this comment

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

This check still has to apply to the ENTER and SPACE keypresses.

Copy link
Member Author

@devversion devversion Feb 25, 2018

Choose a reason for hiding this comment

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

Good point. Done

return;
}

switch (event.keyCode) {
case SPACE:
case ENTER:
this._toggleSelectOnFocusedOption();
// Always prevent space from scrolling the page since the list has focus
event.preventDefault();
if (!this.disabled) {
this._toggleSelectOnFocusedOption();

// Always prevent space from scrolling the page since the list has focus
event.preventDefault();
}
break;
case HOME:
case END:
Expand Down