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

feat(iOS): Implement cursor style prop #43078

Closed
wants to merge 1 commit into from

Conversation

Saadnajmi
Copy link
Contributor

@Saadnajmi Saadnajmi commented Feb 18, 2024

Summary:

Implement the cursor style prop for iOS (and consequently, visionOS), as described in this RFC: react-native-community/discussions-and-proposals#750

See related PR in React Native macOS, where we target macOS and visionOS (not running in iPad compatibility mode) with the same change: microsoft#2080

Docs update: facebook/react-native-website#4033

Changelog:

[IOS] [ADDED] - Implement cursor style prop

Test Plan:

See the added example page, running on iOS with the new architecture enabled. This also runs the same on the old architecture.

Screen.Recording.2024-02-29.at.11.33.21.PM.mov

See the example page running on all three apple platforms. The JS is slightly different because:

  1. The "macOS Cursors" example is not part of this PR but the one in React Native macOS.
  2. This PR (and exapmple) has went though a bunch of iterations and It got hard taking videos of every change 😅
Screen.Recording.2024-02-26.at.11.33.52.PM.mov

Notes

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner labels Feb 18, 2024
@analysis-bot
Copy link

analysis-bot commented Feb 18, 2024

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: 2053364
Branch: main

@Saadnajmi Saadnajmi force-pushed the cursor-rn branch 3 times, most recently from 941db0d to f550094 Compare February 20, 2024 07:55
@Saadnajmi Saadnajmi marked this pull request as ready for review February 20, 2024 08:20
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 20, 2024
@Saadnajmi
Copy link
Contributor Author

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+...

@NickGerleman
Copy link
Contributor

Fabric and JS parts LGTM apart from nits mentioned above. Not familiar enough with the AppKit/UIKit parts to leave a comment there.

@vincentriemer
Copy link
Contributor

In the demos you show in your PR description are those running w/ Fabric or Paper?

@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Feb 29, 2024

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.

Screen.Recording.2024-02-29.at.2.25.49.PM.mov

@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Mar 1, 2024

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!

@Saadnajmi Saadnajmi force-pushed the cursor-rn branch 2 times, most recently from 90bdbcf to 4378943 Compare March 1, 2024 07:28
@facebook-github-bot
Copy link
Contributor

@vincentriemer has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 5, 2024
@facebook-github-bot
Copy link
Contributor

@vincentriemer merged this pull request in 73664f5.

@okwasniewski
Copy link
Contributor

Hey @vincentriemer, should the RFC be now also merged? (react-native-community/discussions-and-proposals#750)

@cipolleschi
Copy link
Contributor

@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.

@okwasniewski
Copy link
Contributor

Hey @cipolleschi, I've created a PR for this: #43331

@cipolleschi
Copy link
Contributor

ah... me too: #43330

@vincentriemer
Copy link
Contributor

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 😬

@Saadnajmi
Copy link
Contributor Author

@cipolleschi @vincentriemer Sorry! I had thought that main maybe 0.74 didn't need Xcode 17.4 compatibility anymore. I guess not :(

@Saadnajmi Saadnajmi deleted the cursor-rn branch March 5, 2024 21:19
@NickGerleman
Copy link
Contributor

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).

@Saadnajmi
Copy link
Contributor Author

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.

@cipolleschi
Copy link
Contributor

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

huntie pushed a commit that referenced this pull request Mar 11, 2024
Summary:
Implement the cursor style prop for iOS (and consequently, visionOS), as described in this RFC: react-native-community/discussions-and-proposals#750

See related PR in React Native macOS, where we target macOS and visionOS (not running in iPad compatibility mode) with the same change: microsoft#2080

Docs update: facebook/react-native-website#4033

## Changelog:

[IOS] [ADDED] - Implement cursor style prop

Pull Request resolved: #43078

Test Plan:
See the added example page, running on iOS with the new architecture enabled. This also runs the same on the old architecture.

https://github.com/facebook/react-native/assets/6722175/2af60a0c-1c1f-45c4-8d66-a20f6d5815df

See the example page running on all three apple platforms. The JS is slightly different because:
1. The "macOS Cursors" example is not part of this PR but the one in React Native macOS.
2. This PR (and exapmple) has went though a bunch of iterations and It got hard taking videos of every change 😅

https://github.com/facebook/react-native/assets/6722175/7775ba7c-8624-4873-a735-7665b94b7233

## Notes

- React Native macOS added the cursor prop to View with microsoft#760 and Text with microsoft#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.

Reviewed By: NickGerleman

Differential Revision: D54512945

Pulled By: vincentriemer

fbshipit-source-id: 699e3a01a901f55a466a2c1a19f667aede5aab80
This was referenced Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants