-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 Accessibility #301
Conversation
2b6cc8a
to
80cbe95
Compare
src/components/CalendarDay.jsx
Outdated
onClick={e => this.onDayClick(day, e)} | ||
tabIndex={isFocused ? 0 : -1} | ||
> | ||
{renderDay ? renderDay(day) : day.format('D')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may cause problems due to the fact that renderDay
could return anything. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We originally decided my suggestion of using a component here like <Day>{renderDay ? renderDay(day) : day.format('D')}</Day>
so that a propType could be applied to the return value - is that perhaps worth doing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not super sure how that would work? What would the Day
component look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function Day({ children }) {
return React.Children.only(children);
}
Day.propTypes = {
children: PropTypes.node.isRequired,
};
and the propType can be as fancy as we want :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my only Q is that given that buttons only allow "Phrasing content" (https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_categories#Phrasing_content) so while we could create a proptype to limit the node to one of these dom elements, that actually seems pretty hard to do. @backwardok with this is mind, do you think it is more reasonable to put a role="button"
on the td
maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do that, but you'll need to make sure that you add all the related keyboard handling to that element
src/components/DateInput.jsx
Outdated
@@ -59,10 +61,10 @@ export default class DateInput extends React.Component { | |||
} | |||
|
|||
componentDidUpdate(prevProps) { | |||
const { focused } = this.props; | |||
if (prevProps.focused === focused) return; | |||
const { focused, hasFocus } = this.props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to come up with some better nomenclature to denote component "focus" (ie which is currently being selected) vs. DOM "focus" because this PR separates the two.
80cbe95
to
617e4b7
Compare
src/components/DayPicker.jsx
Outdated
break; | ||
|
||
case 27: // Escape | ||
onBlur(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this takes focus back to the input, so maybe this name is not quite right.
src/components/DayPicker.jsx
Outdated
} | ||
|
||
// If there was a month transition, do not update the focused date until the transition has | ||
// completed. Otherwise, attempting to focus on a DOM node may interrupt the CSS animation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incidentally, the way this behavior manifests itself is weird AF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does the focus happen after?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and maybe add a comment with a see [insert function name here]
)
src/components/DayPicker.jsx
Outdated
} | ||
} | ||
|
||
onPrevMonthClick(e, nextFocusedDate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with the regular button click event... nextFocusedDate
is actually like the native event object or something... which is weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're passing this here so that we have access to what we want to focus on after the month transition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e
should always be the last argument
3cc2495
to
d0a4f58
Compare
d0a4f58
to
ee36ce8
Compare
Hi @backwardok @wyattdanger @ljharb @lencioni! I still need to write tests, but I'd love to get some feedback before I do. Would y'all be able to take a look at this PR? I am pushing real hard to get it in this week. This is the MVP. I think there will likely be some polish work that still needs to be done. |
css/DayPickerNavigation.scss
Outdated
@@ -23,6 +23,7 @@ | |||
|
|||
&:active { | |||
background: darken($react-dates-color-white, 5%); | |||
// outline: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove
css/variables.scss
Outdated
@@ -16,6 +17,7 @@ $react-dates-color-gray: #565a5c !default; | |||
$react-dates-color-gray-dark: darken($react-dates-color-gray, 10.5%) !default; | |||
$react-dates-color-gray-light: lighten($react-dates-color-gray, 17.8%) !default; // #82888a | |||
$react-dates-color-gray-lighter: lighten($react-dates-color-gray, 45%) !default; // #cacccd | |||
$react-dates-color-gray-lightest: #eeeeee; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this changing the shade of gray by a tiny bit? Why not stick with lighten?
src/components/DayPicker.jsx
Outdated
break; | ||
|
||
case '?': | ||
this.toggleKeyboardShortcuts(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably nitpicking, but can there be an easy way to detect if the shortcut panel was activated by DateInput, hence if we're closing the shortcut panel it would focus back on the DateInput? (I tried played with it but lost myself in managing props)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I kind of wanted to do this... but it was somewhat painful! I will think more on it.
src/components/CalendarDay.jsx
Outdated
|
||
|
||
let availabilityText = ''; | ||
if (BLOCKED_MODIFIER in modifiers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When will modifiers
not include BLOCKED_MODIFIER
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you roll your own DayPicker
wrapper and define your own modifiers
@@ -157,9 +174,37 @@ export default class DayPickerRangeController extends React.Component { | |||
}); | |||
} | |||
|
|||
getFirstFocusableDay(newMonth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you plan to add test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SOON. first I wanted comments in case I was horribly off the mark.
examples/DateRangePickerWrapper.jsx
Outdated
}; | ||
|
||
class DateRangePickerWrapper extends React.Component { | ||
constructor(props) { | ||
super(props); | ||
|
||
let focusedInput = null; | ||
let selectedInput = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming from focused
to selected
creates a pretty big diff that might make merging/rebasing other PRs hairy. If selected
is required in a11y props, can the renaming be limited to external facing html props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This nomenclature change is not ostensibly required for a11y purposes. The reason I am making it is because this PR introduces the concept of DOM focus as separate concept from the focusedInput
prop and it makes sense to have a legitimately different name for the two things and have focus refer to, well, actual focus.
If there are any PRs in particular that you are concerned about, I can help with the rebasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to get a good sense without actually trying this out, but added some comments from a quickish perusal
css/DayPickerKeyboardShortcuts.scss
Outdated
} | ||
|
||
.DayPickerKeyboardShortcuts__show { | ||
border-top: 26px solid $react-dates-color-white; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are a number of size numbers in this stylesheet that seem like non standard units. why are these numbers so specific? are any of these intended to be the same on purpose? can we move some of these into vars for more clarity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean tbh, they're made up numbers loosely based on the padding in the DayPicker
which like, are also made up and not particularly grid-based or anything. I am not a designer and I can pull these out into variables... but I don't think there will really be rhyme or reason until we get a designer to take a look.
|
||
&:hover, | ||
&:focus { | ||
fill: $react-dates-color-gray-light; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps something to do in a future update, but one thing we came across during a11y testing is that users who have ie9 and enable a setting that essentially turns off background colors and text colors will end up not being able to notice the difference here. We'll likely need to do something where we change the border color or add an outline to the element for more visibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's put a pin in that for a future update.
examples/SingleDatePickerWrapper.jsx
Outdated
const { selected, date } = this.state; | ||
|
||
const props = omit(this.props, [ | ||
'autoFocus', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do these need to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also instead of omit
, this could use object destructuring and disable no-unused-vars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb This is in the examples, not in the core code so this only adds lodash.omit
as a dev dependency. I think using omit makes things cleaner and more readable than objecting destructuring and disabling the lint rule.
@backwardok these are props on the example wrapper, not on the DateRangePicker
explicitly because they have to do with instantiating the state on this component. Because we use forbidExtraProps
I wanna clean them out before passing the props through to the DRP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a comment before that line so it's understood?
src/components/CalendarDay.jsx
Outdated
availabilityText = modifiers[BLOCKED_MODIFIER](day) ? unavailable : available; | ||
} | ||
|
||
const ariaLabel = `${availabilityText} ${day.format('dddd')}. ${day.format('LL')}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious whether or not it's more valuable for screen reader users to hear the date first or the availability first. Might be something we want to run through user research at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I mean I am also going to try to get a copy person to look at the text itself, we'll need to get translations done, and so on. :D We will def add this to the fray.
src/components/DateInput.jsx
Outdated
}); | ||
|
||
const defaultProps = { | ||
placeholder: 'Select Date', | ||
displayValue: '', | ||
inputValue: '', | ||
screenReaderMessage: '', | ||
focused: false, | ||
focused: false, // handles actual DOM focus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels like these comments shouldn't need to be duplicated within the defaultProps
definition
))} | ||
</ul> | ||
|
||
<button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is visually one of the first things in the dialog, this should probably equivalently be one of the first things in the markup
src/defaultPhrases.js
Outdated
@@ -0,0 +1,176 @@ | |||
const closeDatePicker = 'Close'; | |||
const focusStartDate = 'Focus on start date'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this phrase feels a little off to me - without running this, I have a hard time understanding what it is this is describing. If I were to hear this phrase being said, I wouldn't understand why I would need to focus on the start date. Seems like this should be something more like "Select start date", but I'm not sure if that's exactly what this is accomplishing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're gonna run this through a copy person before we upgrade react-dates in Airbnb, so maybe let's leave it til then?
src/defaultPhrases.js
Outdated
const keyboardShortcuts = 'Keyboard Shortcuts'; | ||
const showKeyboardShortcutsPanel = 'Show the keyboard shortcuts panel'; | ||
const hideKeyboardShortcutsPanel = 'Hide the keyboard shortcuts panel'; | ||
const toggleKeyboardShortcutsPanel = 'Toggle the keyboard shortcuts panel'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the shortcut panel being shown non modal? Seems like you shouldn't be able to access this button when the shortcut panel is shown, in which case it should always be the "show" version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you shouldn't be able to access this button when the shortcut panel is shown
I am not sure how to make that happen, but that's certainly not the behavior rn. Could you maybe run the code and lemme know what you think?
src/defaultPhrases.js
Outdated
const escape = 'Escape'; | ||
const questionMark = 'Question mark'; | ||
const selectFocusedDate = 'Select the currently focused date'; | ||
const moveFocusByOneDay = 'Decrement/Increment currently focused day by 1 day'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe easier wording as "move forward or backward one day"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or "go back or advance one day"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Decrement and increment feel unusual for something that's not a strict number)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we can nit the text, but it would probs make sense until someone can look at the copy and give us recs.
src/defaultPhrases.js
Outdated
const moveFocusByOneWeek = 'Decrement/Increment currently focused day by 1 week'; | ||
const moveFocusByOneMonth = 'Decrement/Increment currently focused day by 1 month'; | ||
const moveFocustoStartAndEndOfWeek = 'Navigate to the beginning or end of the currently focused week'; | ||
const returnFocusToInput = 'Return focus to the input field'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be "Close date picker", since returning focus isn't really what you're trying to do when you want to close the date picker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the other review comments look good also.
examples/SingleDatePickerWrapper.jsx
Outdated
const { selected, date } = this.state; | ||
|
||
const props = omit(this.props, [ | ||
'autoFocus', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also instead of omit
, this could use object destructuring and disable no-unused-vars
src/components/CalendarDay.jsx
Outdated
ref={(ref) => { this.buttonRef = ref; }} | ||
className="CalendarDay__button" | ||
aria-label={ariaLabel} | ||
onMouseEnter={e => this.onDayMouseEnter(day, e)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all event callbacks ideally should use explicit return, since we don't want them to return anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops! I like, usually do this so this must have just slipped my radar.
src/components/CalendarMonthGrid.jsx
Outdated
'CalendarMonthGrid--vertical': orientation === VERTICAL_ORIENTATION, | ||
'CalendarMonthGrid--vertical-scrollable': orientation === VERTICAL_SCROLLABLE, | ||
'CalendarMonthGrid--animating': isAnimating, | ||
}); | ||
|
||
const style = Object.assign({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use object spread here?
src/components/DateInput.jsx
Outdated
} = this.props; | ||
|
||
switch (e.key) { | ||
case 'Tab': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be shorter and imo cleaner as if/else instead of a switch/case
src/components/DayPicker.jsx
Outdated
} = this.props; | ||
|
||
let onNextMonthClick; | ||
if (orientation === VERTICAL_SCROLLABLE) { | ||
onNextMonthClick = this.multiplyScrollableMonths; | ||
} else { | ||
onNextMonthClick = this.onNextMonthClick; | ||
onNextMonthClick = e => this.onNextMonthClick(null, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explicit return?
src/utils/getPhrasePropTypes.js
Outdated
return Object.keys(defaultPhrases).reduce((phrases, key) => { | ||
const phrasesWithNewKey = { ...phrases }; | ||
phrasesWithNewKey[key] = PropTypes.node; | ||
return phrasesWithNewKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { ...phrases, [key]: PropTypes.node };
7c1e502
to
fd54581
Compare
@airbnb/webinfra @backwardok @ljharb I split out some of the work and broke up the PR into two commits (one for the breaking name change and one for all the a11y work). Would appreciate another set of eyes. Once again, I still have to write tests! but that is incoming. |
aria-label={ariaLabel} | ||
onMouseEnter={(e) => { this.onDayMouseEnter(day, e); }} | ||
onMouseLeave={(e) => { this.onDayMouseLeave(day, e); }} | ||
onMouseUp={(e) => { e.currentTarget.blur(); }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for the future - we should just have a nice looking focus state such that doing this isn't necessary. Really don't like that we have this pattern :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welp we probably should get a designer for this project. X_x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is breaking functionality in IE. #1606
src/components/DateInput.jsx
Outdated
{screenReaderMessage} | ||
</p> | ||
} | ||
<p id={screenReaderMessageId} className="screen-reader-only"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since screenReaderMessage
isn't required, it seems like you should only render this if it exists.
src/components/DateInput.jsx
Outdated
@@ -136,14 +157,12 @@ export default class DateInput extends React.Component { | |||
disabled={disabled} | |||
readOnly={isTouch} | |||
required={required} | |||
aria-describedby={screenReaderMessage && screenReaderMessageId} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you were running into issues before where this was coming back as '' and still adding the attribute, then you might consider having the default value of screenReaderMessage
as null
or undefined
so that this works
src/components/DateRangePicker.jsx
Outdated
}); | ||
} | ||
|
||
onDayPickerBlur() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any potential for this to be triggered as a result of triggering the keyboard shortcut modal? or can you not trigger it from the input itself?
showKeyboardShortcutsPanel() { | ||
this.setState({ | ||
isDateRangePickerInputFocused: false, | ||
isDayPickerFocused: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does showing the keyboard shortcuts panel focus the day picker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The panel is part of the DayPicker. This is mainly to indicate the dichotomy between the input being focused and the dropdown being focused.
src/components/DayPicker.jsx
Outdated
this.toggleKeyboardShortcuts(); | ||
break; | ||
|
||
case 'Escape': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing to be careful with here - I would expect that if the keyboard shortcut modal was open on top of the date picker modal, that hitting escape would close the shortcut modal and hitting escape again would close the date picker modal. It seems like hitting escape once here does both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okeedoke, I will address that.
src/components/DayPicker.jsx
Outdated
} | ||
|
||
// If there was a month transition, do not update the focused date until the transition has | ||
// completed. Otherwise, attempting to focus on a DOM node may interrupt the CSS animation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does the focus happen after?
src/components/DayPicker.jsx
Outdated
} | ||
|
||
// If there was a month transition, do not update the focused date until the transition has | ||
// completed. Otherwise, attempting to focus on a DOM node may interrupt the CSS animation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and maybe add a comment with a see [insert function name here]
)
src/components/DayPicker.jsx
Outdated
const { currentMonth } = this.state; | ||
|
||
const month = newMonth || currentMonth; | ||
return !day.isBefore(month.clone().startOf('month')) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessary to fix, but it might make this line a lot easier to read and understand if you pull out the month.clone()....
pieces into their own const
s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call. 👍
screenReaderMessage, | ||
} = this.props; | ||
|
||
const screenReaderText = screenReaderMessage || phrases.keyboardNavigationInstructions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar comment here as in other file
is the (update) added as an item on TODO list |
a3639b8
to
79e5803
Compare
src/components/DayPicker.jsx
Outdated
} | ||
|
||
onPrevMonthClick(e) { | ||
onKeyDown(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should stop propagation here to prevent shenanigans
aria-label={ariaLabel} | ||
onMouseEnter={(e) => { this.onDayMouseEnter(day, e); }} | ||
onMouseLeave={(e) => { this.onDayMouseLeave(day, e); }} | ||
onMouseUp={(e) => { e.currentTarget.blur(); }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welp we probably should get a designer for this project. X_x
@@ -81,13 +87,29 @@ export default class DateInput extends React.Component { | |||
} | |||
|
|||
onKeyDown(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So a problem with using throttle
... while it doesn't matter for the down arrow or for tabbing, typing ?
(which is a shortcut for opening the keyboard shortcuts panel), sometimes works but sometimes just types a question mark into the input. :|
The basic issue is that there's an onChange
handler and an onKeyDown
handler attached to the input. Without throttling the order is guaranteed... with throttling, the order is more of a question mark. I think that as a result, this is an optimization we need to think a little bit more on.
showKeyboardShortcutsPanel() { | ||
this.setState({ | ||
isDateRangePickerInputFocused: false, | ||
isDayPickerFocused: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The panel is part of the DayPicker. This is mainly to indicate the dichotomy between the input being focused and the dropdown being focused.
src/components/DayPicker.jsx
Outdated
focusedDate, | ||
}); | ||
} else { | ||
this.setState({ focusedDate: null }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good point. I will look into what's happening with that and if they get out of sync or anything.
src/components/DayPicker.jsx
Outdated
this.toggleKeyboardShortcuts(); | ||
break; | ||
|
||
case 'Escape': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okeedoke, I will address that.
src/components/DayPicker.jsx
Outdated
const { currentMonth } = this.state; | ||
|
||
const month = newMonth || currentMonth; | ||
return !day.isBefore(month.clone().startOf('month')) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call. 👍
f40732e
to
a8e86c8
Compare
350fb4b
to
9f56e07
Compare
- Adds screen reader text to the input to describe how to enter the calendar - Adds arrow navigation to the calendar itself - Adds a keyboard shortcuts panel that pops up on top of the calendar - Adds screen reader text to each CalendarDay
1 similar comment
Hey @airbnb/webinfra, this PR is ready to go! Please take a look and give me your feedback! |
const chooseAvailableStartDate = ({ checkin_date }) => `Choose ${checkin_date} as your check-in date. It's available.`; | ||
|
||
// eslint-disable-next-line camelcase | ||
const chooseAvailableEndDate = ({ checkout_date }) => `Choose ${checkout_date} as your check-out date. It's available.`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the plan for integrating these with the internal translation setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So when you look at getPhrase
, you'll see that each phrase accepts a function, node, or string and behaves accordingly. My plan is to add code to our wrappers that automatically pulls in our translations appropriately and create a phrase bundle that can easily be spread onto any page.
We might want to do this for the more base-level components (ie DayPickerRangeController) we're using as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This is an MVP of keyboard accessibility! I imagine there will still be some polish to take care of, especially when it comes to managing focus and aria roles throughout. BUT I would really like to get this merged so that we can iterate more on it (and add better messaging/validation/accessibility to the inputs themselves as well).
HAVE A GIF!
onNextMonthClick
/onPrevMonthClick
????
shortcutDayPickerRangeController
@airbnb/webinfra plz give me some love. #