-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
fix(datepicker): make datepicker work with screen readers #4349
Conversation
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
@@ -1,19 +1,23 @@ | |||
<!-- If there's not enough space in the first row, create a separate label row. --> | |||
<tr *ngIf="_firstRowOffset < labelMinRequiredCells"> | |||
<tr *ngIf="_firstRowOffset < labelMinRequiredCells" aria-hidden="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment mentioning why this is aria-hidden
src/lib/datepicker/calendar-body.ts
Outdated
constructor(public value: number, public displayValue: string, public enabled: boolean) {} | ||
constructor(public value: number, | ||
public displayValue: string, | ||
public a11yLabel: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ariaLabel
?
src/lib/datepicker/calendar.html
Outdated
@@ -15,7 +15,7 @@ | |||
</div> | |||
</div> | |||
|
|||
<div class="mat-calendar-content" tabindex="0" (keydown)="_handleCalendarBodyKeydown($event)" | |||
<div class="mat-calendar-content" tabindex="-1" (keydown)="_handleCalendarBodyKeydown($event)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this receives focus, should it have a role? What does the screen-reader say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was actually immediately shifting its focus to one of the gridcell
, so it was never focused for long enough to be read. I refactored to cut out the middleman though, and just directly focus the gridcell
now
src/lib/datepicker/calendar.ts
Outdated
@@ -207,6 +205,22 @@ export class MdCalendar<D> implements AfterContentInit { | |||
} | |||
} | |||
|
|||
/** Focuses the active cell after the microtask queue is empty. */ | |||
_focusActiveCell() { | |||
this._ngZone.onMicrotaskEmpty.first().subscribe(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good place for runOutsideAngular
, since otherwise the running of this microtask will cause another round of change detection
this._ngZone.runOutsideAngular(() => {
Promise.resolve().then(() => {
// ...
});
});
src/lib/datepicker/calendar.ts
Outdated
@@ -244,14 +258,15 @@ export class MdCalendar<D> implements AfterContentInit { | |||
case ENTER: | |||
if (this._dateFilterForViews(this._activeDate)) { | |||
this._dateSelected(this._activeDate); | |||
break; | |||
event.preventDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment about the default you're preventing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There wasn't anything I ran into, it was more of just a precaution, should I remove it?
@@ -3,7 +3,7 @@ $mat-datepicker-toggle-icon-size: 24px !default; | |||
|
|||
.mat-datepicker-toggle { | |||
display: inline-block; | |||
background: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="24px" height="24px" viewBox="0 0 24 24" fill="currentColor"><path d="M0 0h24v24H0z" fill="none"/><path d="M19 3h-1V1h-2v2H8V1H6v2H5c-1.11 0-1.99.9-1.99 2L3 19c0 1.1.89 2 2 2h14c1.1 0 2-.9 2-2V5c0-1.1-.9-2-2-2zm0 16H5V8h14v11zM7 10h5v5H7z"/></svg>') no-repeat; | |||
background: url('data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIyNHB4IiBoZWlnaHQ9IjI0cHgiIHZpZXdCb3g9IjAgMCAyNCAyNCIgZmlsbD0iY3VycmVudENvbG9yIj48cGF0aCBkPSJNMCAwaDI0djI0SDB6IiBmaWxsPSJub25lIi8+PHBhdGggZD0iTTE5IDNoLTFWMWgtMnYySDhWMUg2djJINWMtMS4xMSAwLTEuOTkuOS0xLjk5IDJMMyAxOWMwIDEuMS44OSAyIDIgMmgxNGMxLjEgMCAyLS45IDItMlY1YzAtMS4xLS45LTItMi0yem0wIDE2SDVWOGgxNHYxMXpNNyAxMGg1djVIN3oiLz48L3N2Zz4=') no-repeat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to base-64 encode this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems IE11 doesn't like the utf8 version, the image didn't show up at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add a comment then mentioning that
src/lib/datepicker/datepicker.ts
Outdated
/** | ||
* Handles keydown event on datepicker content. | ||
* @param event The event. | ||
* @private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need @private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -3,7 +3,7 @@ $mat-datepicker-toggle-icon-size: 24px !default; | |||
|
|||
.mat-datepicker-toggle { | |||
display: inline-block; | |||
background: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="24px" height="24px" viewBox="0 0 24 24" fill="currentColor"><path d="M0 0h24v24H0z" fill="none"/><path d="M19 3h-1V1h-2v2H8V1H6v2H5c-1.11 0-1.99.9-1.99 2L3 19c0 1.1.89 2 2 2h14c1.1 0 2-.9 2-2V5c0-1.1-.9-2-2-2zm0 16H5V8h14v11zM7 10h5v5H7z"/></svg>') no-repeat; | |||
background: url('data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIyNHB4IiBoZWlnaHQ9IjI0cHgiIHZpZXdCb3g9IjAgMCAyNCAyNCIgZmlsbD0iY3VycmVudENvbG9yIj48cGF0aCBkPSJNMCAwaDI0djI0SDB6IiBmaWxsPSJub25lIi8+PHBhdGggZD0iTTE5IDNoLTFWMWgtMnYySDhWMUg2djJINWMtMS4xMSAwLTEuOTkuOS0xLjk5IDJMMyAxOWMwIDEuMS44OSAyIDIgMmgxNGMxLjEgMCAyLS45IDItMlY1YzAtMS4xLS45LTItMi0yem0wIDE2SDVWOGgxNHYxMXpNNyAxMGg1djVIN3oiLz48L3N2Zz4=') no-repeat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add a comment then mentioning that
38d86dd
to
cdced64
Compare
CLAs look good, thanks! |
* first pass a11y * escape key support * month labels * don't steal focus from fwd/back buttons * fix tests * address some comments * fix lint
* first pass a11y * escape key support * month labels * don't steal focus from fwd/back buttons * fix tests * address some comments * fix lint
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
tested with JAWS/IE11 (temporary demo up at: https://mmalerba-demo1.firebaseapp.com/datepicker)