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

Improve keyboard navigation for date range selection #1499

Open
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@monokrome
Copy link

monokrome commented Jan 3, 2019

These changes represent an effort to improve the navigation of the date range selection via keyboard. This is primarily modifying the position of the DayPicker so that it is after the currently focused input on the date ranges, and comes with a few small changes to keep reasonable usability for keyboard navigation inside of the DayPicker.

@monokrome

This comment has been minimized.

Copy link

monokrome commented Jan 3, 2019

@majapw Although not ready for review yet, I'm creating this PR as requested so that you can see the current code 😺

@monokrome monokrome force-pushed the tkjr-calendar-tabbing branch from 7ccde16 to 2f1eecb Jan 3, 2019

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 3, 2019

Coverage Status

Coverage decreased (-0.2%) to 84.968% when pulling 764adf5 on tkjr-calendar-tabbing into 3efefd0 on master.

@monokrome monokrome force-pushed the tkjr-calendar-tabbing branch from 2f1eecb to 7fcce50 Jan 3, 2019

Show resolved Hide resolved .gitignore Outdated
Show resolved Hide resolved src/components/DateRangePicker.jsx Outdated
Show resolved Hide resolved src/components/DateRangePickerInput.jsx Outdated
Show resolved Hide resolved src/components/SingleDatePicker.jsx Outdated
Show resolved Hide resolved src/components/SingleDatePicker.jsx Outdated
Show resolved Hide resolved src/components/SingleDatePickerInput.jsx
Show resolved Hide resolved src/components/DateRangePickerInputController.jsx
@@ -27,6 +27,9 @@ import {

const propTypes = forbidExtraProps({
...withStylesPropTypes,

children: PropTypes.node,

This comment has been minimized.

@ljharb

ljharb Jan 3, 2019

Member

do we need to allow any kind of children here?

This comment has been minimized.

@majapw

majapw Jan 4, 2019

Contributor

Yeah, let's restrict this to the DayPicker, DayPickerRangeController, and DayPickerSingleDateController. We can open it up if people ask.

This comment has been minimized.

@monokrome

monokrome Jan 8, 2019

I think that it needs to be here as well as in DatePickerInputController as well, or else we'll need to move around the logic for this.maybeRenderDayPickerWithPortal()

This comment has been minimized.

@majapw

majapw Jan 10, 2019

Contributor

Sorry @monokrome, can you clarify what you mean here?

This comment has been minimized.

@monokrome

monokrome Jan 11, 2019

@majapw Sure! I think that this.maybeRenderDayPickerWithPortal() is rendered as a child of the input component right now. I am happy to refactor this, but it would require additional effort:

return (
<DateRangePickerInput
startDate={startDateString}
startDateId={startDateId}
startDatePlaceholderText={startDatePlaceholderText}
isStartDateFocused={isStartDateFocused}
endDate={endDateString}
endDateId={endDateId}
endDatePlaceholderText={endDatePlaceholderText}
isEndDateFocused={isEndDateFocused}
isFocused={isFocused}
disabled={disabled}
required={required}
readOnly={readOnly}
openDirection={openDirection}
showCaret={showCaret}
showDefaultInputIcon={showDefaultInputIcon}
inputIconPosition={inputIconPosition}
customInputIcon={customInputIcon}
customArrowIcon={customArrowIcon}
customCloseIcon={customCloseIcon}
phrases={phrases}
onStartDateChange={this.onStartDateChange}
onStartDateFocus={this.onStartDateFocus}
onStartDateShiftTab={this.onClearFocus}
onEndDateChange={this.onEndDateChange}
onEndDateFocus={this.onEndDateFocus}
showClearDates={showClearDates}
onClearDates={this.clearDates}
screenReaderMessage={screenReaderMessage}
onKeyDownArrowDown={onKeyDownArrowDown}
onKeyDownQuestionMark={onKeyDownQuestionMark}
isRTL={isRTL}
noBorder={noBorder}
block={block}
small={small}
regular={regular}
verticalSpacing={verticalSpacing}
>
{children}
</DateRangePickerInput>
);

This comment has been minimized.

@majapw

majapw Jan 15, 2019

Contributor

Ahh, I see. It's usually rendering a div and not one of the DayPicker components directly. Okay! Leaving it as a node seem fine then.

@majapw
Copy link
Contributor

majapw left a comment

Yay, nice test coverage! I think overall we'd prefer to use the onTab/onShiftTab callbacks in the DayPicker as the solution for this rather than add this blur handler. In addition, I think the props already exist on the DateRangePickerInput(Controller) that let you know where to put the calendar.

Show resolved Hide resolved .gitignore Outdated
Show resolved Hide resolved src/components/DateRangePicker.jsx Outdated
@@ -27,6 +27,9 @@ import {

const propTypes = forbidExtraProps({
...withStylesPropTypes,

children: PropTypes.node,

This comment has been minimized.

@majapw

majapw Jan 4, 2019

Contributor

Yeah, let's restrict this to the DayPicker, DayPickerRangeController, and DayPickerSingleDateController. We can open it up if people ask.

Show resolved Hide resolved src/components/DateRangePickerInput.jsx Outdated
>
{arrowIcon}
</div>
}

{datePickerIsAfterEndInput || children}

This comment has been minimized.

@majapw

majapw Jan 4, 2019

Contributor

I think this can be {isStartDateFocused && children}

This comment has been minimized.

@monokrome

monokrome Jan 7, 2019

@majapw I'll try it! That's something I didn't notice until I added the other one, so makes sense as long as the input element is taken into account w/ it as well!

onKeyDownArrowDown={onKeyDownArrowDown}
onKeyDownQuestionMark={onKeyDownQuestionMark}
verticalSpacing={verticalSpacing}
small={small}
regular={regular}
/>

{datePickerIsAfterEndInput && children}

This comment has been minimized.

@majapw

majapw Jan 4, 2019

Contributor

I think this can be {isEndDateFocused && children}

This comment has been minimized.

@monokrome
Show resolved Hide resolved src/components/DateRangePickerInputController.jsx
@@ -232,6 +242,11 @@ export default class DateRangePickerInputController extends React.PureComponent

onStartDateFocus() {
const { disabled, onFocusChange } = this.props;

this.setState({

This comment has been minimized.

@majapw

majapw Jan 4, 2019

Contributor

I'm not convinced we need this state value. we can use the isStartDateFocused and isEndDateFocused props instead

This comment has been minimized.

@monokrome

monokrome Jan 7, 2019

Neither was I, I was going to investigate it a bit more after I figured out the tabbing issues (which I think I'll need to do some somewhat-hacky stuff to get working, unfortunately). Thanks :)

// We manually set event because React has not implemented onFocusIn/onFocusOut.
// Keep an eye on https://github.com/facebook/react/issues/6410 for updates
// We use "blur w/ useCapture param" vs "onfocusout" for FF browser support
this.container.addEventListener('blur', this.onFocusOut, true);

This comment has been minimized.

@majapw

majapw Jan 4, 2019

Contributor

As mentioned offline, I think it would be preferable and simpler to not have this blur functionality but instead rely on the tab callbacks in the DayPicker

This comment has been minimized.

@monokrome

monokrome Jan 7, 2019

@majapw This is from @TaeKimJR's implementation before I took on this project, but the weird situation here is that the only context we have from the browser onTab is what element we were focused on before the onTab? document.activeElement seems to be reset to body during the event cycle w/ React. I think e.relatedTarget may be what I'm looking for, but still need to test that theory. The only solution I've found thus far is quite hacky.

@monokrome

This comment has been minimized.

Copy link

monokrome commented Jan 9, 2019

Oops! Need to write test! Will do so tonight :D

@majapw
Copy link
Contributor

majapw left a comment

I'm a bit confused about the comments about the children prop! Maybe we can talk through it tomorrow.

Other than that, it's looking great!

@@ -238,6 +241,11 @@ class DateRangePicker extends React.PureComponent {
});
}

onDayPickerFocusOut(e) {
if (this.dayPickerContainer.contains(e.relatedTarget)) return;

This comment has been minimized.

@majapw

majapw Jan 10, 2019

Contributor

Just want to confirm that clicking on the month navigation, keyboard shortcuts, or any white space in the calendar does not close the picker, yeah?

This comment has been minimized.

@monokrome

monokrome Jan 11, 2019

@majapw Hmm... Apparently it does? Not sure why, I'll look into it. I'd expect this contains check to work but I may need to get a ref to a parent container?

This comment has been minimized.

@monokrome

monokrome Jan 11, 2019

@majapw I've pushed a fix - good find! I also fixed another instance of the same issue.

Show resolved Hide resolved src/components/DateRangePicker.jsx
}

setContainerRef(ref) {
this.container = ref;
}

addDayPickerEventListeners() {
this.removeDayPickerFocusOut = addEventListener(

This comment has been minimized.

@majapw

majapw Jan 10, 2019

Contributor

It would be great if we could add a comment here as to why we're using this approach rather than a React event listener!

This comment has been minimized.

@monokrome
@@ -27,6 +27,9 @@ import {

const propTypes = forbidExtraProps({
...withStylesPropTypes,

children: PropTypes.node,

This comment has been minimized.

@majapw

majapw Jan 10, 2019

Contributor

Sorry @monokrome, can you clarify what you mean here?

Show resolved Hide resolved src/components/DateRangePickerInputController.jsx
Show resolved Hide resolved src/components/SingleDatePicker.jsx
@majapw

This comment has been minimized.

Copy link
Contributor

majapw commented Jan 10, 2019

Hey @monokrome, would be good to get a status update on this tomorrow! I'd like to do a release for @nkinser's change and I'd love to get this in too if it's close.

@monokrome

This comment has been minimized.

Copy link

monokrome commented Jan 11, 2019

Discussed a bit with @majapw, but I want to document why we are using focusoutinstead of tab for anyone else following.

I spent a bit of time trying to find a way to do this with onblur/onfocus/onkeydown, but the event doesn't give us the information that we need. Tabbing out sets document.activeElement to the body element when the next item isn't a form element (possibly in that case as well).

In order to ensure that we are staying safe, I have implemented this using the focusout event on an element ref until facebook/react#6410 is released, at which time we can consider using FocusOut natively. 😺

TaeKimJR and others added some commits Nov 21, 2018

Update src/components/SingleDatePicker.jsx
Co-Authored-By: monokrome <github@monokro.me>

@monokrome monokrome force-pushed the tkjr-calendar-tabbing branch 2 times, most recently from f288841 to e9fb582 Jan 11, 2019

@monokrome monokrome force-pushed the tkjr-calendar-tabbing branch from e9fb582 to a68d154 Jan 11, 2019

monokrome added some commits Jan 11, 2019

@monokrome monokrome force-pushed the tkjr-calendar-tabbing branch from e7d7b82 to b88c380 Jan 14, 2019

@majapw
Copy link
Contributor

majapw left a comment

One major comment (about the children && children line). Everything else looks good!

@@ -238,6 +241,11 @@ class DateRangePicker extends React.PureComponent {
});
}

onDayPickerFocusOut(event) {
if (this.dayPickerContainer.contains(event.relatedTarget || event.target)) return;

This comment has been minimized.

@majapw

majapw Jan 15, 2019

Contributor

event.relatedTarget || event.target
is this a fallback for different browsers?

This comment has been minimized.

@monokrome

monokrome Jan 15, 2019

@majapw I'll add a comment to explain this one better, but it fixes the issue where clicking on one of the days caused the DayPicker to close. In this case, it will set relatedTarget to null, while in the other cases it gives us the correct relatedTarget but an incorrect target.

@@ -27,6 +27,9 @@ import {

const propTypes = forbidExtraProps({
...withStylesPropTypes,

children: PropTypes.node,

This comment has been minimized.

@majapw

majapw Jan 15, 2019

Contributor

Ahh, I see. It's usually rendering a div and not one of the DayPicker components directly. Okay! Leaving it as a node seem fine then.

@@ -564,8 +570,8 @@ export default class DayPickerRangeController extends React.PureComponent {
};

// eslint-disable-next-line react/destructuring-assignment
if (this.state.dateOffset && this.state.dateOffset.start && this.state.dateOffset.end) {
modifiers = this.deleteModifierFromRange(modifiers, this.state.dateOffset.start, this.state.dateOffset.end, 'hovered-offset');
if (this.state.dateOffset && currentDateOffset.start && currentDateOffset.end) {

This comment has been minimized.

@majapw

majapw Jan 15, 2019

Contributor

nit, update this.state.dateOffset to currentDateOffset

This comment has been minimized.

@monokrome

monokrome Jan 15, 2019

Seems like a good idea! 😺

@@ -251,7 +255,9 @@ export default class SingleDatePickerInputController extends React.PureComponent
small={small}
regular={regular}
verticalSpacing={verticalSpacing}
/>
>
{children && children}

This comment has been minimized.

@majapw

majapw Jan 15, 2019

Contributor

Should this be {focused && children}?

This comment has been minimized.

@monokrome

monokrome Jan 15, 2019

@majapw I don't think so, because there wasn't a conditioner earlier. The case we're checking for here is whether children is null. In the case that focused is true, I still think that it's still possible to have children set to null

EDIT: The other solution which we could go with is making children required, but I wasn't sure if that would cause breaking backward-incompatible API changes?

This comment has been minimized.

@majapw

majapw Jan 15, 2019

Contributor

I don't think we have to do {children && children} to catch for the case that children is null. {children} on its own will have the correct behavior when the value is undefined or null (and it should never be false).

My thought was that {focused && children} has more parity with what is happening in the DateRangePicker (https://github.com/airbnb/react-dates/pull/1499/files/764adf5c892f6802889c9f0ef2886c70e0c0ee1e#diff-3baa0cf2e719c1385850daba9169f742R248 and https://github.com/airbnb/react-dates/pull/1499/files/764adf5c892f6802889c9f0ef2886c70e0c0ee1e#diff-3baa0cf2e719c1385850daba9169f742R272). The calendar should not be showing if the focused value is false.

@majapw

This comment has been minimized.

Copy link
Contributor

majapw commented Jan 16, 2019

Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment