Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(chips): Add basic accessibility support. #9650

Merged
merged 1 commit into from
Jan 4, 2017

Conversation

topherfangio
Copy link
Contributor

@topherfangio topherfangio commented Sep 20, 2016

Previously, chips had no, or very broken, support for accessibility.

Make many updates to fix broken functionality and add required features.

  • Remove existing aria-hidden attributes.
  • Dynamically apply required role and aria-owns attributes.
  • Ensure tabindex is set properly on all elements. Particularly,
    you can now tab to, and navigate through, a set of chips in
    readonly mode.
  • Provide new input-aria-label option to identify the inputs.
  • Provide new container-hint option used when the element is in
    readonly mode.
  • Provide new md-chip-append-delay option which controls the delay
    (in ms) before a user can add a new chip (fixes screen readers).
  • Fix issue with delete-hint never being read by screen readers
    because it was outside of the chip.
  • Fix keyboard navigation to properly wrap when moving both left
    or right.
  • Fix keyboard navigation when in readonly mode.
  • Fix issue where the wrong chip may be focused because the old
    chip was still in the DOM.
  • Fix issue where onAdd callback passed incorrect $index (it
    was 1-based instead of 0-based).

Additionally update the Chips docs with new Accessibility info and
a few other minor changes to the docs/demos.

  • Remove the "WORK IN PROGRESS" notice.
  • Fix mispelling of "maximum" on first chips demo.
  • Fix Contact Chips demo filtering.
  • Fix Contact Chips demo images to use new/faster service.

BREAKING CHANGES

MAJOR

In order to make the chips fully accessible and work properly across
a wide variety of screen readers, we have added a 300ms delay after
appending a chip before we re-focus the input.

This is necessary because some screen readers change operational modes
when the enter key is pressed inside of an input and this causes the
screen reader to move into "navigation" mode and no longer apply
keystrokes to the input.

Additionally, this ensures that the newly added chip is properly
announced to the screen readers.

You may alter this value with the new md-chip-append-delay
attribute, however using a value less than 300 can cause issues
on JAWS and NVDA and will make your application inaccessible to those
users.

Note 1: This issue only seems to affect chips appended when using the
enter key. If you override the md-separator-keys and disable the
enter key (and enable something like , or ;), you may be able
to reduce this delay to 0 and achieve past functionality.

Note 2: This issue does not appear to affect VoiceOver or ChromeVox,
so if you are only targeting those users, you may be able to reduce
the delay to 0.

MINOR

We consider the following to be minor breaking changes since we never
expected these attributes/elements to be utilized by developers.

Nonetheless, we want to ensure that they are documented.

  • The role of the .md-chip-content has been modified from button
    to option so that it works with the new listbox role of it's
    parent.

    If you rely on this role being button, you should update your code
    accordingly.

  • The delete hint on removable chips now resides inside of the
    div.md-chip-content rather than the parent md-chip element where
    it could not be read by screen readers.

    If you interact with this element (in CSS or JS) please update your
    selectors/code to account for the new DOM hierarchy.

Fixes #9391. Fixes #9556. Fixes #8897. Fixes #8867. Fixes #9649.

@topherfangio topherfangio added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: manual testing This issue or PR needs to have some manual testing and verification done needs: unit tests This PR needs unit tests to cover the changes being proposed labels Sep 20, 2016
@topherfangio topherfangio added this to the 1.1.2 milestone Sep 20, 2016
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Tested with JAWS, here's what I found:

  • In IE11 and Firefox, once you add a new chip you get into a weird state where keyboard input (arrow keys, typing into the input) do nothing. You have to tab and then tab back to make it work again.
  • In Edge, nothing is announced. However, the website for JAWS says not to use it with Edge yet, so the ball is probably in their court.
  • Everything is great in Chrome
  • In IE11, when I add a new chip, JAWS announces "{{$chip}}" literally.

ctrl.wrapperId = '_md-chips-wrapper-' + ctrl.$mdUtil.nextUid();

