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.

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

fix(module:time-picker): make keyboard navigation possible #3145

Closed
wants to merge 1 commit into from
Closed

fix(module:time-picker): make keyboard navigation possible #3145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion components/time-picker/nz-time-picker.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
[placeholder]="nzPlaceHolder || ('TimePicker.placeholder' | nzI18n)"
[(ngModel)]="value"
readonly="readonly"
(click)="open()">
(click)="open()"
(keyup.enter)="open()">
Copy link
Member

Choose a reason for hiding this comment

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

You should add the tests about your changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests.

<span class="ant-time-picker-icon">
<i nz-icon nzType="clock-circle"></i>
</span>
Expand Down Expand Up @@ -50,7 +51,9 @@
[opened]="nzOpen"
[nzClearText]="nzClearText"
[nzAllowEmpty]="nzAllowEmpty"
(keyup.enter)="close()"
[(ngModel)]="value">
</nz-time-picker-panel>
<span tabindex="0" (focus)="close()" class="nz-tab-catching-span"></span>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<span tabindex="0" (focus)="close()" class="nz-tab-catching-span"></span>

In theory the focus point should be moved to time picker panel when pressing Tab, it should not close the panel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the same way as it is done in the react version: https://ant.design/components/time-picker/ ?

But I am sure this is a UX bug in that version. This behaviour would be completely unusable with a keyboard. You need to press tab about a hundred times to get anything done there. In reality, choosing time on the time-picker is not needed if the user is already using the keyboard.

Copy link
Member

Choose a reason for hiding this comment

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

The right way is: Users use Tab to toggle HOUR/MINITE/SECOND/INPUT selection, use UP_ARROW or DOWN_ARROW to select number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really? In order to input the time this way in the worst case the user needs 12 + 30 + 30 keystrokes.
Why would anyone do it if they can just input time with numbers, press tab and go to the next input on the form? The way you propose the user would just have to press tab 3 times more to skip through those selections.

The way I see its usage is that:
a) there are some users who use mouse to enter time - and the time panel is great for that,
b) but there are some 'more experienced' users who want to do it faster -- and they could use keyboard for that -- 6 strokes+tab -- might take less than 2 seconds.
Imagine a receptionist somewhere who does it every day, hundreds of times.

Of course, one could implement a special form for the case b), but if the standard component could do it, why not try with a standard one?

Copy link
Member

Choose a reason for hiding this comment

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

We need to support to cover everywhere of the panel only use keyboard, I mean use Tab to toggle when the panel is open, if user want to do it faster they can use 6 strokes + Enter + Tab to switch to next element, the Tab should not just close panel.

</ng-template>

42 changes: 42 additions & 0 deletions components/time-picker/nz-time-picker.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { NzTimePickerModule } from './nz-time-picker.module';

import { registerLocaleData } from '@angular/common';
import zh from '@angular/common/locales/zh';
import { dispatchFakeEvent } from 'ng-zorro-antd/core';
registerLocaleData(zh);

describe('time-picker', () => {
Expand Down Expand Up @@ -87,6 +88,27 @@ describe('time-picker', () => {
expect(testComponent.openChange).toHaveBeenCalledTimes(3);
expect(testComponent.open).toBe(true);
});
it('should open and close on enter', fakeAsync(() => {
testComponent.date = new Date('2018-11-11 11:11:11');
testComponent.open = false;
fixture.detectChanges();
testComponent.nzTimePickerComponent.inputRef.nativeElement.dispatchEvent(
new KeyboardEvent('keyup', { key: 'enter' })
);
fixture.detectChanges();
tick(500);
fixture.detectChanges();
expect(testComponent.open).toBe(true);
const panel = overlayContainer.getContainerElement().querySelector('nz-time-picker-panel');
expect(panel).toBeTruthy();
if (panel) {
panel.dispatchEvent(new KeyboardEvent('keyup', { key: 'enter' }));
}
fixture.detectChanges();
tick(500);
fixture.detectChanges();
expect(testComponent.open).toBe(false);
}));
it('should clear work', fakeAsync(() => {
fixture.detectChanges();
testComponent.date = new Date('2018-11-11 11:11:11');
Expand All @@ -102,6 +124,26 @@ describe('time-picker', () => {
fixture.detectChanges();
expect(testComponent.nzTimePickerComponent.nzFormat).toBe('h:mm:ss a');
});
it('should be tabbable back to trigger wrapper', fakeAsync(() => {
testComponent.date = new Date('2018-11-11 11:11:11');
testComponent.open = true;
fixture.detectChanges();
tick(500);
fixture.detectChanges();
expect(testComponent.open).toBe(true);

// It is impossible to simulate actual TAB behaviour using events.
// This is the next best thing.
dispatchFakeEvent(
overlayContainer.getContainerElement().querySelector('span.nz-tab-catching-span') as HTMLElement,
'focus'
);
fixture.detectChanges();
tick(500);
fixture.detectChanges();
expect(testComponent.open).toBe(false);
expect(document.activeElement).toEqual(testComponent.nzTimePickerComponent.inputRef.nativeElement);
}));
});
});

Expand Down
1 change: 1 addition & 0 deletions components/time-picker/nz-time-picker.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ export class NzTimePickerComponent implements ControlValueAccessor, OnInit, Afte
close(): void {
this.nzOpen = false;
this.nzOpenChange.emit(this.nzOpen);
this.focus();
}

updateAutoFocus(): void {
Expand Down