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

DatePicker: Improve screenreader accessibility #1453

Merged
merged 18 commits into from Apr 18, 2017

Conversation

Projects
None yet
4 participants
@c-w
Copy link
Contributor

commented Apr 10, 2017

Pull request checklist

  • Addresses an existing issue: #1266
  • Include a change request file if publishing
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Description of changes

The previous/next month/year buttons don't have aria-labels and therefore are indiscernible to screen reader users. This pull request fixes this by enabling consumers of the date-picker to provide aria-labels for the buttons.

This pull request also improves the navigation of the date picker for screen reader users by re-announcing the month and year whenever the user navigates across a month/year boundary. This behavior is adapted from some accessibility best practices laid out by the OAA.

Focus areas to test

Test steps (verified manually on DatePicker.Basic.Example):

  1. Open test site in Edge, turn on Narrator, navigate to the date picker via keyboard, and open the date picker.
  2. Navigate from the last day of a month to the first day of the next month. The new month should be read out by the screen reader.
  3. Navigate to the previous/next month/year buttons. The screen reader should read out "Go to {previous,next} {month,year}"

c-w added some commits Apr 10, 2017

Announce year/month when it changes
This is a fix for issue 4 mentioned in #1266
Add aria labels to date picker icon-navigation
This is a fix for issue 3 mentioned in #1266
<div className={ css('ms-DatePicker-month', styles.month) }>{ strings.months[navigatedDate.getMonth()] }</div>
<div className={ css('ms-DatePicker-year', styles.year) }>{ navigatedDate.getFullYear() }</div>
<div role='heading' id={ monthAndYearId } aria-live='assertive' aria-relevant='text' aria-atomic='true'>
<div className={ css('ms-DatePicker-month', styles.month) }>{ strings.months[navigatedDate.getMonth()] }</div>

This comment has been minimized.

Copy link
@cschlechty

cschlechty Apr 13, 2017

Collaborator

Does this localize well? Won't it be wrong in EMEA?

This comment has been minimized.

Copy link
@c-w

c-w Apr 17, 2017

Author Contributor

I agree, the localization approach taken here is likely not ideal. Fixing that is beyond the scope of this pull-request though: I'd just like to focus on the accessibility improvements and leave the localization logic as it currently stands.


return (
<div className={ css('ms-DatePicker-dayPicker', styles.dayPicker) }>
<div className={ css('ms-DatePicker-header', styles.header) }>
<div className={ css('ms-DatePicker-month', styles.month) }>{ strings.months[navigatedDate.getMonth()] }</div>
<div className={ css('ms-DatePicker-year', styles.year) }>{ navigatedDate.getFullYear() }</div>
<div role='heading' id={ monthAndYearId } aria-live='assertive' aria-relevant='text' aria-atomic='true'>

This comment has been minimized.

Copy link
@cschlechty

cschlechty Apr 13, 2017

Collaborator

Add a heading level if you're adding heading.

This comment has been minimized.

Copy link
@c-w

c-w Apr 17, 2017

Author Contributor

We have no way of knowing what the correct level here would be, so I removed the role.

<div role='heading' id={ monthAndYearId } aria-live='assertive' aria-relevant='text' aria-atomic='true'>
<div className={ css('ms-DatePicker-month', styles.month) }>{ strings.months[navigatedDate.getMonth()] }</div>
<div className={ css('ms-DatePicker-year', styles.year) }>{ navigatedDate.getFullYear() }</div>
</div>
</div>
<div className={ css('ms-DatePicker-monthComponents', styles.monthComponents) }>
<div className={ css('ms-DatePicker-navContainer', styles.navContainer) }>
<span

This comment has been minimized.

Copy link
@cschlechty

cschlechty Apr 13, 2017

Collaborator

Why do we not use a true button?

This comment has been minimized.

Copy link
@c-w

c-w Apr 17, 2017

Author Contributor

This was here before my pull-request, so that's a question for the original author: @dzearing.

</div>
<div className={ css('ms-DatePicker-monthComponents', styles.monthComponents) }>
<div className={ css('ms-DatePicker-navContainer', styles.navContainer) }>
<span
className={ css('ms-DatePicker-prevMonth js-prevMonth', styles.prevMonth) }
onClick={ this._onSelectPrevMonth }
onKeyDown={ this._onPrevMonthKeyDown }
aria-controls={ monthAndYearId }
aria-label={ strings.nextMonthAriaLabel }
role='button'

This comment has been minimized.

Copy link
@cschlechty

cschlechty Apr 13, 2017

Collaborator

button implies we handle spacebar as well.

This comment has been minimized.

Copy link
@c-w

c-w Apr 17, 2017

Author Contributor

Done in 12c53ed.


return (
<div className={ css('ms-DatePicker-dayPicker', styles.dayPicker) }>
<div className={ css('ms-DatePicker-header', styles.header) }>
<div className={ css('ms-DatePicker-month', styles.month) }>{ strings.months[navigatedDate.getMonth()] }</div>
<div className={ css('ms-DatePicker-year', styles.year) }>{ navigatedDate.getFullYear() }</div>
<div role='heading' id={ monthAndYearId } aria-live='assertive' aria-relevant='text' aria-atomic='true'>

This comment has been minimized.

Copy link
@cschlechty

cschlechty Apr 13, 2017

Collaborator

Why is this assertive?

This comment has been minimized.

Copy link
@c-w

c-w Apr 17, 2017

Author Contributor

Making the month display assertive makes the control much nicer to use with a screen reader: when navigating between a month boundary (e.g. from the 30th of one month to the 1st of the next month), we'll re-announce the month's name which provides more context to the user. It's better to hear "1 January 2017" than just "1".

@c-w

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2017

@cschlechty: I addressed your comments -- could you please take another look?


return (
<div className={ css('ms-DatePicker-dayPicker', styles.dayPicker) }>
<div className={ css('ms-DatePicker-header', styles.header) }>
<div className={ css('ms-DatePicker-month', styles.month) }>{ strings.months[navigatedDate.getMonth()] }</div>
<div className={ css('ms-DatePicker-year', styles.year) }>{ navigatedDate.getFullYear() }</div>
<div id={ monthAndYearId } aria-live='assertive' aria-relevant='text' aria-atomic='true'>

This comment has been minimized.

Copy link
@cschlechty

cschlechty Apr 18, 2017

Collaborator

Can you use passive, instead?

This comment has been minimized.

Copy link
@c-w

c-w Apr 18, 2017

Author Contributor

Done in 0e1ca9a.

</div>
<div className={ css('ms-DatePicker-monthComponents', styles.monthComponents) }>
<div className={ css('ms-DatePicker-navContainer', styles.navContainer) }>
<span
className={ css('ms-DatePicker-prevMonth js-prevMonth', styles.prevMonth) }
onClick={ this._onSelectPrevMonth }
onKeyDown={ this._onPrevMonthKeyDown }
aria-controls={ monthAndYearId }

This comment has been minimized.

Copy link
@cschlechty

cschlechty Apr 18, 2017

Collaborator

aria-controls specifies the html it has control over. shouldn't this point to the body of the calendar?

This comment has been minimized.

Copy link
@c-w

c-w Apr 18, 2017

Author Contributor

Done in cd30b5d.

@cschlechty
Copy link
Collaborator

left a comment

There's a few suggestions.

c-w added some commits Apr 18, 2017

@cschlechty cschlechty merged commit 876d40b into OfficeDev:master Apr 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.