Add cursor prop to View and Touchables #760

ryanlntn · 2021-04-20T15:18:53Z

Please select one of the following

I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
I am making a fix / change for the macOS implementation of react-native
I am making a change required for Microsoft usage of react-native

Summary

Adds support for managing cursor update events per Apple's Managing Cursor-Update Events documentation. Supports NSCursor name values and as many CSS name values as I could figure out given what's available on NSCursor.

Changelog

[macOS] [Added] - Add cursor prop to View and Touchables

Test Plan

Screen.Recording.2021-04-20.at.9.09.18.AM.mov

appden · 2021-04-22T17:47:09Z

React/Views/RCTViewManager.m

+      @"operationNotAllowed" : @(RCTCursorOperationNotAllowed),
+      @"dragLink" : @(RCTCursorDragLink),
+      @"dragCopy" : @(RCTCursorDragCopy),
+      @"contextualMenu" : @(RCTCursorContextualMenu),


Can we stick to the official CSS snake-case names? https://developer.mozilla.org/en-US/docs/Web/CSS/cursor

Makes sense to me. I'm including "disappearing-item" as well which as far as I can tell has no equivalent in CSS.

appden · 2021-04-22T17:56:58Z

React/Views/RCTView.m

+    case RCTCursorAuto:
+      if (self.focusable) {
+        [self addCursorRect:self.bounds cursor:[NSCursor pointingHandCursor]];
+      }


I'm concerned about auto here since most buttons and other clickable items are focusable, but showing the hand over those is more appropriate on the web than for native applications.

I'm porting a web app so and using enough electron based apps that I didn't even notice this. You're right, most native applications tend not to use pointing hand on hover of buttons. I dropped this case.

appden · 2021-04-22T18:01:28Z

React/Views/RCTView.m

@@ -1380,6 +1380,70 @@ - (void)drawFocusRingMask
 #pragma mark - macOS Event Handler

 #if TARGET_OS_OSX
+- (void)resetCursorRects
+{


I think you might need to call [self discardCursorRects] here before adding new ones below.

Also, may I recommend having a NSCursor *cursor variable that can get set in the switch below, then afterwards add it to reduce some of the repetition here?

if (cursor) { [self addCursorRect:self.bounds cursor:cursor]; }

I add one line of code: [super resetCursorRects];
It will stable display for cursor.

appden · 2021-04-22T18:02:03Z

React/Views/RCTViewManager.m

+      @"dragCopy" : @(RCTCursorDragCopy),
+      @"contextualMenu" : @(RCTCursorContextualMenu),
+    }),
+    RCTCursorAuto,


Can we default to "default" (RCTCursorArrow) here instead?

NickGerleman · 2021-04-22T23:42:46Z

I think some of the folks on Win32 Office came up with a similar prop. Let me dig it up for reference, in case it makes sense to align.

NickGerleman · 2021-04-22T23:44:45Z

Here was the windows one. Same cursor prop on view, exposing some different options.

https://github.com/microsoft/react-native-windows/blob/master/packages/%40office-iss/react-native-win32/src/Libraries/Components/View/ViewWin32.Props.ts

export type Cursor =
  | 'auto'
  | 'default'
  | 'pointer'
  | 'help'
  | 'not-allowed'
  | 'wait'
  | 'move'
  | 'nesw-resize'
  | 'ns-resize'
  | 'nwse-resize'
  | 'we-resize'
  | 'text'

NickGerleman · 2021-04-22T23:45:55Z

So I think everything is aligning to CSS props, but just exposing different ones for now.

Libraries/Components/Touchable/TouchableBounce.js

lyahdav · 2021-04-29T22:14:12Z

Thank you so much @ryanlntn for this PR, very timely as I was going to have to do this myself soon.

One thing I think should be changed: right now if you try to nest a View inside another View that has the cursor prop set, the cursor sometimes doesn't get updated correctly. To resolve you can:

Update the RCT_ENUM_CONVERTER so that the default is RCTCursorAuto.
Update switch in resetCursorRects to include:

    case RCTCursorArrow:
      cursor = [NSCursor arrowCursor];
      break;

Remove the default from that switch.
Move the call to discardCursorRects outside the if (cursor) check.

ryanlntn · 2021-04-30T22:08:13Z

Thanks @lyahdav! Updated per the latest feedback.

lyahdav · 2021-05-03T23:41:33Z

I have a couple more comments:

@necolas gave feedback that this should be added to the styles and not added as a prop, and match the W3C spec as much as possible.
We are repeating the list of valid cursor values in JS in TouchableWithoutFeedback.js and ViewPropTypes.js. Can we make this a Flow type that we reference everywhere it's needed instead?

