-
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
Calendar navigation #24124
Calendar navigation #24124
Conversation
Makes changes to the DOM structure of calendar cells for better screen reader experience. Previously, the DOM structure looksed like this: ``` <!-- Existing DOM structure of each calendar body cell --> <td role="gridcell" aria-disabled="false" aria-current="date" aria-selected="true" <!-- ... --> > <!-- additional details ommited --> </> ``` Using the `gridcell` role allows screenreaders to use table specific navigation and some screenreaders would announce that the cells are interactible because of the presence of `aria-selected`. However, some screenreaders did not announce the cells as interactable and treated it the same as a cell in a static table (e.g. VoiceOver announces element type incorrectly angular#23476). This changes the DOM structure to nest buttons inside of a gridcell to make it more explicit that the table cells can be interacted with and are not static content. The gridcell role is still present, so table navigation will continue to work, but the interaction is done with buttons nested inside the `td` elements. The `td` element is only for adding information to the a11y tree and not used for visual purposes. Updated DOM structure: ``` <td role="gridcell" style="display: contents;" > <div role="button" aria-disabled="false" aria-current="date" aria-selected="true" <!-- ... --> > <!-- additional details ommited --> </div> </td> ``` Fixes angular#23476
When a a date cell on the calendar recieves focus, set the active date to that cell. This ensures that the active date matches the date with browser focus. Previously, we set the active date on keydown and click, but that was problematic for screenreaders. That's because many screenreaders trigger a focus event instead of a keydown event when using screenreader specific navigation (VoiceOver, Chromevox, NVDA). Fixes angular#23483
--> | ||
<td *ngFor="let item of row; let colIndex = index" role="gridcell" class="mat-calendar-body-cell-wrapper"> | ||
<div | ||
role="button" |
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 don't we use a button
element instead of a div
with a role
?
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.
For the draft implementation of it, it was the path of least resistance to use div role="button"
because it doesn't affect the visual styles. We would need to override a few (not sure exactly how many) style rules to get a button
element to look correct.
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's only 5-6 lines of CSS so it might be easier to do it now https://github.com/angular/components/blob/master/src/material/core/style/_button-common.scss#L5.
@@ -31,7 +31,12 @@ $calendar-range-end-body-cell-size: | |||
padding-right: $calendar-body-label-side-padding; | |||
} | |||
|
|||
.mat-calendar-body-cell-wrapper { | |||
display: contents; |
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 is this necessary? Also it seems this isn't supported in Safari.
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.
Hmm, well, the MDN Compatibility table for display says there is some issues on Safari with unusual elements 🤔 . I tried this on Safari and it seems to render fine.
Safari Version 15.1 (17612.2.9.1.20).mat-calendar-body-cell { | ||
display: table-cell; |
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.
We had an issue where NVDA would announce an extra "table" because something had display: table
. I wouldn't be surprised if it becomes an issue here too. See #23446.
@@ -122,6 +122,9 @@ export class MatCalendarBody implements OnChanges, OnDestroy { | |||
/** Emits when a new value is selected. */ | |||
@Output() readonly selectedValueChange = new EventEmitter<MatCalendarUserEvent<number>>(); | |||
|
|||
/** Emits when a new date becomes active. */ | |||
@Output() readonly activeValueChange = new EventEmitter<MatCalendarUserEvent<number>>(); |
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.
What is this event used for? It seems like we're only emitting it/forwarding it.
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.
Hmm, good question. What this is trying to accomplish is making it so that the calendar body can update the value of the activeDate
when a cell receives the focus event. activeDate
has a reference in three components – the root component MatCalendarBody, the component for the specific view month/year/multiyear and the calendar body.
This might be easier to do with banana in a box rather than passing an event. That is currently (in master branch) how the MatCalendar
component gives the activeDate
to the MatMonthView component (and also MatMonthView and MatMultiYearView).
[(activeDate)]="activeDate" |
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.
ok, so if I'm understanding it correctly, what it's trying to achieve is to sync the activeDate
back up with the calendar if focus is moved by something we don't control (e.g. a keyboard shortcut)?
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.
Exactly, if I had more time, I would even go as far as renaming activeDate
to focusedDate
, but I'm not sure that would be worth changing public API for.
--> | ||
<td *ngFor="let item of row; let colIndex = index" role="gridcell" class="mat-calendar-body-cell-wrapper"> | ||
<div | ||
role="button" |
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's only 5-6 lines of CSS so it might be easier to do it now https://github.com/angular/components/blob/master/src/material/core/style/_button-common.scss#L5.
@@ -149,13 +149,14 @@ describe('MatMonthView', () => { | |||
fixture.detectChanges(); | |||
}); | |||
|
|||
it('should decrement date on left arrow press', () => { | |||
fit('should decrement date on left arrow press', () => { |
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.
Leftover from debugging?
@@ -122,6 +122,9 @@ export class MatCalendarBody implements OnChanges, OnDestroy { | |||
/** Emits when a new value is selected. */ | |||
@Output() readonly selectedValueChange = new EventEmitter<MatCalendarUserEvent<number>>(); | |||
|
|||
/** Emits when a new date becomes active. */ | |||
@Output() readonly activeValueChange = new EventEmitter<MatCalendarUserEvent<number>>(); |
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.
ok, so if I'm understanding it correctly, what it's trying to achieve is to sync the activeDate
back up with the calendar if focus is moved by something we don't control (e.g. a keyboard shortcut)?
@crisbeto thanks for looking this draft. Over the break, it was easier to work on multiple changes on the same branch until we started merging PR's again. I'm closing this in favor of individual non-draft PR's for these changes. Please see: |
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. |
Draft PR of fixing focus and navigation issues on the calendar. Please see commit messages for details on these changes.
This is still a draft at least until I can do more testing across different browsers and screen readers.