-
Notifications
You must be signed in to change notification settings - Fork 46.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds experimental event component responder surfaces #15228
Conversation
packages/react-events/src/Press.js
Outdated
let keyPressEventListener = props.onPress; | ||
|
||
// Wrap listener with prevent default behaviour, unless | ||
// we are dealing with an anchor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are anchors excluded?
packages/react-events/src/Press.js
Outdated
case 'touchstart': | ||
// Touch events are for Safari, which lack pointer event support. | ||
if (!state.isPressed && !context.isTargetOwned(eventTarget)) { | ||
// We bail out of polyfilling anchor tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also do with explanation
packages/react-events/src/Press.js
Outdated
state.isPressed = false; | ||
state.isLongPressed = false; | ||
// Prevent mouse events from firing | ||
(event: any).preventDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this can also prevent subsequent non-mouse events from firing, like focus
. I didn't rely on preventDefault
in RNW for this reason and relied on ignoring mouse events that occur within a period after a touch event (which also doesn't require relying on non-passive touch events) - https://github.com/necolas/react-native-web/blob/0.11.2/packages/react-native-web/src/modules/ResponderEventPlugin/index.js#L65-L71.
packages/react-events/src/Press.js
Outdated
return; | ||
} | ||
// Ignore right-clicks | ||
if (event.button === 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding event.button === 1
will also ignore middle-clicks, e.g., on a scroll wheel
const {eventTarget, eventType, event} = context; | ||
|
||
switch (eventType) { | ||
case 'keydown': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider making keyboards trigger pressin
, pressout
, press
, and even longpress
events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do this at a later point, in a follow up.
ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.3% Details of bundled changes.Comparing: 80f8b0d...cf5b523 react-dom
react-events
Generated by 🚫 dangerJS |
I checked manually and this PR does not add any size compared to master. The results.json in master needs updating. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not land without review. If you want to move fast, we should create a new build that we can put behind a flag since this currently goes to real prod it can break FB.
We don't compare against the results.json file but a rebuilt version. I tested locally and the (small) build increase shown by sizebot above seems legit. |
@sebmarkbage I saw it was approved earlier, we also spoke about it earlier too. None of these code paths should get used on FB, but I’ll be sure to sanity check this. The size difference doesn’t make sense. If I remove the changes in this PR and build, I still get 0.3% increase locally. |
I checked out this commit built and then checked out the previous commit and built that. Then compared the two bundles. I never compare against the checked in JSON. That one never matches up. |
Note: this is for an experimental event API that we're testing out internally at Facebook.
This PR adds Press and Hover responder surfaces, some additional event context APIs and fixes some bugs surfaced from the additional Press tests added.
#15036
#15112
#15150
#15168
#15179
This PR also adds tests that validate the triggering of event responder handleEvent function.