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

jimmytheneutrino · 2019-03-24T09:00:59Z

Tabbing away from time-picker overlay was not possible: It wrapped
around and usually ended up in browser's address bar. Also, once you
selected time-picker wrapper span, there was no way to open the overlay
via keyboard.
This fixes this. Tabbing away from overlay now focuses on time-picker
wrapper span. Pressing enter on the wrapper span opens the overlay.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/NG-ZORRO/ng-zorro-antd/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Tabbing away from time-picker overlay was not possible: It wrapped
around and usually ended up in browser's address bar. Also, once you
selected time-picker wrapper span, there was no way to open the overlay
via keyboard.

Issue Number: N/A

What is the new behavior?

Tabbing away from overlay now focuses on time-picker
wrapper span. Pressing enter on the wrapper span opens the overlay.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

The behavior (both current and new) can be observed on the main demo page (npm run site:start).

netlify · 2019-03-24T09:12:20Z

Deploy preview for ng-zorro-master ready!

Built with commit a594623

https://deploy-preview-3145--ng-zorro-master.netlify.com

codecov · 2019-03-24T09:18:14Z

Codecov Report

Merging #3145 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3145      +/-   ##
==========================================
- Coverage   95.39%   95.37%   -0.03%     
==========================================
  Files         712      706       -6     
  Lines       14633    14516     -117     
  Branches     1929     1915      -14     
==========================================
- Hits        13959    13844     -115     
  Misses        244      244              
+ Partials      430      428       -2

Impacted Files	Coverage Δ
components/time-picker/nz-time-picker.component.ts	`92.15% <100%> (+0.07%)`	⬆️
...mponents/descriptions/nz-descriptions.component.ts	`95.83% <0%> (-2.89%)`	⬇️
components/message/nz-message.service.ts	`97.95% <0%> (-2.05%)`	⬇️
components/tabs/nz-tab.component.ts	`96% <0%> (-0.56%)`	⬇️
components/carousel/nz-carousel.component.ts	`93.71% <0%> (-0.54%)`	⬇️
components/input/nz-autosize.directive.ts	`91.76% <0%> (-0.38%)`	⬇️
components/tabs/nz-tabs-nav.component.ts	`90.62% <0%> (-0.34%)`	⬇️
components/core/wave/nz-wave-renderer.ts	`88.23% <0%> (-0.18%)`	⬇️
components/carousel/strategies/base-strategy.ts	`96% <0%> (-0.16%)`	⬇️
components/layout/nz-sider.component.ts	`93.75% <0%> (-0.1%)`	⬇️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab8a58c...a594623. Read the comment docs.

wilsoncook · 2019-03-27T02:13:55Z

components/time-picker/nz-time-picker.component.html

    [(ngModel)]="value">
  </nz-time-picker-panel>
+  <span tabindex=0 (focus)="close()" class="nz-tab-catching-span"></span>


Why class "nz-tab-catching-span"?

Also the syntax tabindex=0 should be tabindex="0"

Why class "nz-tab-catching-span"?
So that it is easier to handle in the future (if there is need) plus it is a bit self-documenting.

Also the syntax tabindex=0 should be tabindex="0"
Ok. I will fix that.

wilsoncook · 2019-03-27T02:26:16Z

Hi @jimmytheneutrino , we're very grateful for your work! It would be better to provide the actual usage scenario for this PR (as i think there is no need to handle the enter key on <input> only, unless we provide the full set of keyboard processing mechanisms such as "left/right arrow to select a specified time value").

jimmytheneutrino · 2019-03-27T10:12:58Z

@wilsoncook
This onEnter is for the case when the user inputs values by hand using digits on the keyboard.
IMHO the user does not need any arrow key bindings when they already can input just digits.

jimmytheneutrino · 2019-03-29T10:35:57Z

@wilsoncook
Hi! Do you need something else fixed?

jimmytheneutrino · 2019-06-12T10:01:23Z

@wilsoncook Could you please tell me what else needs to be done?

wenqi73 · 2019-06-13T02:56:22Z