lyahdav · 2021-05-04T00:41:28Z

Here's a git patch of this PR making it a style prop:
0001-Add-cursor-prop-to-View.txt

NickGerleman · 2021-05-04T06:17:51Z

@lyahdav @appden it looks you're on top of this change. Lmk when it's ready for merge and I can bring it in.

appden · 2021-05-06T19:41:19Z

React/Views/RCTViewManager.m

@@ -24,6 +24,39 @@
 #import "RCTTVView.h"
 #endif

+#if TARGET_OS_OSX
+@implementation RCTConvert (UIView)


Suggested change

@implementation RCTConvert (UIView)

@implementation RCTConvert (RCTCursor)

appden · 2021-05-06T19:49:59Z

React/Views/RCTView.h

@@ -16,6 +16,30 @@
 extern const UIAccessibilityTraits SwitchAccessibilityTrait;
 #endif // TODO(macOS ISS#2323203)

+#if TARGET_OS_OSX
+typedef NS_ENUM(NSInteger, RCTCursor) {


Can we put this into RCTCursor.h header along with the RCTConvert definition into a RCTCursor.m file, much like how RCTResizeMode is handled?

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Components/View/ReactNativeViewViewConfigMacOS.js

Libraries/Components/View/ViewPropTypes.js

Libraries/StyleSheet/StyleSheetTypes.js

React/Base/macOS/RCTUIKit.m

shwanton · 2022-08-09T22:09:33Z

@ryanlntn Thank you for pushing those changes!

Here is what the cursor looks like on View

Saadnajmi · 2022-08-09T23:33:22Z

This should be good to go once build issues are fixed

Saadnajmi · 2022-08-09T23:52:48Z

Actually wait, is this a View prop or View_Style_ prop? I think windows implemented this as a View prop. Could we try and be consistent?

https://github.com/microsoft/react-native-windows/blob/ea292e188b4252c11a80a56ef8e94f7cfe5a6909/packages/%40office-iss/react-native-win32/src/Libraries/Components/View/Tests/ViewWin32Test.tsx#L226

ryanlntn · 2022-08-10T02:43:02Z

@Saadnajmi It's both. I originally implemented as a view prop and extended to add as a view style prop as well per this feedback.

Saadnajmi · 2022-08-11T21:10:48Z

OK, I just did a lot of chatting offline and it's agreed we prefer this as a style prop. Could you remove the bits that implement cursor a view prop and not a style prop and fix the build errors so we can move this PR forward?

analysis-bot · 2022-08-12T02:05:44Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 7783f88
Branch: main

Summary: This brings in PR microsoft#760 to our repo. Test Plan: RNTester > View: {F612877442} Reviewers: skyle Reviewed By: skyle Subscribers: necolas, eliwhite Differential Revision: https://phabricator.intern.facebook.com/D28158243 Tasks: T78743077 Signature: 28158243:1620330169:72c6c9e2371f2ac1a9edabedf3fd9d68468ccb8c # Conflicts: # Libraries/Components/View/ReactNativeViewViewConfigMacOS.js # React/Views/RCTView.h # React/Views/RCTViewManager.m # Conflicts: # packages/rn-tester/js/examples/Text/TextExample.ios.js

Summary: This brings in PR microsoft#760 to our repo. Test Plan: RNTester > View: {F612877442} Reviewers: skyle Reviewed By: skyle Subscribers: necolas, eliwhite Differential Revision: https://phabricator.intern.facebook.com/D28158243 Tasks: T78743077 Signature: 28158243:1620330169:72c6c9e2371f2ac1a9edabedf3fd9d68468ccb8c Remove Cursor for Text Component

Summary: This brings in PR microsoft#760 to our repo. Test Plan: RNTester > View: {F612877442} Reviewers: skyle Reviewed By: skyle Subscribers: necolas, eliwhite Differential Revision: https://phabricator.intern.facebook.com/D28158243 Tasks: T78743077 Signature: 28158243:1620330169:72c6c9e2371f2ac1a9edabedf3fd9d68468ccb8c # Conflicts: # Libraries/Components/View/ReactNativeViewViewConfigMacOS.js # React/Views/RCTView.h # React/Views/RCTViewManager.m # Conflicts: # packages/rn-tester/js/examples/Text/TextExample.ios.js

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

ryanlntn added 5 commits April 19, 2021 13:01

add cursor prop to view

f8e1168

add cursor to touchables

24b7750

use enum with as many css names and native names as we can support

9429d10

clean up

84da3f2

add cursor to view example

a91c0ec

ryanlntn requested a review from alloy as a code owner April 20, 2021 15:18

appden added the Partner: Facebook label Apr 22, 2021

appden reviewed Apr 22, 2021

View reviewed changes

ryanlntn added 4 commits April 22, 2021 15:17

update to only CSS kebab-case names

c452cac

update examples to use new cursor names

2a272ed

update default cursor to arrow

945549c

remove auto case and clean up

6b78c52

NickGerleman approved these changes Apr 22, 2021

View reviewed changes

NickGerleman reviewed Apr 22, 2021

View reviewed changes

Libraries/Components/Touchable/TouchableBounce.js Outdated Show resolved Hide resolved

add cursor to flow types for view and touchables

b99356f

default to auto; fix arrow; always discard rects

fce50ea

appden reviewed May 6, 2021

View reviewed changes

apply patch making it a style prop

34275a7

ryanlntn requested review from a team as code owners August 9, 2022 21:09

ryanlntn force-pushed the feat/cursor branch from 6d81baf to 1a3abaa Compare August 9, 2022 21:10

analysis-bot suggested changes Aug 9, 2022

View reviewed changes

ghost added the Needs: Author Feedback label Aug 9, 2022

Saadnajmi reviewed Aug 9, 2022

View reviewed changes

React/Base/macOS/RCTUIKit.m Show resolved Hide resolved

satisfy linter

43542f8

ghost removed the Needs: Author Feedback label Aug 9, 2022

Saadnajmi added the Needs: Author Feedback label Aug 9, 2022

Saadnajmi approved these changes Aug 9, 2022

View reviewed changes

ghost removed the Needs: Author Feedback label Aug 10, 2022

remove view prop and fix build errors

4eb3cd5

Merge branch 'main' into feat/cursor

e6a4b5d

mischreiber merged commit 0ef7992 into microsoft:main Aug 22, 2022

harrieshin mentioned this pull request Oct 19, 2022

Wrong cursor on hover for TextInput multiline={false} editable={false} #926

Closed

shwanton mentioned this pull request Nov 4, 2022

Add cursor style to Text #1469

Merged

4 tasks

Saadnajmi mentioned this pull request Jan 16, 2024

Cursor pointer API react-native-community/discussions-and-proposals#750

Closed

This was referenced Feb 20, 2024

feat(iOS): Implement cursor style prop facebook/react-native#43078

Closed

Implement cursor: pointer for iOS / visionOS #2080

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cursor prop to View and Touchables #760

Add cursor prop to View and Touchables #760

ryanlntn commented Apr 20, 2021

appden Apr 22, 2021

ryanlntn Apr 22, 2021

appden Apr 22, 2021

ryanlntn Apr 22, 2021

appden Apr 22, 2021

appden Apr 22, 2021

HaddyYang Jul 30, 2021

appden Apr 22, 2021

NickGerleman commented Apr 22, 2021

NickGerleman commented Apr 22, 2021

NickGerleman commented Apr 22, 2021

lyahdav commented Apr 29, 2021

ryanlntn commented Apr 30, 2021

lyahdav commented May 3, 2021

lyahdav commented May 4, 2021

NickGerleman commented May 4, 2021

appden May 6, 2021

appden May 6, 2021

analysis-bot left a comment

shwanton commented Aug 9, 2022

Saadnajmi commented Aug 9, 2022

Saadnajmi commented Aug 9, 2022

ryanlntn commented Aug 10, 2022

Saadnajmi commented Aug 11, 2022

analysis-bot commented Aug 12, 2022

	@implementation RCTConvert (UIView)
	@implementation RCTConvert (RCTCursor)

Add cursor prop to View and Touchables #760

Add cursor prop to View and Touchables #760

Conversation

ryanlntn commented Apr 20, 2021

Please select one of the following

Summary

Changelog

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickGerleman commented Apr 22, 2021

NickGerleman commented Apr 22, 2021

NickGerleman commented Apr 22, 2021

lyahdav commented Apr 29, 2021

ryanlntn commented Apr 30, 2021

lyahdav commented May 3, 2021

lyahdav commented May 4, 2021

NickGerleman commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

shwanton commented Aug 9, 2022

Saadnajmi commented Aug 9, 2022

Saadnajmi commented Aug 9, 2022

ryanlntn commented Aug 10, 2022

Saadnajmi commented Aug 11, 2022

analysis-bot commented Aug 12, 2022