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.

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

Merged
merged 54 commits into from
Jan 20, 2022
Merged

Conversation

mst101
Copy link
Contributor

@mst101 mst101 commented Feb 1, 2021

@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 and arrow 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 and FocusedYear classes which expose a nextCell 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. in PickerDay for today, 1st Feb 2021 (when showing edge-dates):

nextCell: {
    down: {
        date: 'Mon Feb 08 2021 00:00:00 GMT+0000 (Greenwich Mean Time)'
        delta: 7
        isDisabled: false
        onOtherPage: false
    },
    left: {
        date: 'Sun Jan 31 2021 00:00:00 GMT+0000 (Greenwich Mean Time)'
        delta: -1
        isDisabled: false
        onOtherPage: false
    },
    right: {
        date: 'Tue Feb 02 2021 00:00:00 GMT+0000 (Greenwich Mean Time)'
        delta: 1
        isDisabled: false
        onOtherPage: false
    },
    up: {
        date: 'Mon Jan 25 2021 00:00:00 GMT+0000 (Greenwich Mean Time)'
        delta: -13
        isDisabled: false
        onOtherPage: 'Previous'
    },
}

We don't actually need the date property, but I've left it in temporarily for clarity. When remaining on the same month, year or decade, the delta property simply indicates how many cells to move up/down/left or right. However, when a change of focus requires a page change, the delta property refers to the number of cells that the focus should move:

  • forward from the start of the cells array (when onOtherPage is 'Next'), or
  • back from the end of the cells array (when onOtherPage 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 in Datepicker.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 in Datepicker.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 a check-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 the calendar-button), but it does not close when the calendar loses focus. However, if you uncomment the return statement in the checkFocus method in Datepicker.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 the input or calendar-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 and close-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 both opened and closed 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.

@MrWook
Copy link
Collaborator

MrWook commented Feb 1, 2021

Hey @mst101
like always thanks for your contribution.

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:
Most of the custom datepicker are closing the datepicker on the tab key. If a datepicker doesn't close it, it will just cycle through the possible navigation keys inside the datepicker like next month/next decade.
I guess we should stick with the second approach to reach all buttons inside the datepicker.
Which means i would say we should probably do something like that:

esc:
close

enter:
select current selection
close

tab:
cycle trough navigation

arrow keys:
cycle trough picker dates

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.
If the difference between tab and arrow keys are weird to implement i would be totaly fine with using the same function for both type of keys and just cycle endlessly through all keys and implement it in the future.

I think your changes are a little bit to big for such a simple feature. Wouldn't it be easier to have a addEventListener for keyup inside the Datepicker.vue on the PickerView ref?
You already used document.activeElement inside the tests. You could use it to get the next and previous element
document.activeElement.previousElementSibling
document.activeElement.nextElementSibling
On the arrow keys for up and down you could use the previous commands to go 8 forward or backwards on the day view and 3 for the other views and if you hit the end of buttons change the month/year/decade.
I think this should work and reduce the new code by a lot.

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:
I agree with you, so go for it!

@MrWook MrWook added the enhancement New feature or request label Feb 1, 2021
@betweenbrain
Copy link

@mst101 great work, thank you for this! I'm happy to help with this however possible. I did clone and test mst101:feat/navigate-via-keyboard with Voiceover on Chrome and these are my findings. We're currently working with the Center for Accessible Technology and much of this is from similar issues we've needed to address elsewhere on our website. Please let me know if you'd like me to open a PR against your fork for any of these items.

  • .vdp-datepicker__calendar-button needs an aria-label attribute, maybe with a value like "open date picker" or "close date picker" depending on the state of the date picker. It currently reads as "ellipsis button".
  • .vdp-datepicker__calendar-button also needs the aria-expanded attribute with the value of either 'true' or 'false', depending on the state of the date picker. The button should also have an aria-controls attribute that is set to an ID assigned to .vdp-datepicker__calendar.
  • .vdp-datepicker__calendar should have an aria-labelledby attribute that refers back to the id attribute of the button that controls it. It should also have a role="region"
  • .picker-view .prev and .picker-view .next similarly need the aria-label attribute. They each read as "less than button", and "greater than button"
  • Each of the .cell .day buttons need an aria-label. I would suggest something like "Select [local date string]" where [local date string] is:
