feat(datepicker): Make the calendar navigable by keyboard

[mst101]

Hi @MrWook, please brace yourself - this is a big one :-)

Firstly, apologies for the Can’t automatically merge. message. I realised too late that I hadn't pulled your changes concerning the 'typeable date timezone' issue. I then had a go at merging those changes in to my work, but for some reason I'm still getting the same message'. I may just need to get round to reading that 'git book' before I do much else...
[Update: I've resolved the merge issue now, but the tests are failing due to the :focus-within issue described below...]

I hope you will find the following notes helpful:

• Where appropriate, I have switched over from spans to buttons to benefit from their native behaviour.

• I have removed any references to .disabled classes in favour of a disabled attribute and have updated the stylesheet accordingly.

• The prev/next buttons are now being properly styled when disabled i.e. grey text (with no change to the background on hover). I have, however, kept a disabled 'up' button as black text (with no hover) as this also acts a heading.

• I removed the cursor: not-allowed style from the disabled calendar-button as this now uses the browser's default style for a disabled button. Let me know if you'd like to keep it, but if so, we should probably add the same style to the clear-button for consistency. I've kept the class references: vdp-datepicker__calendar-button and vdp-datepicker__clear-button to allow people to customise these buttons easily.

• Not sure if I'm missing something here, but I have dismantled the headerConfig object in PickerMixin and am now passing the properties into PickerHeader individually as props. The reason for this is that the isNextDisabled, isPreviousDisabled etc. computed properties were appearing as undefined when inside the mixin's config object and the disabled attribute was therefore not being set correctly on the prev/next buttons. I realise that you originally passed these in these as methods and were then calling them from within PickerHeader. This works, however, as we are now also using isNextDisabled and isPreviousDisabled to determine whether to move the focus up or down on pressing the arrow keys, we probably shouldn't be firing these functions more often than we need to.

• Previously, the DateInput component emitted a close-calendar event on blur. However, as we now want to focus the most relevant part of the calendar when it is opened, I added a defaultFocus method to the pickerMixin's mounted lifecycle hook which determines where to place the focus. Let me know, if you feel these choices should be tweaked in any way. Note that the defaultFocus method is also fired when the prev/next button becomes disabled.

• The typeable calendar retains focus on the input field at all times, regardless of any clicks made within the picker - and as expected it submits the date on blur.

• The inline calendar is not focused initally (to avoid any undesired page scrolls), however, once it has been interacted with, its focus will be determined in the usual way.

• Pressing the escape key now also clears the date as opposed to just closing the calendar. The focus remains on the input field, unless the calendar-button is used (in which case that button is focused).

• In order to close the calendar when it is not focused, I could no longer rely on the input field being blured, so I have made use of the :focus-within pseudo selector to keep track of whether any element within the datepicker is focused. When I say 'keep track of', since $refs are not reactive in Vue, I actually mean 'fire a check-focus event' each time a focused element within the datepicker is blured. The :focus-within selector is currently supported by over 93% of browsers. Is this good enough, or can you think of a better strategy to achieve this functionality?

• Annoyingly, although it works in my browser, I get a [Vue warn]: Error in nextTick: "SyntaxError: ':focus-within' is not a valid selector" error message when running the tests. I tried to overcome this by wrapping the 'offending' code in a try/catch block. Whilst this does remove the error message when testing, it introduces a new error message when I run npm run serve: [!] (plugin buble) TypeError: Cannot read property 'type' of null. It would appear that this is an issue with the rollup-plugin-buble package (which has been deprecated in favour of @rollup/plugin-buble), but I haven't had any immediate luck in trying to resolve the issue!

• Determining how many cells to skip when adjusting the focus with the up/down arrows was a bit of a mental challenge - not made any easier by the recent introduction of the year-picker-range prop(!) - but I think I've cracked it now. :-)

• While working on this, I noticed that the alignment of the cells on the last row of the YearPicker was incorrect when using a rtl language (the cells were on the left instead of the right), so I fixed that by filling in the gap with some blank, disabled cells.

• I've managed to get the existing tests to pass (with a few, hopefully minor, adjustments here and there). I still need to write some tests for the new functionality I have created, but while I work on that, I wanted to share with you the current state-of-play and look forward to hearing any comments or suggestions you may have.

Regards,

Mark
PS. The above changes have caused the minified size of the component to increase from 35 KB to 51 KB (although the css has reduced from 4 KB to 3 KB!). Probably not much that can be done about this, as it's pretty core functionality?

PPS. Unrelated to the above, but the select lists are not displaying correctly on the demo pages as the small css change I made - height: 2.5em -> line-height: 2.5 em - does not appear to have been applied.

[MrWook]

This is a big one :D
I will need some time to go through it.

[mst101]

Hmm... It looks like it won't be possible to use vue-test-utils to test this. See this discussion.

@MrWook - what are your thoughts on how to go about this?

[mst101]

I see you've been busy @MrWook and have managed to fix the buble plugin issue I mentioned above... Well done! 👍

Now that I am a 'git ninja' (?), I have merged your changes into this branch. N.B. I was having some trouble with the PickerMixin tests, so have temporarily commented those out.

I look forward to hearing from you on the other points in due course... 😄

[mst101]

N.B. Ignore that last commit - I'm still trying to figure out how to let slots inform their parent whether any element within them is focused...

[mst101]

@MrWook - FYI - I've merged your latest changes into my local branch and am working through the eslint issues. I can definitely see the benefit of using prettier - it's forced me to take another look at some aspects of my code - and rewrite them in a better way.

Will upload another commit asap.

[MrWook]

Hey,
I'm really sry that i don't have the time to review this PR. I know that you worked a lot for it but unfortunately I'm in a new project for some time and don't have the time any more to review a PR this big. Please bear with me, the time will come when i have the time for it.
Neverless i will publish a new major release so all other changes that you made can finally go live.

[mst101]

No problem... there's no urgency. I'm also not happy with my code just yet.

I feel like I've learned a lot from this project already and really appreciate your efforts in managing it. Thanks also for accepting my last two pull requests. Small ones are a lot easier, eh? As you know, I've been working on a refactor for the disabled and highlighted dates and I suspect those changes will also be helpful in cleaning up the code for the keyboard navigation. I'll try to keep my commits small, so you can follow along more easily.

BTW - I'm part way though reading a book on 'Testing VueJs Components' by Edd Yerbugh and will want to apply the knowledge I've gained there to this branch. It looks like we'll need some end-to-end tests for the keyboard navigation... I've already got some experience with Selenium, but will want to read what Edd Yerburgh has to say about how best to tackle this. So, there's plenty for me to get on with!

Hope your new project goes well. Let's catch up again in a little while...

[mst101]

I'm closing this PR as the commit history is a mess and some things appear to have become corrupted.

I'm working on extracting the logic for focusing a cell to its own class (similar to the disabled/highlighted dates) and will open up a new PR in due course...