// Setup a watcher which manages the role and aria-owns attributes
ctrl.$scope.$watch('$mdChipsCtrl.items.length', function() {
Copy link
Member

@jelbourn jelbourn Sep 21, 2016

Choose a reason for hiding this comment

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

$watchCollection is generally better than $watch for array length because it's possible to add and remove the same number of items in one cycle.

ctrl.$scope.$watch('$mdChipsCtrl.items.length', function() {
if (ctrl.items && ctrl.items.length) {
// Dynamically add the listbox role
wrapper.attr('role', 'listbox');
Copy link
Member

@jelbourn jelbourn Sep 21, 2016

Choose a reason for hiding this comment

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

Add a comment like "Add the listbox on every change because it must be removed when there are no items."

tabindex="{{$mdChipsCtrl.ariaTabIndex == $index ? 0 : -1}}"\
id="{{$mdChipsCtrl.contentIdFor($index)}}"\
role="option"\
aria-live="assertive"\
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if aria-live is a good idea here. Generally you only want one live region for the whole app.

@marcysutton do you have any thoughts on this?

@@ -5,7 +5,7 @@ <h2 class="md-title">Use a custom chip template.</h2>

<form name="fruitForm">
<md-chips ng-model="ctrl.roFruitNames" name="fruitName" readonly="ctrl.readonly"
md-removable="ctrl.removable" md-max-chips="5">
md-removable="ctrl.removable" md-max-chips="5" placeholder="Enter a fruit...">
<md-chip-template>
<strong>{{$chip}}</strong>
Copy link
Member

@jelbourn jelbourn Sep 21, 2016

Choose a reason for hiding this comment

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

In JAWS w/ IE11, upon adding a new chip, the screen reader announces "Left brace left brace dollar chip right brace right brace". Is the template being added to the dom before being linked?

@marcysutton
Copy link
Contributor

marcysutton commented Sep 22, 2016

IE11 and JAWS is a known issue that may be fixed by using ng-bind: angular/angular.js#15079

It works in Chrome and what AT? You should try Safari and Voiceover. NVDA is typically used with Firefox, and Edge is best supported with Narrator. Mobile Safari and Voiceover would be good to check as well. For Firefox and JAWS, I'll have to take a closer look when I'm in front of my computer. So many AT's, it can be tough I know.

It is a good idea to limit use of aria-live to specific announcer elements to avoid conflicting messages. Too many similar messages will also get swallowed in JAWS, so we often use two for each politeness level. You can see this implemented in ngA11y. https://github.com/dequelabs/ngA11y/blob/master/src/nga11yannounce.js

@ThomasBurleson
Copy link
Contributor

@topherfangio ping to finish.

@topherfangio topherfangio force-pushed the fix-chips-a11y-9391 branch 2 times, most recently from b2d6596 to 1217121 Compare October 10, 2016 17:39
@topherfangio topherfangio force-pushed the fix-chips-a11y-9391 branch 2 times, most recently from 184467c to f0d883b Compare October 20, 2016 16:55
@topherfangio
Copy link
Contributor Author

@jelbourn After discussion with team and @marcysutton, we have added a 300ms delay after adding the chips to fix some JAWS/NVDA issues.

The latest updates include this change as well as all of the related documentation. Can you review this change and the new docs?

Also, I have tested in again JAWS but only have 40 minute mode so it was pretty basic. Can you please re-test and see if you have any additional issues?

I still want to add a few test and hopefully remove one TODO, but this is pretty close to final.

@topherfangio topherfangio added the needs: review This PR is waiting on review from the team label Oct 20, 2016
@ThomasBurleson
Copy link
Contributor

@jelbourn - ping for review.

@ThomasBurleson ThomasBurleson added needs: presubmit and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: manual testing This issue or PR needs to have some manual testing and verification done needs: review This PR is waiting on review from the team needs: unit tests This PR needs unit tests to cover the changes being proposed labels Nov 29, 2016
@jelbourn
Copy link
Member

jelbourn commented Dec 2, 2016

@topherfangio can you rebase?

Previously, chips had no, or very broken, support for accessibility.

Make many updates to fix broken functionality and add required features.

 - Remove existing `aria-hidden` attributes.

 - Dynamically apply required `role` and `aria-owns` attributes.

 - Ensure `tabindex` is set properly on all elements. Particularly,
   you can now tab to, and navigate through, a set of chips in
   readonly mode.

 - Provide new `input-aria-label` option to identify the inputs.

 - Provide new `container-hint` option used when the element is in
   readonly mode.

 - Provide new `md-chip-append-delay` option which controls the delay
   (in ms) before a user can add a new chip (fixes screen readers).

 - Fix issue with `delete-hint` never being read by screen readers
   because it was outside of the chip.

 - Fix keyboard navigation to properly wrap when moving both left
   or right.

 - Fix keyboard navigation when in readonly mode.

 - Fix issue where the wrong chip may be focused because the old
   chip was still in the DOM.

 - Fix issue where `onAdd` callback passed incorrect `$index` (it
   was 1-based instead of 0-based).

Additionally update the Chips docs with new Accessibility info and
a few other minor changes to the docs/demos.

 - Remove the "WORK IN PROGRESS" notice.

 - Fix mispelling of "maximum" on first chips demo.

 - Fix Contact Chips demo filtering.

 - Fix Contact Chips demo images to use new/faster service.

BREAKING CHANGES

**MAJOR**

In order to make the chips fully accessible and work properly across
a wide variety of screen readers, we have added a 300ms delay after
appending a chip before we re-focus the input.

This is necessary because some screen readers change operational modes
when the enter key is pressed inside of an input and this causes the
screen reader to move into "navigation" mode and no longer apply
keystrokes to the input.

Additionally, this ensures that the newly added chip is properly
announced to the screen readers.

You **may** alter this value with the new `md-chip-append-delay`
attribute, however using a value less than `300` can cause issues
on JAWS and NVDA and will make your application inaccessible to those
users.

Note 1: This issue only seems to affect chips appended when using the
`enter` key. If you override the `md-separator-keys` and disable the
`enter` key (and enable something like `,` or `;`), you may be able
to reduce this delay to `0` and achieve past functionality.

Note 2: This issue does not appear to affect VoiceOver or ChromeVox,
so if you are only targeting those users, you may be able to reduce
the delay to `0`.

**MINOR**

We consider the following to be minor breaking changes since we never
expected these attributes/elements to be utilized by developers.

Nonetheless, we want to ensure that they are documented.

 - The `role` of the `.md-chip-content` has been modified from `button`
   to `option` so that it works with the new `listbox` role of it's
   parent.

   If you rely on this role being `button`, you should update your code
   accordingly.

- The delete hint on removable chips now resides inside of the
  `div.md-chip-content` rather than the parent `md-chip` element where
  it could not be read by screen readers.

  If you interact with this element (in CSS or JS) please update your
  selectors/code to account for the new DOM hierarchy.

Fixes angular#9391. Fixes angular#9556. Fixes angular#8897. Fixes angular#8867. Fixes angular#9649.
@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jan 4, 2017
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.2, 1.1.3 Jan 4, 2017
@ThomasBurleson ThomasBurleson removed this from the 1.1.2 milestone Jan 4, 2017
@kara kara added pr: merge ready This PR is ready for a caretaker to review and removed needs: presubmit labels Jan 4, 2017
@kara kara merged commit f18cb2b into angular:master Jan 4, 2017
davidenochk pushed a commit to davidenochk/material that referenced this pull request Feb 3, 2017
@Splaktar Splaktar modified the milestones: 1.1.3, 1.1.2 May 27, 2018
Splaktar added a commit that referenced this pull request May 30, 2018
this solves a regression introduced in 1.1.2 by #9650
change `ng-change` demo to use ``$log` instead of `alert`
fix some lint line length warnings
JSDoc clean up

Fixes #10758
Splaktar added a commit that referenced this pull request May 30, 2018
this solves a regression introduced in 1.1.2 by #9650
change `ng-change` demo to use `$log` instead of `alert`
fix some lint line length warnings
JSDoc clean up

Fixes #10758
Splaktar added a commit that referenced this pull request May 30, 2018
this solves a regression introduced in 1.1.2 by #9650
change `ng-change` demo to use `$log` instead of `alert`
fix some lint line length warnings
JSDoc clean up

Fixes #10758
Splaktar added a commit that referenced this pull request Jun 17, 2018
this solves a regression introduced in 1.1.2 by #9650
change `ng-change` demo to use `$log` instead of `alert`
fix some lint line length warnings
JSDoc clean up

Fixes #10758
jelbourn pushed a commit that referenced this pull request Jun 22, 2018
this solves a regression introduced in 1.1.2 by #9650
change `ng-change` demo to use `$log` instead of `alert`
fix some lint line length warnings
JSDoc clean up

Fixes #10758
Splaktar added a commit that referenced this pull request Jul 31, 2018
this solves a regression introduced in 1.1.2 by #9650
change `ng-change` demo to use `$log` instead of `alert`
fix some lint line length warnings
JSDoc clean up

Fixes #10758
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
7 participants