const options = {
  weekday: 'long',
  year: 'numeric',
   month: 'long',
  day: 'numeric',
      }
      
new Date(day.timestamp).toLocaleDateString(undefined, options)

I agree with @MrWook about:

esc:
close

enter:
select current selection
close

tab:
cycle trough navigation

arrow keys:
cycle trough picker dates

Furthermore, I would recommend implementing focus trapping on .vdp-datepicker__calendar. If you're not familiar with it, it basically restricts keyboard navigation (tabbing) from being able to navigate outside of an element until the escape key or close button is pressed. There's a good example at https://gist.githubusercontent.com/myogeshchavan97/d50d42aa9205573b811587d57c2e58a6/raw/c91616484e2aa2428bc68aaf207b5a8f8d2b9cec/trap_focus.js and with more details at https://uxdesign.cc/how-to-trap-focus-inside-modal-to-make-it-ada-compliant-6a50f9a70700

I would also suggest moving the close button to within .vdp-datepicker__calendar, maybe after .picker-view .next, and make it hidden off-screen so that it's only for screen readers. We were suggested to use the following CSS for this:

{clip: rect(1px, 1px, 1px, 1px);
clip-path: inset(50%);
height: 1px;
width: 1px;
margin: -1px;
overflow: hidden;
padding: 0;
position: absolute;}

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.

@MrWook
Copy link
Collaborator

MrWook commented Feb 2, 2021

Hey @betweenbrain
let us move the accessibility with aria-labels for screen readers into an own PR. It doesn't directly involve the keyboard navigation and can be implemented separately.
This way it is easier to follow a PR and you could already fork the master branch and create a pull request with aria-labels.
I had this on my todo list anyway, I just did not have the time to further develop this repository.
Thanks for the ideas where we should add the aria labels and what should go into them 👍

The focus trapping is basically the implementation detail for

tab:
cycle trough navigation

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.
If i get your suggestion right you just want to have a button inside the picker which states close picker with esc to help people with a screen reader close the datepicker without selecting a date?
I'm not so happy about your css suggestion because clip and clip-path are not supported in older browsers like IE and till now this library should have support for those older browsers.

@betweenbrain
Copy link

Hello @MrWook

let us move the accessibility with aria-labels for screen readers into an own PR. It doesn't directly involve the keyboard navigation and can be implemented separately.
This way it is easier to follow a PR and you could already fork the master branch and create a pull request with aria-labels.

Perfect, I will do that.

If i get your suggestion right you just want to have a button inside the picker which states close picker with esc to help people with a screen reader close the datepicker without selecting a date?

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.

I'm not so happy about your css suggestion because clip and clip-path are not supported in older browsers like IE and till now this library should have support for those older browsers

No problem, I certainly understand about supporting older browsers. There are other approaches, such as the following, to accomplish this:

position:absolute;
left:-10000px;
top:auto;
width:1px;
height:1px;
overflow:hidden;

@mst101
Copy link
Contributor Author

mst101 commented Feb 2, 2021

@MrWook - thanks for your helpful comments. And kudos for researching what others have done before us!

I look forward to the day when <input type="date"> has greater than 88.7% global usage, but in the meantime I'm happy to try and emulate the way Chrome/Edge/Opera implement their keyboard navigation (I wasn't convinced by the Firefox version, at least not in this respect).

Don't know about you, but I quite like the slide transition as well... but one thing at a time :-)

