Storybook: onKeyUp event doesn't work after upgrade to Storybook 5.0.1

[aralroca]

Describe the bug
I have a React component that use the onKeyPress event and before the Storybook version 5.0.1 was working fine. However, after migrate to the last version, it stop working. Doing nothing.

To Reproduce

I have this React hook:

import { useEffect } from 'react'
import includes from 'lodash/includes'

export default function useOnKeyUp({ action, keyCodes = [], when = true }) {
  useEffect(() => {
    function onKeyUp(e) {
      if (includes(keyCodes, e.keyCode) && when) {
        action()
      }
    }

    document.body.addEventListener('keyup', onKeyUp)

    return () => document.body.removeEventListener('keyup', onKeyUp)
  }, [when, action, keyCodes])
}

And I'm using a component that use this hook to close a modal:

// Close on press keyCodes (ex: ESCAPE or ENTER)
  useOnBodyKeyUp({
    action: closeModal,
    keyCodes: keyCodesToClose,
    when: isOpen,
  })

Expected behavior
To fire the event.

[CodeByAlex]

@aralroca are you using any addons and if so, which ones?

[aralroca]

Yes! I'm using this ones:

adons.js

import 'storybook-addon-jsx/register'
import '@storybook/addon-knobs/register'

package.json

"@storybook/addon-knobs": "5.0.1",
"@storybook/addon-info": "5.0.1",
"storybook-addon-jsx": "6.0.0"

[shilman]

@aralroca What version of Storybook are you upgrading from?

[aralroca]

@shilman I was using 4.1 before 5.0.1

[shilman]

@tmeasday @Jessica-Koch Could this be due to the updated keyboard handling in SB5?

[tmeasday]

This is all the keybinding we do inside the preview, AFAIK:

https://github.com/storybooks/storybook/blob/cf96e889c18095c225325d034501e40ced0472d3/lib/core/src/client/preview/start.js#L197-L205

I'm not sure how that could stop the keyup event firing?

[Jessica-Koch]

Tom is right

Sent from my iPhone

On Mar 12, 2019, at 3:34 AM, Tom Coleman [e-mail] wrote:

This is all the keybinding we do inside the preview, AFAIK:

https://github.com/storybooks/storybook/blob/cf96e889c18095c225325d034501e40ced0472d3/lib/core/src/client/preview/start.js#L197-L205

I'm not sure how that could stop the keyup event firing?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[stale[bot]]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[mouhannad-sh]

This is all the keybinding we do inside the preview, AFAIK:

storybook/lib/core/src/client/preview/start.js

Lines 197 to 205 in cf96e88

window.onkeydown = event => {
if (!focusInInput(event)) {
// We have to pick off the keys of the event that we need on the other side
const { altKey, ctrlKey, metaKey, shiftKey, key, code, keyCode } = event;
channel.emit(Events.PREVIEW_KEYDOWN, {
event: { altKey, ctrlKey, metaKey, shiftKey, key, code, keyCode },
});
}
};
I'm not sure how that could stop the keyup event firing?

I'm not familiar with the storybook codebase but from some investigation and looking at this, it's likely that you have a global listener on the manager view that listens to the key events in which you've emitted here.

I think whatever is listening to this event in the manager view is changing the focus from the preview to the parent. That would explain the keyup event never firing inside of the preview if the keydown "key" matches one of your targets.

running the following in the browser console will disable the key listeners in the preview but it will allow the browser to fire keyup events as usual.

[tmeasday]

Hmm, thanks for investigating @mouhannad-sh and I think you are likely right. What is the specific key you are pressing that causes the issue?

I'm not quite sure what we can do to avoid this problem. We currently check to see if the keydown event happens inside an input or textarea, but of course if you have a custom element that is handling key events this will not help. What is likely happening is you are triggering a shortcut such as the / key which focuses the search box.

We could be very conservative and only pass events through if they happen outside the rendered story but this would have the confusing effect of making shortcuts not work in many cases 🤔

[mouhannad-sh]

Thanks for replying so quickly @tmeasday
In my story I have Modal component that should close on "Escape" keyup.

I think the storybook script has more priority to the preview's context.
That's been said your event listeners are likely to execute their logic before anythings else inside of the preview view.

I would suggest one of 3 solutions that comes to my head:

• To have a more complex key bindings to solve this issue.
• Provide a config to modify/disable specific hotkeys per story.
• Do not change the focus if it's inside of the preview

[tmeasday]

To have a more complex key bindings to solve this issue.

Well we have customizable keybindings so I'm not sure we can solve it this way. I guess the escape keybinding isn't customizable though so I'm not sure you can even work around it by changing that one :/

Provide a config to modify/disable specific hotkeys per story.

This approach is definitely doable, although a bit heavy for the user.

Do not change the focus if it's inside of the preview

This seems interesting although maybe hard to guarantee. I wonder why pressing the esc key is changing the focus in the first place?

I dug into the code. The reason is this:

https://github.com/storybooks/storybook/blob/next/lib/api/src/modules/shortcuts.ts#L130

I wonder why it does that? It was originally added in this commit by @ndelangen - do you remember what it is about Norbert?

I suspect it is something to do with the treeview. If so, I think maybe it should be removed from the general keybinding and added to the keybinding for the treeview only.

This wouldn't entirely solve the problem in all cases (e.g. the / key) but given esc is a really common key for people to use as a shortcut in their own components it would probably go a long way towards it.

[ndelangen]

I'm fine with removing that line actually. It's purpose is to remove focus on something in the treeview.

Would you open a PR removing that line and verify that it fixes the issue @aralroca?

[benbowes]

I am using storybook for a wcag accessible component, and the keyboard shortcuts totally kill my component's ability to function properly with it's key bindings.

Is it possible to disable all of the storybook key bindings? That is what I would do if I could.

[benbowes]

Here is an example https://benbowes.github.io/react-responsive-select-next/?path=/story/info--home

[shilman]

@benbowes I think there's an option that's not working and should be fixed: https://github.com/storybooks/storybook/issues/6569#issuecomment-485239507

[benbowes]

Great @shilman, thanks a mill

[shilman]

Yowza!! I just released https://github.com/storybooks/storybook/releases/tag/v5.1.0-alpha.35 containing PR #6578 that references this issue. Upgrade today to try it out!

Because it's a pre-release you can find it on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

[benbowes]

Works a treat on 5.1.0-alpha.35 🎉