Skip to content

fix(datepicker): make date range aria-live and fix active date logic #11144

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 7, 2018
Merged

fix(datepicker): make date range aria-live and fix active date logic #11144

merged 3 commits into from
Jun 7, 2018

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented May 3, 2018

fixes #11128
fixes #11129
fixes #11132

@mmalerba mmalerba added pr: needs review Accessibility This issue is related to accessibility (a11y) labels May 3, 2018
@mmalerba mmalerba requested review from jelbourn and crisbeto May 3, 2018 23:16
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 3, 2018
@jelbourn
Copy link
Member

jelbourn commented May 3, 2018

Which screen-readers did you try this in?

@mmalerba
Copy link
Contributor Author

mmalerba commented May 4, 2018

Only on ChromeVox. Now that you mentioned it I tried VoiceOver and it doesn't seem to respect the aria-live=polite. Any ideas? Should I do assertive instead?

@jelbourn
Copy link
Member

jelbourn commented May 4, 2018

Try using the LiveAnnouncer? The reason we have that is because there are reportedly issues with using more than one aria-live region on some environments

@mmalerba mmalerba added the blocked This issue is blocked by some external factor, such as a prerequisite PR label May 4, 2018
@mmalerba
Copy link
Contributor Author

mmalerba commented May 4, 2018

Blocked on some other PRs I'm doing to enhance LiveAnnouncer

@mmalerba mmalerba mentioned this pull request May 15, 2018
Merged
@mmalerba mmalerba added the target: minor This PR is targeted for the next minor release label May 15, 2018
@mmalerba
Copy link
Contributor Author

marking this as target: minor since it will wind up depending on features only available in the next minor

@mmalerba mmalerba removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label May 24, 2018
@mmalerba
Copy link
Contributor Author

@jelbourn switched to cdkAriaLive and tested on ChromeVox, JAWS, NVDA, VoiceOver - seems to work on all of them now.

jelbourn
jelbourn reviewed May 31, 2018
View reviewed changes
src/lib/datepicker/calendar-header.html
@@ -1,9 +1,11 @@
<div class="mat-calendar-header">
<div class="mat-calendar-controls">
<button mat-button type="button" class="mat-calendar-period-button"
(click)="currentPeriodClicked()" [attr.aria-label]="periodButtonLabel">
(click)="currentPeriodClicked()" [attr.aria-label]="periodButtonLabel"
cdkAriaLive="polite">
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed this now, but we should make cdkAriaLive default to polite in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created #11618 so we don't forget

src/lib/datepicker/multi-year-view.ts
@@ -101,6 +101,9 @@ export class MatMultiYearView<D> implements AfterContentInit {
/** Emits the selected year. This doesn't imply a change on the selected date */
@Output() readonly yearSelected: EventEmitter<D> = new EventEmitter<D>();

/** Emits when any date is activated. */
@Output() readonly activeDateChange: EventEmitter<D> = new EventEmitter<D>();
Copy link
Member

Choose a reason for hiding this comment

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

Add unit test for these new outputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the tests to work the same way as month-view tests, which test the @Output()

@mmalerba
Copy link
Contributor Author

mmalerba commented Jun 1, 2018

@jelbourn PTAL

jelbourn
jelbourn approved these changes Jun 1, 2018
View reviewed changes
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.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jun 1, 2018
@andrewseguin andrewseguin merged commit 8063c26 into angular:master Jun 7, 2018
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@crisbeto crisbeto Awaiting requested review from crisbeto

Assignees

@jelbourn jelbourn

@crisbeto crisbeto

Labels
Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
5 participants
@mmalerba @jelbourn @crisbeto @googlebot @andrewseguin