Hit test is broken on Text screen in RNTester app #754
Labels
Comments
ghost
added
|
Commit that added the |
|
I'm not sure if it repros on older versions but I'm guessing it does given that commit is so old (don't have time to verify now). I couldn't find any downsides to removing the padding. I believe the deeper issue is that setting |
christophpurrer
pushed a commit
to christophpurrer/react-native-macos
that referenced
this issue
Jan 22, 2022
Summary: RN Mac's implementation of the `selectable` prop on `Text` only allows selecting the entire Text component and right click to copy. This diff makes the `Text.selectable` prop on Mac allow arbitrary selection. To do this we used `NSTextView` to render the `Text` component instead of RN Mac's custom text rendering, because it has a `selectable` property which gives us the behavior we want. We also allow adding custom menu items in the context menu. Note the change to RNTesterPage.js was required to fix microsoft#754. Test Plan: See test plan of D27250072 for integration to Zeratul. Confirmed text selection works in RNTester Text example: {F588619781} --- Also I went to RNTester Text examples and did an image diff comparison before and after these changes (differences are in pink): {F588602710} - The font smoothing isn't something we need --- {F588602715} - The examples with images are different because they load random images - The pink background on "With size and background color" isn't a difference, the background color is pink in code --- {F588602706} - The <TextInput multiline/> example has an off by 1 pixel difference that wasn't trivial to fix and doesn't seem significant enough to investigate Reviewers: skyle, ericroz Reviewed By: skyle Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D27484533 Tasks: T83817888 Signature: 27484533:1617928003:6c1c60a15db8ef3551aafe22229fafc9fea0053e # Conflicts: # Libraries/Text/Text/RCTTextView.m # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m
christophpurrer
pushed a commit
to christophpurrer/react-native-macos
that referenced
this issue
Jan 30, 2022
Summary: RN Mac's implementation of the `selectable` prop on `Text` only allows selecting the entire Text component and right click to copy. This diff makes the `Text.selectable` prop on Mac allow arbitrary selection. To do this we used `NSTextView` to render the `Text` component instead of RN Mac's custom text rendering, because it has a `selectable` property which gives us the behavior we want. We also allow adding custom menu items in the context menu. Note the change to RNTesterPage.js was required to fix microsoft#754. Test Plan: See test plan of D27250072 for integration to Zeratul. Confirmed text selection works in RNTester Text example: {F588619781} --- Also I went to RNTester Text examples and did an image diff comparison before and after these changes (differences are in pink): {F588602710} - The font smoothing isn't something we need --- {F588602715} - The examples with images are different because they load random images - The pink background on "With size and background color" isn't a difference, the background color is pink in code --- {F588602706} - The <TextInput multiline/> example has an off by 1 pixel difference that wasn't trivial to fix and doesn't seem significant enough to investigate Reviewers: skyle, ericroz Reviewed By: skyle Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D27484533 Tasks: T83817888 Signature: 27484533:1617928003:6c1c60a15db8ef3551aafe22229fafc9fea0053e # Conflicts: # Libraries/Text/Text/RCTTextView.m # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m
christophpurrer
pushed a commit
to christophpurrer/react-native-macos
that referenced
this issue
Mar 5, 2022
Summary: RN Mac's implementation of the `selectable` prop on `Text` only allows selecting the entire Text component and right click to copy. This diff makes the `Text.selectable` prop on Mac allow arbitrary selection. To do this we used `NSTextView` to render the `Text` component instead of RN Mac's custom text rendering, because it has a `selectable` property which gives us the behavior we want. We also allow adding custom menu items in the context menu. Note the change to RNTesterPage.js was required to fix microsoft#754. Test Plan: See test plan of D27250072 for integration to Zeratul. Confirmed text selection works in RNTester Text example: {F588619781} --- Also I went to RNTester Text examples and did an image diff comparison before and after these changes (differences are in pink): {F588602710} - The font smoothing isn't something we need --- {F588602715} - The examples with images are different because they load random images - The pink background on "With size and background color" isn't a difference, the background color is pink in code --- {F588602706} - The <TextInput multiline/> example has an off by 1 pixel difference that wasn't trivial to fix and doesn't seem significant enough to investigate Reviewers: skyle, ericroz Reviewed By: skyle Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D27484533 Tasks: T83817888 Signature: 27484533:1617928003:6c1c60a15db8ef3551aafe22229fafc9fea0053e # Conflicts: # Libraries/Text/Text/RCTTextView.m # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m
|
This issue is stale because it has been open 365 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
It sounds like this year old issue has a fix on your local fork @lyahdav ? |
|
@Saadnajmi yes, in our fork we just deleted that paddingTop. You can see that in the change to RNTesterPage.js here. Just one of many changes we haven't had time to contribute back yet. |
|
I am not able to repro this problem with react-native-macOS 0.68-stable/main at this point Removing Screen.Recording.2022-08-05.at.10.28.21.AM.mov |
christophpurrer
pushed a commit
to christophpurrer/react-native-macos
that referenced
this issue
Aug 9, 2022
Summary: RN Mac's implementation of the `selectable` prop on `Text` only allows selecting the entire Text component and right click to copy. This diff makes the `Text.selectable` prop on Mac allow arbitrary selection. To do this we used `NSTextView` to render the `Text` component instead of RN Mac's custom text rendering, because it has a `selectable` property which gives us the behavior we want. We also allow adding custom menu items in the context menu. Note the change to RNTesterPage.js was required to fix microsoft#754. Test Plan: See test plan of D27250072 for integration to Zeratul. Confirmed text selection works in RNTester Text example: {F588619781} --- Also I went to RNTester Text examples and did an image diff comparison before and after these changes (differences are in pink): {F588602710} - The font smoothing isn't something we need --- {F588602715} - The examples with images are different because they load random images - The pink background on "With size and background color" isn't a difference, the background color is pink in code --- {F588602706} - The <TextInput multiline/> example has an off by 1 pixel difference that wasn't trivial to fix and doesn't seem significant enough to investigate Reviewers: skyle, ericroz Reviewed By: skyle Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D27484533 Tasks: T83817888 Signature: 27484533:1617928003:6c1c60a15db8ef3551aafe22229fafc9fea0053e # Conflicts: # Libraries/Text/Text/RCTTextView.m # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m
christophpurrer
pushed a commit
to christophpurrer/react-native-macos
that referenced
this issue
Aug 9, 2022
Summary: RN Mac's implementation of the `selectable` prop on `Text` only allows selecting the entire Text component and right click to copy. This diff makes the `Text.selectable` prop on Mac allow arbitrary selection. To do this we used `NSTextView` to render the `Text` component instead of RN Mac's custom text rendering, because it has a `selectable` property which gives us the behavior we want. We also allow adding custom menu items in the context menu. Note the change to RNTesterPage.js was required to fix microsoft#754. Test Plan: See test plan of D27250072 for integration to Zeratul. Confirmed text selection works in RNTester Text example: {F588619781} --- Also I went to RNTester Text examples and did an image diff comparison before and after these changes (differences are in pink): {F588602710} - The font smoothing isn't something we need --- {F588602715} - The examples with images are different because they load random images - The pink background on "With size and background color" isn't a difference, the background color is pink in code --- {F588602706} - The <TextInput multiline/> example has an off by 1 pixel difference that wasn't trivial to fix and doesn't seem significant enough to investigate Reviewers: skyle, ericroz Reviewed By: skyle Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D27484533 Tasks: T83817888 Signature: 27484533:1617928003:6c1c60a15db8ef3551aafe22229fafc9fea0053e # Conflicts: # Libraries/Text/Text/RCTTextView.m # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m
christophpurrer
pushed a commit
to christophpurrer/react-native-macos
that referenced
this issue
Aug 10, 2022
Summary: RN Mac's implementation of the `selectable` prop on `Text` only allows selecting the entire Text component and right click to copy. This diff makes the `Text.selectable` prop on Mac allow arbitrary selection. To do this we used `NSTextView` to render the `Text` component instead of RN Mac's custom text rendering, because it has a `selectable` property which gives us the behavior we want. We also allow adding custom menu items in the context menu. Note the change to RNTesterPage.js was required to fix microsoft#754. Test Plan: See test plan of D27250072 for integration to Zeratul. Confirmed text selection works in RNTester Text example: {F588619781} --- Also I went to RNTester Text examples and did an image diff comparison before and after these changes (differences are in pink): {F588602710} - The font smoothing isn't something we need --- {F588602715} - The examples with images are different because they load random images - The pink background on "With size and background color" isn't a difference, the background color is pink in code --- {F588602706} - The <TextInput multiline/> example has an off by 1 pixel difference that wasn't trivial to fix and doesn't seem significant enough to investigate Reviewers: skyle, ericroz Reviewed By: skyle Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D27484533 Tasks: T83817888 Signature: 27484533:1617928003:6c1c60a15db8ef3551aafe22229fafc9fea0053e # Conflicts: # Libraries/Text/Text/RCTTextView.m # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m
shwanton
pushed a commit
to shwanton/react-native-macos
that referenced
this issue
Aug 10, 2022
Summary: RN Mac's implementation of the `selectable` prop on `Text` only allows selecting the entire Text component and right click to copy. This diff makes the `Text.selectable` prop on Mac allow arbitrary selection. To do this we used `NSTextView` to render the `Text` component instead of RN Mac's custom text rendering, because it has a `selectable` property which gives us the behavior we want. We also allow adding custom menu items in the context menu. Note the change to RNTesterPage.js was required to fix microsoft#754. Test Plan: See test plan of D27250072 for integration to Zeratul. Confirmed text selection works in RNTester Text example: {F588619781} --- Also I went to RNTester Text examples and did an image diff comparison before and after these changes (differences are in pink): {F588602710} - The font smoothing isn't something we need --- {F588602715} - The examples with images are different because they load random images - The pink background on "With size and background color" isn't a difference, the background color is pink in code --- {F588602706} - The <TextInput multiline/> example has an off by 1 pixel difference that wasn't trivial to fix and doesn't seem significant enough to investigate Reviewers: skyle, ericroz Reviewed By: skyle Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D27484533 Tasks: T83817888 Signature: 27484533:1617928003:6c1c60a15db8ef3551aafe22229fafc9fea0053e # Conflicts: # Libraries/Text/Text/RCTTextView.m # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m
4 tasks
|
Great! |
shwanton
pushed a commit
to shwanton/react-native-macos
that referenced
this issue
Feb 13, 2023
Summary: RN Mac's implementation of the `selectable` prop on `Text` only allows selecting the entire Text component and right click to copy. This diff makes the `Text.selectable` prop on Mac allow arbitrary selection. To do this we used `NSTextView` to render the `Text` component instead of RN Mac's custom text rendering, because it has a `selectable` property which gives us the behavior we want. We also allow adding custom menu items in the context menu. Note the change to RNTesterPage.js was required to fix microsoft#754. Test Plan: See test plan of D27250072 for integration to Zeratul. Confirmed text selection works in RNTester Text example: {F588619781} --- Also I went to RNTester Text examples and did an image diff comparison before and after these changes (differences are in pink): {F588602710} - The font smoothing isn't something we need --- {F588602715} - The examples with images are different because they load random images - The pink background on "With size and background color" isn't a difference, the background color is pink in code --- {F588602706} - The <TextInput multiline/> example has an off by 1 pixel difference that wasn't trivial to fix and doesn't seem significant enough to investigate Reviewers: skyle, ericroz Reviewed By: skyle Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D27484533 Tasks: T83817888 Signature: 27484533:1617928003:6c1c60a15db8ef3551aafe22229fafc9fea0053e # Conflicts: # Libraries/Text/Text/RCTTextView.m # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m
shwanton
added a commit
to shwanton/react-native-macos
that referenced
this issue
Feb 13, 2023
* Support arbitrary selectable text in Text component Summary: RN Mac's implementation of the `selectable` prop on `Text` only allows selecting the entire Text component and right click to copy. This diff makes the `Text.selectable` prop on Mac allow arbitrary selection. To do this we used `NSTextView` to render the `Text` component instead of RN Mac's custom text rendering, because it has a `selectable` property which gives us the behavior we want. We also allow adding custom menu items in the context menu. Note the change to RNTesterPage.js was required to fix microsoft#754. Test Plan: See test plan of D27250072 for integration to Zeratul. Confirmed text selection works in RNTester Text example: {F588619781} --- Also I went to RNTester Text examples and did an image diff comparison before and after these changes (differences are in pink): {F588602710} - The font smoothing isn't something we need --- {F588602715} - The examples with images are different because they load random images - The pink background on "With size and background color" isn't a difference, the background color is pink in code --- {F588602706} - The <TextInput multiline/> example has an off by 1 pixel difference that wasn't trivial to fix and doesn't seem significant enough to investigate Reviewers: skyle, ericroz Reviewed By: skyle Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D27484533 Tasks: T83817888 Signature: 27484533:1617928003:6c1c60a15db8ef3551aafe22229fafc9fea0053e # Conflicts: # Libraries/Text/Text/RCTTextView.m # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m * Fix GH tags * Revert RNTesterPage style change * Fix hit-testing in RCTTextView for selection Summary: This fixes a regression introduced by D29340382 since the `contentView` of the window was changed to the `RCTRootView` instance. The problem isn't there though, but is due to now the `contentView` having flipped geometry. The `hitTest:` method expects coordinates in the superview's coordinate space: > A point that is in the coordinate system of the view’s superview, not of the view itself. Also see how `RCTTouchHandler` also calls `convertPoint:` on the `superview` before passing to `hitTest:` for the same reason: https://fburl.com/diffusion/krx4lxao Test Plan: {F628902534} Reviewers: lyahdav Reviewed By: lyahdav Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D29469639 Tasks: T94420821 Signature: 29469639:1625001662:97028699aee404282c83e35cd66f6308bc793a2a * Revert changes to touch handler * Only keep NSTextView changes * Remove unused property * Re-add focus ring for selected text * Fix typos * Ensure that RCTTextView manages the key loop view * move focusable property lower in list * Fix macos tags * Remove iOS only highlighted prop that was causing re-render issues on macOS * yarn lint Co-authored-by: Liron Yahdav <[email protected]> Co-authored-by: Shawn Dempsey <[email protected]> Co-authored-by: Scott Kyle <[email protected]>
shwanton
pushed a commit
to shwanton/react-native-macos
that referenced
this issue
Mar 10, 2023
Summary: RN Mac's implementation of the `selectable` prop on `Text` only allows selecting the entire Text component and right click to copy. This diff makes the `Text.selectable` prop on Mac allow arbitrary selection. To do this we used `NSTextView` to render the `Text` component instead of RN Mac's custom text rendering, because it has a `selectable` property which gives us the behavior we want. We also allow adding custom menu items in the context menu. Note the change to RNTesterPage.js was required to fix microsoft#754. Test Plan: See test plan of D27250072 for integration to Zeratul. Confirmed text selection works in RNTester Text example: {F588619781} --- Also I went to RNTester Text examples and did an image diff comparison before and after these changes (differences are in pink): {F588602710} - The font smoothing isn't something we need --- {F588602715} - The examples with images are different because they load random images - The pink background on "With size and background color" isn't a difference, the background color is pink in code --- {F588602706} - The <TextInput multiline/> example has an off by 1 pixel difference that wasn't trivial to fix and doesn't seem significant enough to investigate Reviewers: skyle, ericroz Reviewed By: skyle Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D27484533 Tasks: T83817888 Signature: 27484533:1617928003:6c1c60a15db8ef3551aafe22229fafc9fea0053e # Conflicts: # Libraries/Text/Text/RCTTextView.m # React/Base/RCTTouchHandler.h # React/Base/RCTTouchHandler.m
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Environment
react-native -v: I'm on this commit:npm ls react-native-macos:(empty)node -v:v12.14.1npm -v:6.13.4yarn --version:1.21.1xcodebuild -version:Issue
When I run RNTester on Mac and go to Text examples, if I right click on the selectable text example, it will only show the correct context menu if I click within the top few pixels of the component.
Steps to Reproduce
Expected Behavior
Context menu with copy should show.
Actual Behavior
RN Context menu shows.
Reproducible Demo
The first click in this video is on top half (works fine), second click is bottom half (bug).
Screen.Recording.2021-04-05.at.4.20.53.PM.mov
Additional context
The bug is due to this line: https://github.com/microsoft/react-native-macos/blob/master/RNTester/js/components/RNTesterPage.js#L72
If I delete it, everything works fine. It seems that setting a
paddingToponScrollViewbreaks hit tests.The text was updated successfully, but these errors were encountered: