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

[grgr-dkrk]

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

[vutran]

Can we add a brief comment here noting why we're calling event.preventDefault? This makes it easily apparent when viewing the code in the future.

[davidtheclark]

This seems like a dangerously broad move, preventing default on all key-up events regardless of the key — doesn't it? Should we limit it to only target Enter and Space?

[vutran]

To add to David's comment, I think we only need to limit it to Space since Enter works.

[vutran]

I think we can improve upon this assertion here. Rather than asserting event.preventDefault is called (that doesn't really provide any useful information whether the menu is being opened/toggled), we should first open the menu, and simulate the keyDown, and keyUp with the spaceEvent you have and check if toggleMenu is called multiple times like so.

[grgr-dkrk]

Thanks.
Could you tell me if you have a good idea about this?
In the test case, I think it is difficult to reproduce the behavior of Firefox simulating a click event when you release the space bar on a button element.
Calling toggleMenu is the role of keyDown or click, and keyUp itself is not involved in calling it, hence it asserts whether keyUp calls event.preventDefault.

[vutran]

Yeah, I'm not sure how to run these tests against a FF environment (I think this can be tackled as a separate issue). However, the tests is just to ensure it doesn't cause any regression in general.

I think something like the steps below would work:

1. simulate click to open menu first
2. expect ambManager.isOpen is true (ensures the menu is open before we run assertions to validate the Space events
3. simulate keyDown and then a keyUp Space event
4. expect ambManager.isOpen is false

[grgr-dkrk]

Thank you very much.
I will work on it.

[malcalevak]

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.

[grgr-dkrk]

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

[malcalevak]

@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?