react-aria-menubutton icon indicating copy to clipboard operation
react-aria-menubutton copied to clipboard

Prevent behavior of a Button element when the space key is pressed

Open grgr-dkrk opened this issue 5 years ago • 3 comments

related: #107

The space key in the Button element has a behavior similar to the keyUp event. This will fire an unexpected click event in some browsers. This PR prevents keyUp events on the Button component and avoids malfunctions in browsers such as Firefox.

example: https://codesandbox.io/s/spacekey-sample-9odi3

grgr-dkrk avatar Oct 19 '19 04:10 grgr-dkrk

Apologies for just barging in here, particularly if I'm misreading the situation, but, from your example and description, it sounds like Firefox is behaving correctly - native HTML buttons fire the click event on spacebar and enter. If you're adding role="button" that's not the case. You may want to consider removing the keydown events for enter and spacebar if actually using a button.

malcalevak avatar Oct 27 '19 03:10 malcalevak

@malcalevak Thank you. You are right. The behavior on Firefox is correct.

In Firefox, when the menu is closed and the focus moves to the Button component and then space key is released, if it is a native HTML button element, the Button component's onClick event is fired. It is causing problems like #107. The behavior of the space key in Firefox is executed at the timing like keyUp, and it is regarded as click. If the focused component is not an HTML native button element (for example,role="button"), there is no problem.

As you said, if the Button component is a button element, removing thehandleKeyDown is a natural solution. However, handleKeyDown also has the role of focusing the menu with the openMenu method of ambManager, and removing it causes another problem.

As @davidtheclark mentions, I'm thinking of limit the scope of keyUp's preventDefault to the space key.

grgr-dkrk avatar Oct 27 '19 09:10 grgr-dkrk

@grgr-dkrk I always bristle at negating native functionality, but, after reviewing the code, the only other option I could think of would be changing how focus was triggered, which would still rely on a key event, and it would require more rework, while possibly still being imperfect. I think you're correct that @davidtheclark's suggestion is probably the best (be sure to include the enter key, as he suggests!). You could go a step further, and limit the restriction to native buttons, but that's probably unnecessary overkill. This discussion has prompted me to question how other browsers handle the native button click; do they all use keyUp?

malcalevak avatar Oct 27 '19 17:10 malcalevak