Conversation
2b6cc8a to
80cbe95
Compare
| onClick={e => this.onDayClick(day, e)} | ||
| tabIndex={isFocused ? 0 : -1} | ||
| > | ||
| {renderDay ? renderDay(day) : day.format('D')} |
There was a problem hiding this comment.
This may cause problems due to the fact that renderDay could return anything. Thoughts?
There was a problem hiding this comment.
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.
I'm still not super sure how that would work? What would the Day component look like?
There was a problem hiding this comment.
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.
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.
You can do that, but you'll need to make sure that you add all the related keyboard handling to that element
| componentDidUpdate(prevProps) { | ||
| const { focused } = this.props; | ||
| if (prevProps.focused === focused) return; | ||
| const { focused, hasFocus } = this.props; |
There was a problem hiding this comment.
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
| break; | ||
|
|
||
| case 27: // Escape | ||
| onBlur(); |
There was a problem hiding this comment.
So this takes focus back to the input, so maybe this name is not quite right.
| } | ||
|
|
||
| // 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.
Incidentally, the way this behavior manifests itself is weird AF
There was a problem hiding this comment.
where does the focus happen after?
There was a problem hiding this comment.
(and maybe add a comment with a see [insert function name here])
| } | ||
| } | ||
|
|
||
| onPrevMonthClick(e, nextFocusedDate) { |
There was a problem hiding this comment.
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.
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.
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. |
|
|
||
| &:active { | ||
| background: darken($react-dates-color-white, 5%); | ||
| // outline: 0; |
| $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.
Isn't this changing the shade of gray by a tiny bit? Why not stick with lighten?
| break; | ||
|
|
||
| case '?': | ||
| this.toggleKeyboardShortcuts(); |
There was a problem hiding this comment.
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.
Yeah I kind of wanted to do this... but it was somewhat painful! I will think more on it.
|
|
||
|
|
||
| let availabilityText = ''; | ||
| if (BLOCKED_MODIFIER in modifiers) { |
There was a problem hiding this comment.
When will modifiers not include BLOCKED_MODIFIER?
There was a problem hiding this comment.
if you roll your own DayPicker wrapper and define your own modifiers
| }); | ||
| } | ||
|
|
||
| getFirstFocusableDay(newMonth) { |
There was a problem hiding this comment.
SOON. first I wanted comments in case I was horribly off the mark.
| super(props); | ||
|
|
||
| let focusedInput = null; | ||
| let selectedInput = null; |
There was a problem hiding this comment.
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.
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.
backwardok
left a comment
There was a problem hiding this comment.
Hard to get a good sense without actually trying this out, but added some comments from a quickish perusal
| } | ||
|
|
||
| .DayPickerKeyboardShortcuts__show { | ||
| border-top: 26px solid $react-dates-color-white; |
There was a problem hiding this comment.
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.
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.
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.
Yeah, let's put a pin in that for a future update.
| const { selected, date } = this.state; | ||
|
|
||
| const props = omit(this.props, [ | ||
| 'autoFocus', |
There was a problem hiding this comment.
Why do these need to be removed?
There was a problem hiding this comment.
also instead of omit, this could use object destructuring and disable no-unused-vars
There was a problem hiding this comment.
@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.
Maybe a comment before that line so it's understood?
| availabilityText = modifiers[BLOCKED_MODIFIER](day) ? unavailable : available; | ||
| } | ||
|
|
||
| const ariaLabel = `${availabilityText} ${day.format('dddd')}. ${day.format('LL')}`; |
There was a problem hiding this comment.
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.
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.
| inputValue: '', | ||
| screenReaderMessage: '', | ||
| focused: false, | ||
| focused: false, // handles actual DOM focus |
There was a problem hiding this comment.
feels like these comments shouldn't need to be duplicated within the defaultProps definition
| ))} | ||
| </ul> | ||
|
|
||
| <button |
There was a problem hiding this comment.
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
| @@ -0,0 +1,176 @@ | |||
| const closeDatePicker = 'Close'; | |||
| const focusStartDate = 'Focus on start date'; | |||
There was a problem hiding this comment.
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.
We're gonna run this through a copy person before we upgrade react-dates in Airbnb, so maybe let's leave it til then?
| 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.
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.
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?
| 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.
maybe easier wording as "move forward or backward one day"
There was a problem hiding this comment.
or "go back or advance one day"
There was a problem hiding this comment.
(Decrement and increment feel unusual for something that's not a strict number)
There was a problem hiding this comment.
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.
| 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.
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
ljharb
left a comment
There was a problem hiding this comment.
All the other review comments look good also.
| const { selected, date } = this.state; | ||
|
|
||
| const props = omit(this.props, [ | ||
| 'autoFocus', |
There was a problem hiding this comment.
also instead of omit, this could use object destructuring and disable no-unused-vars
| ref={(ref) => { this.buttonRef = ref; }} | ||
| className="CalendarDay__button" | ||
| aria-label={ariaLabel} | ||
| onMouseEnter={e => this.onDayMouseEnter(day, e)} |
There was a problem hiding this comment.
all event callbacks ideally should use explicit return, since we don't want them to return anything
There was a problem hiding this comment.
Whoops! I like, usually do this so this must have just slipped my radar.
| 'CalendarMonthGrid--animating': isAnimating, | ||
| }); | ||
|
|
||
| const style = Object.assign({ |
| } = this.props; | ||
|
|
||
| switch (e.key) { | ||
| case 'Tab': |
There was a problem hiding this comment.
this would be shorter and imo cleaner as if/else instead of a switch/case
| onNextMonthClick = this.multiplyScrollableMonths; | ||
| } else { | ||
| onNextMonthClick = this.onNextMonthClick; | ||
| onNextMonthClick = e => this.onNextMonthClick(null, e); |
| return Object.keys(defaultPhrases).reduce((phrases, key) => { | ||
| const phrasesWithNewKey = { ...phrases }; | ||
| phrasesWithNewKey[key] = PropTypes.node; | ||
| return phrasesWithNewKey; |
There was a problem hiding this comment.
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.
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.
Welp we probably should get a designer for this project. X_x
There was a problem hiding this comment.
This is breaking functionality in IE. #1606
| {screenReaderMessage} | ||
| </p> | ||
| } | ||
| <p id={screenReaderMessageId} className="screen-reader-only"> |
There was a problem hiding this comment.
since screenReaderMessage isn't required, it seems like you should only render this if it exists.
| disabled={disabled} | ||
| readOnly={isTouch} | ||
| required={required} | ||
| aria-describedby={screenReaderMessage && screenReaderMessageId} |
There was a problem hiding this comment.
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
| }); | ||
| } | ||
|
|
||
| onDayPickerBlur() { |
There was a problem hiding this comment.
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.
why does showing the keyboard shortcuts panel focus the day picker?
There was a problem hiding this comment.
The panel is part of the DayPicker. This is mainly to indicate the dichotomy between the input being focused and the dropdown being focused.
| this.toggleKeyboardShortcuts(); | ||
| break; | ||
|
|
||
| case 'Escape': |
There was a problem hiding this comment.
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.
Okeedoke, I will address that.
| } | ||
|
|
||
| // 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.
where does the focus happen after?
| } | ||
|
|
||
| // 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.
(and maybe add a comment with a see [insert function name here])
| const { currentMonth } = this.state; | ||
|
|
||
| const month = newMonth || currentMonth; | ||
| return !day.isBefore(month.clone().startOf('month')) && |
There was a problem hiding this comment.
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 consts
| screenReaderMessage, | ||
| } = this.props; | ||
|
|
||
| const screenReaderText = screenReaderMessage || phrases.keyboardNavigationInstructions; |
There was a problem hiding this comment.
similar comment here as in other file
|
is the (update) added as an item on TODO list |
a3639b8 to
79e5803
Compare
| } | ||
|
|
||
| onPrevMonthClick(e) { | ||
| onKeyDown(e) { |
There was a problem hiding this comment.
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.
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.
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.
The panel is part of the DayPicker. This is mainly to indicate the dichotomy between the input being focused and the dropdown being focused.
| focusedDate, | ||
| }); | ||
| } else { | ||
| this.setState({ focusedDate: null }); |
There was a problem hiding this comment.
Hmm, good point. I will look into what's happening with that and if they get out of sync or anything.
| this.toggleKeyboardShortcuts(); | ||
| break; | ||
|
|
||
| case 'Escape': |
There was a problem hiding this comment.
Okeedoke, I will address that.
| const { currentMonth } = this.state; | ||
|
|
||
| const month = newMonth || currentMonth; | ||
| return !day.isBefore(month.clone().startOf('month')) && |
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.
What's the plan for integrating these with the internal translation setup?
There was a problem hiding this comment.
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.

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