-
-
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
[Fix] for Issue #557 moving focus to the keyboard shortcuts panel when opened #825
Conversation
const { focusedDate } = this.state; | ||
|
||
if ( | ||
(!prevProps.isFocused && isFocused && !focusedDate) || | ||
(!prevProps.showKeyboardShortcuts && showKeyboardShortcuts) |
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 removed this line because it was pulling the focus back to the DayPicker after it had just been moved to 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.
@majapw, any idea why this condition was here in the first place?
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.
hmmm, i don't remember. Focusing in the keyboard shortcuts panel may have evolved since I first integrated it and this may be a relic of the initial a11y change.
@@ -38,7 +38,7 @@ function KeyboardShortcutRow({ | |||
<span | |||
{...css(styles.KeyboardShortcutRow_key)} | |||
role="img" | |||
aria-label={label} | |||
aria-label={`${label} `} |
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.
Adding in the space here inserts a space between the screen reader reading the label and then the {action} from the <div>
below. Previously it was reading like:
"Enter keySelect the date in focus"
Basically it was merging the last word of one to the first word of the next and making it hard to understand when read.
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.
would adding :
or similar instead of a space have the same effect?
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.
Adding a :
works great with Safari but in Chrome the punctuation is read so it sounds like:
"Enter key colon Select the date in focus"
I tried a ,
instead and that seems to work nicely and consistently in both browsers. So I'll change this to a ,
instead of a space
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.
awesome!
@@ -404,6 +404,7 @@ class SingleDatePicker extends React.Component { | |||
renderCalendarInfo={renderCalendarInfo} | |||
isFocused={isDayPickerFocused} | |||
showKeyboardShortcuts={showKeyboardShortcuts} | |||
onBlur={this.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.
This was another important miss I found in functionality between the DateRangePicker but not the SingleDatePicker. This was the missing piece to the focus returning back to the DateInput when that was where the keyboard shortcuts panel was triggered to open.
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.
It seems like the getKeyboardShortcuts
change is performance-motivated, and could/should be in a separate commit from the fix itself - it'd be easier for me to review with them separate. Do you mind doing that?
unicode: '↵', | ||
label: phrases.enterKey, | ||
action: phrases.selectFocusedDate, | ||
}, |
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 indentation here is a little off; if it's going to start with [{
, then each line needs to be }, {
- otherwise the first {
needs to be on its own line.
class DayPickerKeyboardShortcuts extends React.Component { | ||
constructor(...args) { | ||
super(...args); | ||
|
||
this.onClick = this.onClick.bind(this); | ||
this.keyboardShortcuts = getKeyboardShortcuts(this.props.phrases); |
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 also need to happen in componentWillReceiveProps
.
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.
Moving this was not performance motivated, I just wanted to make render() a little more concise. Sounds like I should just move it back.
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 think it's a great idea to move it; but if it's not in the render path, then it has to be updated in both of the places where props change.
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.
Ok I'll make that change in a separate commit then 👍
33b133f
to
8396f88
Compare
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.
Thanks, having it in two commits made it much easier to review!
const { focusedDate } = this.state; | ||
|
||
if ( | ||
(!prevProps.isFocused && isFocused && !focusedDate) || | ||
(!prevProps.showKeyboardShortcuts && showKeyboardShortcuts) |
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.
@majapw, any idea why this condition was here in the first place?
this.setShowKeyboardShortcutsButtonRef = this.setShowKeyboardShortcutsButtonRef.bind(this); | ||
this.setHideKeyboardShortcutsButtonRef = this.setHideKeyboardShortcutsButtonRef.bind(this); | ||
this.handleFocus = this.handleFocus.bind(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.
this.handleFocus
doesn't seem to be passed anywhere as a prop - can this binding be removed?
if (e.key === 'Enter') { | ||
e.preventDefault(); | ||
} else if (e.key === 'Space') { | ||
this.onShowKeyboardShortcutsButtonClick(); |
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 pass the e
through?
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 function doesn't use the event object at all, you still prefer I pass it on though?
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 think it'd be more future-proof if it ever needed it in the future. Not a huge deal obv.
@@ -110,6 +157,7 @@ class DayPickerKeyboardShortcuts extends React.Component { | |||
return ( | |||
<div> | |||
<button | |||
id="showKeyboardShortcutsButton" |
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.
it seems like this ID only exists so it can be targeted in tests; if so, please remove it - there's surely another way it can be located.
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.
Ok I'll remove and update the test(s)
onKeyDown(e) { | ||
const { closeKeyboardShortcutsPanel } = this.props; | ||
|
||
e.stopPropagation(); |
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 propagation need to be stopped 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.
It's being done that way in DayPicker's onKeyDown()
so figured it would probably be needed here as well.
@@ -154,26 +210,25 @@ class DayPickerKeyboardShortcuts extends React.Component { | |||
</div> | |||
|
|||
<button | |||
id="hideKeyboardShortcutsButton" |
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 this one
<ul {...css(styles.DayPickerKeyboardShortcuts__list)}> | ||
<ul | ||
{...css(styles.DayPickerKeyboardShortcuts__list)} | ||
id="DayPickerKeyboardShortcuts__description" |
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 not sure if this is a pattern we already follow or not; but an ID must be unique on the page, and there should be able to be multiple date of these in the DOM at once - is there another way this can be linked?
@@ -38,7 +38,7 @@ function KeyboardShortcutRow({ | |||
<span | |||
{...css(styles.KeyboardShortcutRow_key)} | |||
role="img" | |||
aria-label={label} | |||
aria-label={`${label} `} |
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.
would adding :
or similar instead of a space have the same effect?
I just want to follow up on this and make sure there weren't any more outstanding actions on my part that you may be waiting for. I think I got it all but if not let me know! |
b43aaf8
to
023c50c
Compare
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.
LGTM
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 great, and you're a hero for doing this hella important work! :)
I think the content of the code is good to go, but I added a few places where I think it would be good to either at a comment in the code or on the PR so as to leave a bit more of a paper trail as to what's going on.
|
||
case 'ArrowUp': | ||
case 'ArrowDown': | ||
e.stopPropagation(); |
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 you leave a comment in here to explain what's going on? I'm not sure it's self-evident
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.
Yep no problem!
@@ -38,7 +38,7 @@ function KeyboardShortcutRow({ | |||
<span | |||
{...css(styles.KeyboardShortcutRow_key)} | |||
role="img" | |||
aria-label={label} | |||
aria-label={`${label},`} |
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 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 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 can add a comment to the code on this as well.
…hortcuts panel receives focus, is read appropriately by screen readers, prevents focus from accidentally moving back to the other elements without use of the proper shortcuts, and then returns focus to the appropriate place when closed.
…oardShortcutsButtonClick() called from onKeyDown. 2) Change the aria-label on the unicode value in the keyboard shortcuts to be followed by a comma instead of a space.
…pen. Only stopping the propagation on the ArrowUp and ArrowDown keys so that the default behavior of scrolling the dialog is not prevented.
023c50c
to
c742e76
Compare
Looking in to build failures now, all I did was rebase on master to pick up any recent changes and then add comments... |
Linting error: 111:1 error Line 111 exceeds the maximum line length of 100 max-len 🤦♀️ |
c742e76
to
121c2e0
Compare
…t handling for keyboard shortcuts
121c2e0
to
b3d3de1
Compare
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.
Wooooot! :)
Yay! 🎉 😌 |
Would it be cool to go ahead and push a version update now that this is in? Patch? I can do it if y'all are busy but didn't want to assume in case you were holding off for something on its way. |
v15.1.0 is out! |
Modified aria attributes and focusing behavior so that the keyboard shortcuts panel receives focus, is read appropriately by screen readers, prevents focus from accidentally moving back to the other elements without use of the proper shortcuts, and then returns focus to the appropriate place when closed.
I tested this on Chrome and Safari with Mac VoiceOver and FireFox with no screen reader. It seems that all the keyboard navigation and screen reading is working consistently across them all now. Triggering the keyboard shortcuts panel opens, focuses and returns focus when closed appropriately now regardless of how its triggered.
EDIT: also tested in Microsoft Edge along with NVDA.