-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that we should leave this one as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can also check for the type and just default to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
||
const event = document.createEvent('Event'); | ||
event.initEvent(type, canBubble, cancelable); | ||
return event; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 {} | ||
|
@@ -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>; | ||
|
@@ -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); | ||
|
||
|
@@ -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); | ||
|
@@ -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(); | ||
} | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check still has to apply to the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
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.
Call this one
skipPredicate
?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.
This would conflict with the function name that sets the predicate. We could name it
_skipPredicate
, but having theFn
suffix feels appropriate IMO.