feat(datepicker): keyboard navigation for accessibility #2270
Conversation
There was a problem hiding this comment.
Thanks for the PR,
Looks good, but a couple of use cases are broken.
Apart from the comments in the code:
-
When focusing left/right navigation arrow → press
Enter→ it will change the month and move focus to the day. The focus should stay on the arrow. Same for month/year select boxes. -
I don't think it should be possible to focus a disabled datepicker (it's the case currently, see the demo) + tests for this use case
-
Arrows are not of bootstrap blue color (see comment in the code)
-
activeclass is broken on a focused day (see comment in the code) -
Should check the use case when
minDateis set and left arrow becomes disabled → it should havetabindex = -1 -
Could you please add
Fixes #1716andBREAKING CHANGESsection in the commit? (ex. like here → 2311616). Breaking changes should explain that it's not possible focus the datepicker anymore and.focus()method focuses a day
| @@ -68,8 +72,8 @@ import {NgbDatepickerI18n} from './datepicker-i18n'; | |||
| `], | |||
| template: ` | |||
| <div class="ngb-dp-arrow"> | |||
| <button type="button" class="btn btn-link ngb-dp-arrow-btn" | |||
| (click)="!!navigate.emit(navigation.PREV)" [disabled]="prevDisabled" tabindex="-1"> | |||
| <button type="button" class="btn ngb-dp-arrow-btn" | |||
There was a problem hiding this comment.
The button became black after removing btn-link
I guess we should leave it and override :focus, so it would reflect current bootstrap theme
src/datepicker/datepicker.ts
Outdated
| @@ -132,7 +127,7 @@ export interface NgbDatepickerNavigateEvent { | |||
| </ngb-datepicker-navigation> | |||
| </div> | |||
|
|
|||
| <div class="ngb-dp-months"> | |||
| <div class="ngb-dp-months" (keydown)="onKeyDown($event)" (focus)="showFocus(true)" (focusout)="showFocus(false)"> | |||
There was a problem hiding this comment.
Should be (focusin), otherwise showFocus() is never called → we never set focusVisible in the service → we never get active class on the current day (we lost greyish highlight of the current day + day.context.focused is never true)
I also think there are unit tests missing for checking active class on the focused day in datepicker.spec.ts.
src/datepicker/datepicker.ts
Outdated
| focus() { this._elementRef.nativeElement.focus(); } | ||
| focus(focusDay = true) { | ||
| take.call(this._ngZone.onStable.asObservable(), 1).subscribe(() => { | ||
| const elementToFocus = this._elementRef.nativeElement.querySelector(focusDay ? 'div.ngb-dp-day[tabindex="0"]' : 'button'); |
There was a problem hiding this comment.
Focusing button is not an option, because there might be no button whatsoever, ex. with datepicker navigation='none' there are no arrows
I'm actually not sure how we could find what to focus apart from the day... we probably should always focus the day here
Tests missing for this case
src/datepicker/datepicker.ts
Outdated
| @@ -1,4 +1,5 @@ | |||
| import {Subscription} from 'rxjs/Subscription'; | |||
| import {take} from 'rxjs/operator/take'; | |||
There was a problem hiding this comment.
Could we please change it to and use .pipe in the code?
import {take} from 'rxjs/operators';It will be merged in 2.0.0 and we're gonna have pipeable operators by default by then
|
Oh, also CI failed because of code formatting issue and there is a tiny conflict to resolve |
src/datepicker/datepicker.ts
Outdated
| @@ -272,8 +275,16 @@ export class NgbDatepicker implements OnDestroy, | |||
|
|
|||
| /** | |||
| * Manually focus the datepicker | |||
| * If focusDay is false, the left navigation button will be focused, if true, the focusable day will be focused | |||
There was a problem hiding this comment.
Please remove this description, as no longer relevant
BREAKING CHANGE: The datepicker is no longer focusable as a whole component. Instead, the focus is enable on each elements inside the datepicker (navigation buttons, selects, focusable day) in the natural order. The datepicker focus method will focus the focusable day.
|
Part of #1946 |
No description provided.