I think your changes are a little bit to big for such a simple feature. Wouldn't it be easier to have a addEventListener for keyup inside the Datepicker.vue on the PickerView ref?
You already used document.activeElement inside the tests. You could use it to get the next and previous element
document.activeElement.previousElementSibling
document.activeElement.nextElementSibling
On the arrow keys for up and down you could use the previous commands to go 8 forward or backwards on the day view and 3 for the other views and if you hit the end of buttons change the month/year/decade.
I think this should work and reduce the new code by a lot.

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 picker-year-range prop allows you to display a variable number of them? Perhaps you're suggesting that blank cells should be focusable, but that would seem strange to me...

So, as convenient as it would be to have a keyup event listener in Datepicker.vue or PickerView, I fear we do need to keep these at the PickerDay/Month/Year level. If you think the code is a bit heavy, there may be a way to refactor it later (inherit from a base class perhaps?). But for now, I'll crack on with the tab navigation and hope that resolves my current predicament.

@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 :-)

@betweenbrain betweenbrain mentioned this pull request Feb 11, 2021
@mst101
Copy link
Contributor Author

mst101 commented Mar 2, 2021

Hi @MrWook,

I just wanted to update you with where I'm at with this...

  1. Your earlier comment that we should "discuss the approach of the keyboard support" + your efforts to work out a spec for Close calendar with button when clicking anywhere on the page #67 made me wonder whether we should perhaps adopt a "BDD approach" to creating this keyboard navigation feature?

  2. BDD (Behaviour Driven Design) is something I've briefly read about in the past, but have never actually used. Perhaps, you're already familiar with the subject, but if not, I highly recommend this excellent webinar on BDD Testing with Cypress Cucumber and Vue.js / Vuex by Kyle Coberly.

  3. It occurred to me that if we were to apply a similar approach to creating a spec for the keyboard navigation as you did for Close calendar with button when clicking anywhere on the page #67, in addition to considering the open/closed state of the calendar and the props, we'd also need to take into account the focus state i.e. the activeElement.

  4. So I came up with this rather ugly table which attempts to detail the 'initial state' vs. the 'end state' after various 'actions' have been performed.

  5. I then made a start on translating this table into various 'feature files' with each row (or 'scenario') having a corresponding 'cypress e2e test' See example..

