search

microsoft / react-native-macos

A framework for building native macOS apps with React.
https://microsoft.github.io/react-native-windows/
MIT License
3.37k stars 129 forks source link

New events for RCTUIView #2136

Open amgleitman opened 3 weeks ago

amgleitman commented 3 weeks ago

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.

Saadnajmi commented 3 weeks ago

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)

Saadnajmi commented 3 weeks ago

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 commented 3 weeks ago

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.