react-native
react-native copied to clipboard
facebook
feat(iOS): Implement cursor style prop
Summary:
Implement the cursor style prop for iOS (and consequently, visionOS), as described in this RFC: https://github.com/react-native-community/discussions-and-proposals/pull/750
See related PR in React Native macOS, where we target macOS and visionOS (not running in iPad compatibility mode) with the same change: https://github.com/microsoft/react-native-macos/pull/2080
Changelog:
[IOS] [ADDED] - Implement cursor style prop
Test Plan:
I added a new RNTester API example for the Cursor Prop:
https://github.com/facebook/react-native/assets/6722175/3d4c0534-3b02-4258-b9ff-8c784505eed1
See the same example running on visionOS (Using React Native macOS, our fork that we added native visionOS support to):
https://github.com/facebook/react-native/assets/6722175/2f32f28f-cdfc-43f4-83e5-9de457147595
Notes
-
React Native macOS added the cursor prop to View with https://github.com/microsoft/react-native-macos/pull/760 and Text with https://github.com/microsoft/react-native-macos/pull/1469 . Much of the implementation comes from there.
-
Due to an Apple bug, as of iOS 17.4 Beta 4, the shape of the iOS cursor hover effect doesn't render in the correct bounds (but it does on visionOS). I've worked around it with an ifdef. The result is that the hover effect will work on iOS and visionOS, but not iPad apps running in compatibility mode on visionOS.
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 17,889,754 | -119,504 |
| android | hermes | armeabi-v7a | n/a | -- |
| android | hermes | x86 | n/a | -- |
| android | hermes | x86_64 | n/a | -- |
| android | jsc | arm64-v8a | 21,244,133 | -123,446 |
| android | jsc | armeabi-v7a | n/a | -- |
| android | jsc | x86 | n/a | -- |
| android | jsc | x86_64 | n/a | -- |
Base commit: 2053364e5a3bdec07d534a3f2bb5b7de21f42ce9 Branch: main
The "Test with Ruby Version X" jobs seem to be using Xcode 14.3.1, which this PR will definitely fail because it's an API only on Xcode 15+...
Fabric and JS parts LGTM apart from nits mentioned above. Not familiar enough with the AppKit/UIKit parts to leave a comment there.
In the demos you show in your PR description are those running w/ Fabric or Paper?
In the demos you show in your PR description are those running w/ Fabric or Paper?
That was in paper, but the demo works the same in Fabric. I figured attaching 6 videos ( [paper,fabric]x[ios, macOS, visionos]) wasn't the best, but I can do that!
@vincentriemer here's a video in Fabric on iOS off of this branch, since I was just making changes.
https://github.com/facebook/react-native/assets/6722175/d4696f35-4e53-46e4-803d-2b8d2fd0a8fc
EDIT: Handled. Finding out where view flattening was handled was pretty easy. Yay!
Slight update (discovered while testing on macOS Fabric): It seems adding the cursor doesn't prevent the view from getting flattened. Therefore, if you have a View that just has a cursor and padding wrapping another view (say, some Text), the view is flattened away, and there is no cursor :D. I'll need to update to handle view flattening & add an example of nested views with a cursor to my Cursor Example page. Will update shortly!
@vincentriemer has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@vincentriemer merged this pull request in facebook/react-native@73664f576aaa472d5c8fb2a02e0ddd017bbb2ea4.
Hey @vincentriemer, should the RFC be now also merged? (https://github.com/react-native-community/discussions-and-proposals/pull/750)
@okwasniewski @vincentriemer @Saadnajmi @NickGerleman This diff uses some APIs that are only available in iOS 17. For example: https://developer.apple.com/documentation/uikit/uihoverstyle?changes=latest_minor&language=objc
We need to keep compatibility with iOS 13.4. CI was red for the jobs that are still using Xcode 14.3.1.
Hey @cipolleschi, I've created a PR for this: https://github.com/facebook/react-native/pull/43331
Hey @vincentriemer, should the RFC be now also merged? (react-native-community/discussions-and-proposals#750)
I put in an approval but I'm not one of the people with merging capabilities 😬
@cipolleschi @vincentriemer Sorry! I had thought that main maybe 0.74 didn't need Xcode 17.4 compatibility anymore. I guess not :(
Last I heard, we were wanting to bump min to 15, because we can't locally test older XCode versions on modern macOS anymore. @cipolleschi we should do this alongside the change to move to GitHub Actions (we were already hitting issues bc they were trying to build using 14.2).
Last I heard, we were wanting to bump min to 15, because we can't locally test older XCode versions on modern macOS anymore. @cipolleschi we should do this alongside the change to move to GitHub Actions (we were already hitting issues bc they were trying to build using 14.2).
Yes, this is my issue as well. Unless I had an older install of Xcode around, it's usually difficult to actually test on older Xcode releases. Luckily I tend to have 2-3 versions around, and I'll keep this in mind.
That's true, but until we make the change, let's try to keep ci green. Otherwise, once CI is red, things lands, breakages pile up and we might end up in a situation where getting CI back to green is very expensive. We don't have to wait too much before moving everything to 15, the Apple deadline is the 1st of May