[Fix] for Issue #557 moving focus to the keyboard shortcuts panel when opened #825

erin-doyle · 2017-11-04T18:21:51Z

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.

coveralls · 2017-11-04T18:27:47Z

Coverage increased (+0.4%) to 87.148% when pulling 33b133f on fix_issue557_keyboard_shortcuts_panel into ae49b71 on master.

erin-doyle · 2017-11-04T18:31:26Z

src/components/DayPicker.jsx

    const { focusedDate } = this.state;

-    if (
-      (!prevProps.isFocused && isFocused && !focusedDate) ||
-      (!prevProps.showKeyboardShortcuts && showKeyboardShortcuts)


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.

@majapw, any idea why this condition was here in the first place?

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.

erin-doyle · 2017-11-04T18:34:51Z

src/components/KeyboardShortcutRow.jsx

@@ -38,7 +38,7 @@ function KeyboardShortcutRow({
        <span
          {...css(styles.KeyboardShortcutRow_key)}
          role="img"
-          aria-label={label}
+          aria-label={`${label} `}


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.

would adding : or similar instead of a space have the same effect?

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

erin-doyle · 2017-11-04T18:36:19Z

src/components/SingleDatePicker.jsx

@@ -404,6 +404,7 @@ class SingleDatePicker extends React.Component {
          renderCalendarInfo={renderCalendarInfo}
          isFocused={isDayPickerFocused}
          showKeyboardShortcuts={showKeyboardShortcuts}
+          onBlur={this.onDayPickerBlur}


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.

ljharb

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?

ljharb · 2017-11-04T18:36:37Z

src/components/DayPickerKeyboardShortcuts.jsx

+    unicode: '↵',
+    label: phrases.enterKey,
+    action: phrases.selectFocusedDate,
+  },


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.

ljharb · 2017-11-04T18:37:15Z

src/components/DayPickerKeyboardShortcuts.jsx

 class DayPickerKeyboardShortcuts extends React.Component {
  constructor(...args) {
    super(...args);

-    this.onClick = this.onClick.bind(this);
+    this.keyboardShortcuts = getKeyboardShortcuts(this.props.phrases);


this would also need to happen in componentWillReceiveProps.

Moving this was not performance motivated, I just wanted to make render() a little more concise. Sounds like I should just move it back.

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.

Ok I'll make that change in a separate commit then 👍

coveralls · 2017-11-05T17:18:09Z

Coverage increased (+0.4%) to 87.165% when pulling 8396f88 on fix_issue557_keyboard_shortcuts_panel into ae49b71 on master.

ljharb

Thanks, having it in two commits made it much easier to review!

ljharb · 2017-11-05T17:59:25Z

src/components/DayPicker.jsx

    const { focusedDate } = this.state;

-    if (
-      (!prevProps.isFocused && isFocused && !focusedDate) ||
-      (!prevProps.showKeyboardShortcuts && showKeyboardShortcuts)


@majapw, any idea why this condition was here in the first place?

ljharb · 2017-11-05T17:59:53Z

src/components/DayPickerKeyboardShortcuts.jsx

    this.setShowKeyboardShortcutsButtonRef = this.setShowKeyboardShortcutsButtonRef.bind(this);
+    this.setHideKeyboardShortcutsButtonRef = this.setHideKeyboardShortcutsButtonRef.bind(this);
+    this.handleFocus = this.handleFocus.bind(this);


this.handleFocus doesn't seem to be passed anywhere as a prop - can this binding be removed?

ljharb · 2017-11-05T18:00:15Z

src/components/DayPickerKeyboardShortcuts.jsx

+            if (e.key === 'Enter') {
+              e.preventDefault();
+            } else if (e.key === 'Space') {
+              this.onShowKeyboardShortcutsButtonClick();


this should probably pass the e through?

The function doesn't use the event object at all, you still prefer I pass it on though?

i think it'd be more future-proof if it ever needed it in the future. Not a huge deal obv.

ljharb · 2017-11-05T18:02:19Z

src/components/DayPickerKeyboardShortcuts.jsx

@@ -110,6 +157,7 @@ class DayPickerKeyboardShortcuts extends React.Component {
    return (
      <div>
        <button
+          id="showKeyboardShortcutsButton"


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.

Ok I'll remove and update the test(s)

ljharb · 2017-11-05T18:03:17Z

src/components/DayPickerKeyboardShortcuts.jsx

+  onKeyDown(e) {
+    const { closeKeyboardShortcutsPanel } = this.props;
+
+    e.stopPropagation();


why does propagation need to be stopped here?

It's being done that way in DayPicker's onKeyDown() so figured it would probably be needed here as well.

ljharb · 2017-11-05T18:03:58Z

src/components/DayPickerKeyboardShortcuts.jsx

@@ -154,26 +210,25 @@ class DayPickerKeyboardShortcuts extends React.Component {
            </div>

            <button
+              id="hideKeyboardShortcutsButton"


also this one

ljharb · 2017-11-05T18:05:10Z

src/components/DayPickerKeyboardShortcuts.jsx

-            <ul {...css(styles.DayPickerKeyboardShortcuts__list)}>
+            <ul
+              {...css(styles.DayPickerKeyboardShortcuts__list)}
+              id="DayPickerKeyboardShortcuts__description"


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?

ljharb · 2017-11-05T18:05:46Z

src/components/KeyboardShortcutRow.jsx

@@ -38,7 +38,7 @@ function KeyboardShortcutRow({
        <span
          {...css(styles.KeyboardShortcutRow_key)}
          role="img"
-          aria-label={label}
+          aria-label={`${label} `}


would adding : or similar instead of a space have the same effect?

coveralls · 2017-11-05T22:12:57Z

Coverage increased (+0.4%) to 87.165% when pulling b43aaf8 on fix_issue557_keyboard_shortcuts_panel into ae49b71 on master.

erin-doyle · 2017-11-07T13:03:33Z

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!

coveralls · 2017-11-07T23:49:56Z

Coverage increased (+0.4%) to 87.192% when pulling 023c50c on fix_issue557_keyboard_shortcuts_panel into 7eb2191 on master.

ljharb

LGTM

@majapw

however, deferring to @majapw

majapw

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.

majapw · 2017-11-09T06:10:08Z

src/components/DayPickerKeyboardShortcuts.jsx

+
+      case 'ArrowUp':
+      case 'ArrowDown':
+        e.stopPropagation();


Can you leave a comment in here to explain what's going on? I'm not sure it's self-evident

Yep no problem!

majapw · 2017-11-09T06:10:50Z

src/components/KeyboardShortcutRow.jsx

@@ -38,7 +38,7 @@ function KeyboardShortcutRow({
        <span
          {...css(styles.KeyboardShortcutRow_key)}
          role="img"
-          aria-label={label}
+          aria-label={`${label},`}


#825 (comment)

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.

…ttle less noisy

…ests

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

coveralls · 2017-11-09T12:36:38Z

Coverage increased (+0.4%) to 87.192% when pulling c742e76 on fix_issue557_keyboard_shortcuts_panel into 44fa104 on master.

erin-doyle · 2017-11-09T13:57:35Z

Looking in to build failures now, all I did was rebase on master to pick up any recent changes and then add comments...

erin-doyle · 2017-11-09T20:23:14Z

Linting error:

111:1 error Line 111 exceeds the maximum line length of 100 max-len

🤦‍♀️

…t handling for keyboard shortcuts

coveralls · 2017-11-09T21:12:35Z

Coverage increased (+0.4%) to 87.192% when pulling 121c2e0 on fix_issue557_keyboard_shortcuts_panel into 44fa104 on master.

coveralls · 2017-11-09T21:22:04Z

Coverage increased (+0.4%) to 87.192% when pulling b3d3de1 on fix_issue557_keyboard_shortcuts_panel into 44fa104 on master.

majapw

Wooooot! :)

erin-doyle · 2017-11-10T11:29:07Z

Yay! 🎉 😌

erin-doyle · 2017-11-10T11:38:10Z

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.

majapw · 2017-11-10T18:06:06Z

v15.1.0 is out!

erin-doyle mentioned this pull request Nov 4, 2017

Keyboard navigation issues #557

Open

erin-doyle commented Nov 4, 2017

View reviewed changes

ljharb reviewed Nov 4, 2017

View reviewed changes

erin-doyle force-pushed the fix_issue557_keyboard_shortcuts_panel branch from 33b133f to 8396f88 Compare November 5, 2017 17:07

ljharb reviewed Nov 5, 2017

View reviewed changes

erin-doyle force-pushed the fix_issue557_keyboard_shortcuts_panel branch from b43aaf8 to 023c50c Compare November 7, 2017 23:40

ljharb previously approved these changes Nov 8, 2017

View reviewed changes

ljharb requested a review from majapw November 8, 2017 01:33

majapw previously approved these changes Nov 9, 2017

View reviewed changes

Erin Doyle added 5 commits November 9, 2017 07:14

Moved the keyboardShortcuts array into a function to make render a li…

d8ea2fc

…ttle less noisy

Removed the button id's being used only for testing and updated the t…

15601f3

…ests

Minor cleanup: 1) add the passing of the event object when onShowKeyb…

c5bcf34

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

Missed blocking the Arrow keys when the keyboard shortcuts panel is o…

5c9f986

…pen. Only stopping the propagation on the ArrowUp and ArrowDown keys so that the default behavior of scrolling the dialog is not prevented.

erin-doyle dismissed majapw’s stale review via c742e76 November 9, 2017 12:27

erin-doyle force-pushed the fix_issue557_keyboard_shortcuts_panel branch from 023c50c to c742e76 Compare November 9, 2017 12:27

erin-doyle force-pushed the fix_issue557_keyboard_shortcuts_panel branch from c742e76 to 121c2e0 Compare November 9, 2017 20:47

Added comments to clarify some changes concerning aria-label and even…

b3d3de1

…t handling for keyboard shortcuts

erin-doyle force-pushed the fix_issue557_keyboard_shortcuts_panel branch from 121c2e0 to b3d3de1 Compare November 9, 2017 21:06

majapw approved these changes Nov 10, 2017

View reviewed changes

majapw merged commit 191cdaf into master Nov 10, 2017

majapw deleted the fix_issue557_keyboard_shortcuts_panel branch November 10, 2017 01:45

erin-doyle mentioned this pull request Jan 3, 2018

SingleDatePicker focus problems on touch devices #747

Open

[Fix] for Issue #557 moving focus to the keyboard shortcuts panel when opened #825

[Fix] for Issue #557 moving focus to the keyboard shortcuts panel when opened #825

Conversation

erin-doyle commented Nov 4, 2017 • edited Loading

coveralls commented Nov 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erin-doyle Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erin-doyle Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 5, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 5, 2017

erin-doyle commented Nov 7, 2017

coveralls commented Nov 7, 2017

ljharb left a comment

Choose a reason for hiding this comment

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 9, 2017

erin-doyle commented Nov 9, 2017

erin-doyle commented Nov 9, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017

majapw left a comment

Choose a reason for hiding this comment

erin-doyle commented Nov 10, 2017

erin-doyle commented Nov 10, 2017

majapw commented Nov 10, 2017

erin-doyle commented Nov 4, 2017 •

edited

Loading

erin-doyle Nov 4, 2017 •

edited

Loading

erin-doyle Nov 5, 2017 •

edited

Loading