fix(selection-list): improve accessibility of selection list #10137

devversion · 2018-02-24T18:57:31Z

Since the selection list is still a list that contains some content, there should be a way for screenreader users, to navigate through the options (even if disabled). With this change, if the list is disabled, it's still possible to walk through the options.
Adds a new functionality to the ListKeyManager that allows the developer to control the items that can't be focused. skipPredicate. (e.g. for the selection list we want to make sure that users can navigate using the arrow keys to disabled items as well)
Testing: Fixes that by default all fake events bubble up the DOM. This works in most of the cases, but it's wrong to always bubble events. e.g. focus doesn't bubble.

crisbeto · 2018-02-25T09:27:28Z

src/cdk/a11y/key-manager/list-key-manager.spec.ts

+        expect(keyManager.activeItemIndex).toBe(2);
+      });
+
+      it('should be able to not skip any item', () => {


This is already covered by the custom predicate test.

crisbeto · 2018-02-25T09:31:59Z

src/cdk/a11y/key-manager/list-key-manager.spec.ts

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


I think the item here is a FakeFocusable. Alternatively you could let TS infer the type itself.

crisbeto · 2018-02-25T09:34:01Z

src/cdk/a11y/key-manager/list-key-manager.ts

+   * Predicate function that determines items that should be skipped by the list key manager.
+   * By default, disabled items are skipped by the key manager.
+   */
+  skipPredicate(predicateFn: (item: T) => boolean): this {


Needs an @param for the predicate. Also it might be better to call it predicate, because the consumer can see that it's a function by looking at the type.

crisbeto · 2018-02-25T09:34:12Z

src/cdk/a11y/key-manager/list-key-manager.ts

+   * 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;


Call this one skipPredicate?

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

crisbeto · 2018-02-25T09:35:21Z

src/cdk/testing/event-objects.ts

@@ -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) {


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.

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

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.

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.

crisbeto · 2018-02-25T09:38:15Z

src/lib/list/selection-list.ts

    'class': 'mat-selection-list',
    '(focus)': 'focus()',
    '(blur)': '_onTouched()',
    '(keydown)': '_keydown($event)',
+    '[attr.tabindex]': 'disabled ? 0 : tabIndex',


Shouldn't this be just tabIndex? Either way the list will be focusable, unless the consumer passed in -1, in which case this will make it focusable only when disabled.

With -1 the selection list won't be tabbable anymore, but it should be tabbable if disabled for a11y.

What I meant was that with this setup, if the consumer set it to -1 explicitly, it would only be tabbable when it's disabled which seems wrong.

crisbeto · 2018-02-25T09:40:11Z

src/lib/list/selection-list.ts

@@ -392,10 +406,6 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements Focu

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


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

Good point. Done

devversion · 2018-02-25T16:49:45Z

@crisbeto Done. Addressed most of your comments.

crisbeto

LGTM, left a couple more nits.

crisbeto · 2018-02-25T18:01:51Z

src/lib/list/selection-list.ts

        // Always prevent space from scrolling the page since the list has focus
        event.preventDefault();
+
+        if (!this.disabled) {
+          this._toggleSelectOnFocusedOption();


I'd move the preventDefault into the check as well. That's what it used to do before.

crisbeto · 2018-02-25T18:02:48Z

src/cdk/a11y/key-manager/list-key-manager.ts

@@ -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>();

+  /**
+   * Predicate function that determines items that should be skipped by the list key manager.
+   * By default, disabled items are skipped by the key manager.


This sounds like it's describing the predicate argument, instead of what the method does. Consider changing it to something like "Sets the predicate function that the determines..."?

* Since the selection list is still a `list` that contains some content, there should be a way for screenreader users, to navigate through the options (even if disabled). With this change, if the list is disabled, it's still possible to walk through the options. * Adds a new functionality to the `ListKeyManager` that allows the developer to control the items that can't be focused. `skipPredicate`. (e.g. for the selection list we want to make sure that users can navigate using the arrow keys to disabled items as well) * Testing: Fixes that by default all fake events bubble up the DOM. This works in most of the cases, but it's wrong to always bubble events. e.g. `focus` doesn't bubble. Fixes angular#9995

* Since the selection list is still a `list` that contains some content, there should be a way for screenreader users, to navigate through the options (even if disabled). With this change, if the list is disabled, it's still possible to walk through the options. * Adds a new functionality to the `ListKeyManager` that allows the developer to control the items that can't be focused. `skipPredicate`. (e.g. for the selection list we want to make sure that users can navigate using the arrow keys to disabled items as well) * Testing: Fixes that by default all fake events bubble up the DOM. This works in most of the cases, but it's wrong to always bubble events. e.g. `focus` doesn't bubble. Fixes #9995

angular-automatic-lock-bot · 2019-09-08T10:02:10Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

devversion added the pr: needs review label Feb 24, 2018

devversion assigned jelbourn and crisbeto Feb 24, 2018

devversion requested review from crisbeto and jelbourn as code owners February 24, 2018 18:57

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 24, 2018

devversion added the Accessibility This issue is related to accessibility (a11y) label Feb 24, 2018

crisbeto reviewed Feb 25, 2018

View reviewed changes

devversion force-pushed the fix/a11y-selection-list branch 2 times, most recently from a625f70 to 9cb81dc Compare February 25, 2018 16:48

crisbeto approved these changes Feb 25, 2018

View reviewed changes

crisbeto added pr: lgtm and removed pr: needs review labels Feb 25, 2018

devversion force-pushed the fix/a11y-selection-list branch from 9cb81dc to ca8273f Compare February 25, 2018 18:33

devversion added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Feb 25, 2018

tinayuangao merged commit 51fce51 into angular:master Feb 28, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(selection-list): improve accessibility of selection list #10137

fix(selection-list): improve accessibility of selection list #10137

devversion commented Feb 24, 2018 •

edited

Loading

crisbeto Feb 25, 2018

crisbeto Feb 25, 2018

devversion Feb 25, 2018

crisbeto Feb 25, 2018 •

edited

Loading

devversion Feb 25, 2018

crisbeto Feb 25, 2018

devversion Feb 25, 2018

crisbeto Feb 25, 2018

devversion Feb 25, 2018 •

edited

Loading

crisbeto Feb 25, 2018

devversion Feb 25, 2018

crisbeto Feb 25, 2018

devversion Feb 25, 2018

crisbeto Feb 25, 2018

crisbeto Feb 25, 2018

devversion Feb 25, 2018 •

edited

Loading

devversion commented Feb 25, 2018 •

edited

Loading

crisbeto left a comment

crisbeto Feb 25, 2018

crisbeto Feb 25, 2018

angular-automatic-lock-bot bot commented Sep 8, 2019

fix(selection-list): improve accessibility of selection list #10137

fix(selection-list): improve accessibility of selection list #10137

Conversation

devversion commented Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devversion Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devversion Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

devversion commented Feb 25, 2018 • edited Loading

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 8, 2019

devversion commented Feb 24, 2018 •

edited

Loading

crisbeto Feb 25, 2018 •

edited

Loading

devversion Feb 25, 2018 •

edited

Loading

devversion Feb 25, 2018 •

edited

Loading

devversion commented Feb 25, 2018 •

edited

Loading