@jimmytheneutrino Thanks for your PR, this belongs to a11y (#1651) which contains full set of keyboard processing as @wilsoncook said, not just OnEnter. It will be helpful that if you provide a full solution of this component.

jimmytheneutrino · 2019-07-16T15:04:47Z

@wenqi73 @wilsoncook
I understand that the full a11y solution would be preferable. Unfortunately, this seems to be a large task and I do not have time to do all of its nuances.

However, isn't a partial improvement better than no improvement? Please see the videos:
https://imgur.com/a/IhI9R6i .

I think that just this little bit could be a huge improvement in usability for some users.

wenqi73 · 2019-07-18T07:34:36Z

components/time-picker/nz-time-picker.component.html

    [(ngModel)]="value">
  </nz-time-picker-panel>
+  <span tabindex="0" (focus)="close()" class="nz-tab-catching-span"></span>


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.

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.

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

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?

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.

wenqi73 · 2019-07-18T07:35:29Z

components/time-picker/nz-time-picker.component.html

@@ -6,7 +6,8 @@
  [placeholder]="nzPlaceHolder || ('TimePicker.placeholder' | nzI18n)"
  [(ngModel)]="value"
  readonly="readonly"
-  (click)="open()">
+  (click)="open()"
+  (keyup.enter)="open()">


You should add the tests about your changes.

Added tests.

Tabbing away from time-picker overlay was not possible: It wrapped around and usually ended up in browser's address bar. Also, once you selected time-picker wrapper span, there was no way to open the overlay via keyboard. This fixes this. Tabbing away from overlay now focuses on time-picker wrapper span. Pressing enter on the wrapper span opens the overlay.

vthinkxie · 2019-10-14T02:48:41Z

@wenqi73 plz check this again

wenqi73

The behavior of TAB key does not match our design.

vthinkxie · 2020-01-06T08:31:57Z

As there is no update for a long time, the PR will be closed.
If you have any questions, feel free to open another PR, thanks a lot.

jimmytheneutrino mentioned this pull request Mar 24, 2019

fix(module:date-picker): make keyboard navigation possible #3146

Closed

3 tasks

hsuanxyz requested a review from wilsoncook March 25, 2019 01:49

hsuanxyz added the PR::eyes: Need Review label Mar 25, 2019

wilsoncook reviewed Mar 27, 2019

View reviewed changes

jimmytheneutrino force-pushed the fix-time-picker-tabbing branch from c5f594c to 89f1d87 Compare March 27, 2019 10:32

weekly-digest bot mentioned this pull request Apr 1, 2019

Weekly Digest (25 March, 2019 - 1 April, 2019) #3196

Closed

weekly-digest bot mentioned this pull request Jun 16, 2019

Weekly Digest (9 June, 2019 - 16 June, 2019) #3582

Closed

hsuanxyz added the Component: TimePicker label Jun 18, 2019

zorro-bot bot requested a review from wenqi73 June 18, 2019 11:50

vthinkxie added PR: unreviewed and removed PR::eyes: Need Review labels Jun 22, 2019

weekly-digest bot mentioned this pull request Jun 23, 2019

Weekly Digest (16 June, 2019 - 23 June, 2019) #3630

Closed

vthinkxie added PR: reviewed-changes-requested and removed PR: unreviewed labels Jun 25, 2019

pr-triage bot added the PR: unreviewed label Jul 14, 2019

danieldubrovski force-pushed the fix-time-picker-tabbing branch from 5c23e0b to bc9f809 Compare July 15, 2019 21:10

wenqi73 reviewed Jul 18, 2019

View reviewed changes

jimmytheneutrino force-pushed the fix-time-picker-tabbing branch from bc9f809 to 6bdcee4 Compare July 18, 2019 15:01

jimmytheneutrino force-pushed the fix-time-picker-tabbing branch from 6bdcee4 to a594623 Compare July 18, 2019 15:19

wzhudev assigned wenqi73 Jul 19, 2019

vthinkxie removed the PR: reviewed-changes-requested label Oct 14, 2019

wenqi73 requested changes Oct 14, 2019

View reviewed changes

pr-triage bot added the PR: reviewed-changes-requested label Oct 14, 2019

vthinkxie removed the PR: unreviewed label Oct 14, 2019

weekly-digest bot mentioned this pull request Oct 20, 2019

Weekly Digest (13 October, 2019 - 20 October, 2019) #4326

Closed

vthinkxie closed this Jan 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jimmytheneutrino commented Mar 24, 2019

netlify bot commented Mar 24, 2019 •

edited

Loading

codecov bot commented Mar 24, 2019 •

edited

Loading

wilsoncook Mar 27, 2019

wilsoncook Mar 27, 2019

jimmytheneutrino Mar 27, 2019

wilsoncook commented Mar 27, 2019

jimmytheneutrino commented Mar 27, 2019

jimmytheneutrino commented Mar 29, 2019

jimmytheneutrino commented Jun 12, 2019

wenqi73 commented Jun 13, 2019

jimmytheneutrino commented Jul 16, 2019

wenqi73 Jul 18, 2019

jimmytheneutrino Jul 18, 2019

wenqi73 Jul 19, 2019

jimmytheneutrino Jul 19, 2019

wenqi73 Jul 25, 2019

wenqi73 Jul 18, 2019

jimmytheneutrino Jul 18, 2019

vthinkxie commented Oct 14, 2019

wenqi73 left a comment

vthinkxie commented Jan 6, 2020

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

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

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

Conversation

jimmytheneutrino commented Mar 24, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

netlify bot commented Mar 24, 2019 • edited Loading

codecov bot commented Mar 24, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsoncook commented Mar 27, 2019

jimmytheneutrino commented Mar 27, 2019

jimmytheneutrino commented Mar 29, 2019

jimmytheneutrino commented Jun 12, 2019

wenqi73 commented Jun 13, 2019

jimmytheneutrino commented Jul 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vthinkxie commented Oct 14, 2019

wenqi73 left a comment

Choose a reason for hiding this comment

vthinkxie commented Jan 6, 2020

netlify bot commented Mar 24, 2019 •

edited

Loading

codecov bot commented Mar 24, 2019 •

edited

Loading