Skip to content
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

New events for RCTUIView #2136

Closed

Conversation

amgleitman
Copy link
Member

@amgleitman amgleitman commented Jun 13, 2024

Summary:

Moves mouse event implementations from RCTView to superclass RCTUIView so other React Native views can access them.

Test Plan:

Did a quick pass through RNTester, nothing seems broken. This should only affect macOS since everything is within TARGET_OS_OSX blocks or macOS-only files.

@amgleitman amgleitman marked this pull request as ready for review June 14, 2024 22:21
@amgleitman amgleitman requested a review from a team as a code owner June 14, 2024 22:21
@Saadnajmi
Copy link
Collaborator

Saadnajmi commented Jun 18, 2024

EDIT: I think no circular dependencies because both RCTUIKit and RCTComponent are in the React-Core module

I'm thinking about this more, and feeling like maybe the actual event creation/handling for stuff like onMouseEnter should move to a separate file, similar to RCTViewKeyboardEvent is handled. RCTUIView currently is only a macOS/iOS shim, and this PR changes its definition to handle some React specific stuff. That might be ok, but I haven't finished thinking through the ramifications of that (does the pod it's in take a new dip, causing circular dependencies? etc)

Comment on lines +451 to +456
@property (nonatomic, copy) RCTBubblingEventBlock onResponderGrant;
@property (nonatomic, copy) RCTBubblingEventBlock onResponderMove;
@property (nonatomic, copy) RCTBubblingEventBlock onResponderRelease;
@property (nonatomic, copy) RCTBubblingEventBlock onResponderTerminate;
@property (nonatomic, copy) RCTBubblingEventBlock onResponderTerminationRequest;
@property (nonatomic, copy) RCTBubblingEventBlock onStartShouldSetResponder;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can actually remove these, I think these are actually mostly handled in JS, if nothing is calling this in native code across iOS/macOS

@Saadnajmi
Copy link
Collaborator

OK, I spoke with @acoates-ms on how this is handled in RNW. He told me it's done at the framework level through CompositionEventHandler.cpp, instead of being implemented on specific components. This kinda matches how both RNM and RNW handle keyboard events (RNM has RCTViewKeyboardEvent.h/.m and RNW has KeyboardEventHandler.cpp).

I think what makes sense is to move mouse hovering Logic to like a RCTMouseEventHandler (though we need to be clear it handles hover, not clicks, which are still through RCTTouchHandler), and have views each call that similar to what we do for keyboard events. That does mean adding logic to handle calling the new methods to Text/VirtualText, so maybe there's a middle ground where that is still handled by RCTUIView. I think that is what the UIView+React category is meant for, but categories aren't great and we should perhaps not extend. Fabric doesn't use that category AFAIK.

@amgleitman
Copy link
Member Author

OK, I spoke with @acoates-ms on how this is handled in RNW. He told me it's done at the framework level through CompositionEventHandler.cpp, instead of being implemented on specific components. This kinda matches how both RNM and RNW handle keyboard events (RNM has RCTViewKeyboardEvent.h/.m and RNW has KeyboardEventHandler.cpp).

I think what makes sense is to move mouse hovering Logic to like a RCTMouseEventHandler (though we need to be clear it handles hover, not clicks, which are still through RCTTouchHandler), and have views each call that similar to what we do for keyboard events. That does mean adding logic to handle calling the new methods to Text/VirtualText, so maybe there's a middle ground where that is still handled by RCTUIView. I think that is what the UIView+React category is meant for, but categories aren't great and we should perhaps not extend. Fabric doesn't use that category AFAIK.

I agree that having something like RCTMouseEventHandler does seem like a much nicer solution in the long term. I don't know how much extra work it'll be to refactor everything, but I don't want perfect to be the enemy of good, so I think that's an argument to go with this for now and create an issue to tackle this later.

@Saadnajmi
Copy link
Collaborator

@amgleitman we still want this for 0.74, correct?

@amgleitman
Copy link
Member Author

This has since been covered by #2149.

@amgleitman amgleitman closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants