Fix onPress when used with onKeyUp on a button #5554
Conversation
|
Build successful! 🎉 |
| if (shouldStopPropagation) { | ||
| e.stopPropagation(); | ||
| state.target?.dispatchEvent(new KeyboardEvent('keyup', {...e, bubbles: false})); |
There was a problem hiding this comment.
@LFDanLu when do you anticipate this being hit? I'm not seeing it fail any of our current tests if I revert this line
There was a problem hiding this comment.
hm, could've swore that without it I wasn't getting the onKeyDown attached to a button firing but it is working locally as well.
There was a problem hiding this comment.
ah I remember now, I'm not sure why I removed the e.stopPropagation actually but IMO we should be stopping propagation of the keyup since usePress has handled it. The dispatch was there in order to try and get the keyUp on the button to still fire but it doesn't seem to work...
There was a problem hiding this comment.
well, it should only stop it for the Enter and Space keys, everything else should go through
we don't allow through all the pointerDown/Up etc when we handle it, so I don't think there is a problem if we block the keyUp for those two keys?
The test I wrote shows that keyUp coming through, I actually thought that was a bit weird.
There was a problem hiding this comment.
At the moment the wrapping if statement that has isValidKeyboardEvent should already limit this block to only Enter/Space so readding the stopPropagation back above should be fine. However, adding it back in makes it so that keyUp doesn't fire on a Button when pressing Enter/Space even with the dispatchEvent, IMO if possible we should have keyUp on the same element handling the press still fire for Enter/Space but then stop at that level.
There was a problem hiding this comment.
I tried this, unfortunately we fail some others as a result.
I checked against pointerUp, and that does make it out, so I think your argument that keyUp should make it at least as far as pointerUp has merit.
There was a problem hiding this comment.
Just so we don't forget: after talking about this with the rest of the team, I think we can get rid of the dispatchEvent and stop propagation here. usePress has historically not stopped propagation of the keydown event technically since it happened at the window level on a non-capturing listener. useKeyboard should stop propagation where we need it to happen, all other cases should have onKeyUp continue bubbling
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
see #5554 (comment), essentialy it is ok that we dont stop propagation since we never have for keyup since it used to be a non capturing listener attached to the window
…epress-event-order
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
## API Changes
unknown top level export { type: 'any' } |
Closes #4350
Fixes issue where passing onKeyUp prevented onPress from being called.
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: