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
Merged

DatePicker: Improve screenreader accessibility #1453

merged 18 commits into from Apr 18, 2017

Conversation

c-w
Copy link
Contributor

@c-w c-w 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 3 commits Apr 10, 2017
This is a fix for issue 4 mentioned in #1266
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>
Copy link
Member

@cschlechty cschlechty Apr 13, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@c-w c-w Apr 17, 2017

Choose a reason for hiding this comment

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

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'>
Copy link
Member

@cschlechty cschlechty Apr 13, 2017

Choose a reason for hiding this comment

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

Add a heading level if you're adding heading.

Copy link
Contributor Author

@c-w c-w Apr 17, 2017

Choose a reason for hiding this comment

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

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
Copy link
Member

@cschlechty cschlechty Apr 13, 2017

Choose a reason for hiding this comment

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

Why do we not use a true button?

Copy link
Contributor Author

@c-w c-w Apr 17, 2017

Choose a reason for hiding this comment

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

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'
Copy link
Member

@cschlechty cschlechty Apr 13, 2017

Choose a reason for hiding this comment

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

button implies we handle spacebar as well.

Copy link
Contributor Author

@c-w c-w Apr 17, 2017

Choose a reason for hiding this comment

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

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'>
Copy link
Member

@cschlechty cschlechty Apr 13, 2017

Choose a reason for hiding this comment

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

Why is this assertive?

Copy link
Contributor Author

@c-w c-w Apr 17, 2017

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@c-w c-w 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'>
Copy link
Member

@cschlechty cschlechty Apr 18, 2017

Choose a reason for hiding this comment

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

Can you use passive, instead?

Copy link
Contributor Author

@c-w c-w Apr 18, 2017

Choose a reason for hiding this comment

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

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 }
Copy link
Member

@cschlechty cschlechty Apr 18, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@c-w c-w Apr 18, 2017

Choose a reason for hiding this comment

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

Done in cd30b5d.

Copy link
Member

@cschlechty cschlechty left a comment

There's a few suggestions.

@cschlechty cschlechty merged commit 876d40b into microsoft:master Apr 18, 2017
1 check passed
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants