-
Notifications
You must be signed in to change notification settings - Fork 22
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
Keyboard navigation #81
Conversation
Hey @mst101 I guess we should discuss the approach of the keyboard support to get rid of the focus problem because i checked out a few datepicker and all them seem to be a little bit different: esc: enter: tab: arrow keys: This way we don't have the issue that you described and we are basically having the same keyboard navigation like google chrome which you can checkout if you open https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/date with google chrome. I think your changes are a little bit to big for such a simple feature. Wouldn't it be easier to have a Let me know what you think of my approach, i didn't test it this is just a little thought on this topic. To your side note: |
@mst101 great work, thank you for this! I'm happy to help with this however possible. I did clone and test
I agree with @MrWook about:
Furthermore, I would recommend implementing focus trapping on I would also suggest moving the close button to within
Please see https://webaim.org/techniques/css/invisiblecontent/ for more details. Once again, thank you very much for your work on this. It will improve the lives of many people. It's amazing the joy we've witnessed during testing sessions when small improvements like these make something accessible. |
Hey @betweenbrain The focus trapping is basically the implementation detail for
which means it should be easier to implement with your example. There isn't really a "close-button", there is a just a calendar button for people who want to have this kind of thing next to the input field. |
Hello @MrWook
Perfect, I will do that.
The hidden close button would support use of the enter key when the button has focus. Escape should already be supported with this PR by @mst101. My understanding is that for users of screen readers, pressing escape to close something, that opens & closes, is expected behavior and doesn't need to be announced to the user.
No problem, I certainly understand about supporting older browsers. There are other approaches, such as the following, to accomplish this:
|
@MrWook - thanks for your helpful comments. And kudos for researching what others have done before us! I look forward to the day when Don't know about you, but I quite like the slide transition as well... but one thing at a time :-)
Some useful thoughts here, but unfortunately (for the up/down keys), it's not as simple as +/- 7 for the days and +/- 3 for the months/years. That would work for the days (if we forced people to show 'edge dates') - and the months would be fine too - but what about the years? Especially when the So, as convenient as it would be to have a @betweenbrain - thanks for all your insights. As @MrWook says, best to generate your own branch from master and submit as a new PR. Gotta love the internet - Germany - Gambia - USA - all trying to get a job done :-) |
Hi @MrWook, I just wanted to update you with where I'm at with this...
So, if you check out the `test/e2e' branch and run the following commands:
...you should see Cypress fire up and be able to run a whole load of e2e tests. Notes:
Anyway, my plan now is to continue with building up the remaining tests based on the info in that ugly table. We should then have a nicely documented spec that we can agree upon for how the keyboard navigation should work. And then perhaps I can actually get round to implementing it :-) Please let me know if you have any thoughts/comments on this. Cheers! |
Days and weeks fly by and I can only assume:
I was heartened to hear recently that even the mighty Adam Wathan had to "gulp" at the prospect of implementing a datepicker for Tailwind UI. That said, the good news is that I'm getting close to being able to show you something... It even has a nice fade transition on opening/closing and changing the Thanks for your patience and watch this space :-) |
Don't worry mate, i didn't had the time to do anything here. But i think i should have time now and will do some smaller things next week. |
7504b6f
to
9d01421
Compare
In case it's of interest, this latest commit (big-fat-dump) shows where I've been trying to get to with the keyboard navigation. It includes a number of e2e tests (if you run There are still some errors e.g. the transitions are not working correctly and in Overall though, this might provide some incentive for ploughing on with this work? Right now, however, I'm afraid I feel I've exhausted my limits and either need a break, or some external assistance to at least overcome the frustrating challenges of trying to add these (fade) transitions on open/close and view change. |
8fcdac5
to
ddbd947
Compare
OK, it's finally here! The approach I adopted for determining where to apply the focus when a user action is taken was to push up an array of The I hope you'll be able to follow along OK? Any questions, or suggestions for improvement, gratefully received. Cheers! |
@mst101 Fantastic! I will check this out as soon as I can and report back with any findings. |
@MrWook @betweenbrain - does anyone have any smart ideas as to how to escape the 'black hole of doom' when tabbing into an inline calendar? How should we determine which element to focus when tabbing forwards/backwards to leave an inline datepicker? Alternatively, do we put this in the "too difficult" box and simply disable all keyboard navigation for inline calendars? Your thoughts? |
Hello @mst101, thanks for the mention. If I understand the issue correctly, I'm not sure if the datepicker component should be concerned with what is focused when a user leaves the component. But, one thought would be to consider finishing up what you have now, release that and wait on feedback, if any, before worrying too much about some of these potential edge cases. Keep up the great work! |
@betweenbrain - thanks for confirming what I'd suspected, namely that a humble datepicker should have no business knowing what other focusable elements may be on the page! It should have been apparent much sooner, but it seems a better approach to solving this problem would be to disable the focus-trapping on an inline calendar when:
I'll have a go at implementing this asap. Thanks for your input and encouragement - it's appreciated. |
bb9c870
to
216f639
Compare
216f639
to
e2ee8de
Compare
@MrWook @betweenbrain I've also fixed a few other issues e.g.
So, as far as I'm aware - everything now works exactly as it should! And the BDD feature files are backed up by 120 e2e tests:
Finally, you'll be pleased to hear that the minified code (incl. css) has only increased from 66.7 kB to 90.9 kB (i.e. not the 105.2 kB it was previously). I think this may be because I updated the BrowsersList which means that certain polyfills for older browsers are no longer needed. Anyway - please check it out - and feel free to merge in my changes once you're ready! |
I think you did an amazing job on this one. It feels amazing to navigation through the demo page with keyboard only ❤️ BUT let's do this in a later stage. Let's merge this bad boy ✨ 🚀 |
@MrWook - Awesome - thanks for merging! I think you're right about the escape key - it should just close the calendar (if it's open). But if the calendar is closed, then it should clear the date. I'll have a go at fixing this asap. |
I just noticed you mentioned delete/backspace. This seems to be how Chrome works for |
@betweenbrain - OK, so this is where I've got to so far with the keyboard navigation...
As you can see, navigation via
tab
,space
,enter
,escape
andarrow
keys seems to be working well and I've added tests for most of this.@MrWook - I've improved upon my initial strategy (#53) by creating
FocusedDate
,FocusedMonth
andFocusedYear
classes which expose anextCell
computed property in the relevant picker. This allows us to determine certain properties of the next cell that would be focused up/down/left/right. e.g. inPickerDay
for today, 1st Feb 2021 (when showing edge-dates):We don't actually need the
date
property, but I've left it in temporarily for clarity. When remaining on the samemonth
,year
ordecade
, thedelta
property simply indicates how many cells to move up/down/left or right. However, when a change of focus requires a page change, thedelta
property refers to the number of cells that the focus should move:cells
array (whenonOtherPage
is 'Next'), orcells
array (whenonOtherPage
is 'Previous').The issue:
I'm currently having trouble with finding a way to close the calendar when no element within it is focused (see the
it('should close when the calendar loses focus'
test inDatepicker.spec.js
...As I mentioned in the PR I referred to earlier (#53), I've been trying to do this by using the :focus-within pseudo selector to keep track of whether any element within the datepicker is focused (see the
checkFocus
method inDatepicker.vue
. Since $refs are not reactive in Vue, we need to fire a check-focus event each time a focused element within the datepicker is blurred, so I'm emitting acheck-focus
event on blur from any button that can be focused.However, I'm not sure if this is a sensible strategy (as I haven't managed to get it to work properly yet!)
As it stands, the calendar opens and closes correctly by clicking on the
input
field (or via thecalendar-button
), but it does not close when the calendar loses focus. However, if you uncomment thereturn
statement in thecheckFocus
method inDatepicker.vue
, you'll see that the calendar does close when the calendar loses focus, but, we now have the problem that it closes - and opens up again! - when theinput
orcalendar-button
is clicked.This has been driving me a little crazy, so I would really appreciate your help... :-)
Side note:
Perhaps unnecessary, but I changed some of the functions & event names to help me reason about what's going on. It seemed strange to me that we are using event names such as
show-calendar
andclose-calendar
when show/hide or open/close appear to be a more natural and consistent way to describe what's happening. Personally, I prefer open/close as we are emitting bothopened
andclosed
events from the component. @MrWook - let me know if you agree with this change and if you'd like me to create a separate PR for this.