So, if you check out the `test/e2e' branch and run the following commands:

npm install
npm run serve:e2e
npm run test:e2e

...you should see Cypress fire up and be able to run a whole load of e2e tests.

Notes:

  • It appears we have a bug with the show-calendar-on-focus as this test fails in Firefox (but is fine in Chrome)
  • One of the tests for the typeable calendar was also failing, so I added this.input.value = '' in the clearDate method of DateInput.vue. Not sure if this is necessary though as it seems to work ok in production?

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!

@mst101
Copy link
Contributor Author

mst101 commented May 7, 2021

Days and weeks fly by and I can only assume:

  • I'm not very clever
  • I'm not as clever as I think I am
  • Adding support for keyboard navigation to a datepicker is actually a very tricky business

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 view - and a slide right/left transition for page changes e.g. via the prev/next buttons. What's more - gulp - it will be fully backed by a suite of unit and e2e tests.

Thanks for your patience and watch this space :-)

@MrWook
Copy link
Collaborator

MrWook commented May 7, 2021

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.

@mst101 mst101 force-pushed the feat/navigate-via-keyboard branch from 7504b6f to 9d01421 Compare July 20, 2021 07:06
@mst101
Copy link
Contributor Author

mst101 commented Jul 20, 2021

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 npm run serve:e2e and npm run test:e2e, you'll see that all 117 tests pass).

There are still some errors e.g. the transitions are not working correctly and in Datepicker.vue the second fade transition should either be a transition-group or its contents should be wrapped in a div. Some things have also moved on from where they are here e.g. we decided not to use props for the transition durations and apparently we shouldn't use inline styles... Also of note is that I tried to calculate the pickerHeight by building up the various 'sections' within pickerMixin, but I've since realised that this neglects the height of both the beforeCalendarHeader and calendarFooter slots...

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.

@mst101 mst101 force-pushed the feat/navigate-via-keyboard branch 3 times, most recently from 8fcdac5 to ddbd947 Compare August 10, 2021 13:19
@mst101
Copy link
Contributor Author

mst101 commented Aug 10, 2021

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 refs to data.focus.refs in Datepicker.vue - and, if appropriate, also a delay (in milliseconds) to data.focus.delay. The latter is needed when a new page slides in and you don't want to focus the new cell immediately as this would spoil the transition.

The reviewFocus function in navMixin.vue is then fired on $nextTick and iterates through the refs and sets the focus on the first non-disabled value. The exception to this approach is when navigating the cells via the arrow keys. Here, we bypass the reviewFocus function and simply set the focus directly in pickerMixin.vue. Incidentally @MrWook, I did end up using your recursive strategy for this, so thanks for your help with that!

I hope you'll be able to follow along OK? Any questions, or suggestions for improvement, gratefully received.

Cheers!

@betweenbrain
Copy link

@mst101 Fantastic! I will check this out as soon as I can and report back with any findings.

@mst101
Copy link
Contributor Author

mst101 commented Jan 11, 2022

@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?

@betweenbrain
Copy link

betweenbrain commented Jan 12, 2022

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!

@mst101
Copy link
Contributor Author

mst101 commented Jan 14, 2022

@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:

  • tabbing forwards from the last focusable element in the datepicker
  • tabbing backwards from the first focusable element in the datepicker
    ...and then simply allow the browser to determine which element should be focused next.

I'll have a go at implementing this asap. Thanks for your input and encouragement - it's appreciated.

@mst101 mst101 force-pushed the feat/navigate-via-keyboard branch 3 times, most recently from bb9c870 to 216f639 Compare January 16, 2022 18:21
@mst101 mst101 force-pushed the feat/navigate-via-keyboard branch from 216f639 to e2ee8de Compare January 18, 2022 16:27
@mst101
Copy link
Contributor Author

mst101 commented Jan 18, 2022

@MrWook @betweenbrain
OK - I've managed to implement the desired tabbing for an inline datepicker. Aside from disabling the focus-trapping when tabbing AWAY from an inline datepicker (and recording which cell last had focus), I had to add some checks when tabbing TO the datepicker. The datepicker now has an isActive flag which is true when any element within the datepicker is focused. When a inline datepicker receives focus, the recently tabbed-to cell is corrected when a) the cell was tabbed to (forwards) when show-header is false, and b) if the cell was tabbed to (backwards) when there are no focusable elements in the footer slots.

I've also fixed a few other issues e.g.

  1. When arrowing up (or down) from a cell to another page - if the destination cell is disabled AND all other cells beyond it are also disabled (i.e. to or from is used in the disabled-dates prop), then the earliest (or latest) possible cell will be focused.

  2. Tabbing now works properly when append-to-body is true (it wasn't before as the events weren't bubbling up to the root Datepicker).

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:

    npm run serve:e2e
    npm run test:e2e

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!

@MrWook
Copy link
Collaborator

MrWook commented Jan 20, 2022

I think you did an amazing job on this one. It feels amazing to navigation through the demo page with keyboard only ❤️
There is only one thing that is somehow weird.
If you press esc you clear the whole input field. This is something that i would expect from delete or backspace.
The esc key is mostly used to just close the opened window.

BUT let's do this in a later stage. Let's merge this bad boy ✨ 🚀

@MrWook MrWook merged commit b03b493 into sumcumo:master Jan 20, 2022
@mst101
Copy link
Contributor Author

mst101 commented Jan 20, 2022

@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.

@mst101
Copy link
Contributor Author

mst101 commented Jan 20, 2022

I just noticed you mentioned delete/backspace. This seems to be how Chrome works for <input type="date">. So, you're right, let's run with that (and just have esc close the window).

@mst101 mst101 deleted the feat/navigate-via-keyboard branch April 27, 2022 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants