Support arbitrary selectable text in Text component #1346

shwanton · 2022-08-10T16:51:30Z

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

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.

Differential Revision: https://phabricator.intern.facebook.com/D27484533

Tasks: T83817888

Changelog

[macOS] [Fixed] - Support arbitrary selectable text in Text component
[macOS] [Fixed] - Add focusable and enableFocusRing props to Text to control focus behavior

Test Plan

before (RN iOS behavior)

after

CleanShot.2022-09-28.at.16.04.59.mp4

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

Libraries/Text/Text/RCTTextView.h

packages/rn-tester/js/components/RNTesterPage.js

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

lyahdav · 2022-08-22T21:11:59Z

@shwanton seems like you can remove this line from the PR summary now?

Note the change to RNTesterPage.js was required to fix #754

Saadnajmi · 2022-08-23T18:01:42Z

We probably want to do a lot more testing for this specific change, as it's a fundamental difference for how we render text. Curious, have y'all done any performance impact testing with this changee?

lyahdav · 2022-08-23T18:28:06Z

We did not do any performance impact testing. Does this repo (internally to MSFT or in OSS) have any performance testing setup?

Saadnajmi · 2022-08-23T18:31:28Z

We did not do any performance impact testing. Does this repo (internally to MSFT or in OSS) have any performance testing setup?

Not that I'm aware of. I guess the more generic question: What kind of testing was done with making sure that Text still rendered the same everywhere? I can imagine just running RN-Tester before and after this change is a good start (we'll probably do that too).

Also curious if you all heard back from RN Core about why they didn't use UITextView with iOS?

lyahdav · 2022-08-23T18:39:30Z

What kind of testing was done with making sure that Text still rendered the same everywhere?

We went through all the Text examples in RN Tester and confirmed the before/after were identical.

Also curious if you all heard back from RN Core about why they didn't use UITextView with iOS?

I'll let @shwanton answer this, he has an internal thread on this.

christophpurrer · 2022-08-23T19:05:20Z

What kind of testing was done with making sure that Text still rendered the same everywhere?

Another data point -> we shipped Messenger Desktop (macOS) with that change end of 2021 and so far haven't been aware of any text rendering issues.

shwanton · 2022-08-24T17:51:20Z

Also curious if you all heard back from RN Core about why they didn't use UITextView with iOS?

This Textview refactor happened in late 2017 so not many folks have full context on why we removed UITextView. However, it does look like the performance reasons should be now mitigated. One main callout was that "it does not instantiate UIView for virtual components which reduces memory utilization", which should be solved with Fabric.

RN Core recommended we run an experiment before switching out UITextView on iOS. This work is planned but has not been started yet.

We have been using this on Messenger Desktop all year & haven't seen any regressions or issues w/ NSTextView.
I can do some more general smoke tests & post the before/after shots in the test plan as well.

shwanton · 2022-08-30T00:01:21Z

When we added the change, we did a visual diff of the Text component in RN Tester. (The differences are highlighted in pink)

The font smoothing isn't something we need
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

shwanton · 2022-09-08T19:12:05Z

@Saadnajmi was there any other data you needed to help this PR get merged?

Saadnajmi · 2022-09-08T20:14:02Z

@shwanton Sorry, I totally missed the screenshots you sent. Thanks! @mischreiber FYI

lyahdav · 2022-09-09T17:18:40Z

I have some concerns with this PR as-is:

It seems this brings in an older implementation of our internal changes. We have fixed some bugs, for example with the hitTest: method. @shwanton an example diff is D37429403. You might want to audit if there are any other changes we missed here.
There's a known issue internally where sometimes mouse clicks are lost in our RN Mac fork. We still haven't gotten to the bottom of it, but since that hitTest and mouseDown code are kind of hacky, I wonder if this is related. @shwanton are we confident the lost mouse clicks is related to our RCTTouchHandler changes and not this?
There are potentially other bugs with this implementation. @shwanton T124433678 might require changes to this code. Is that a task you could take on? If so I could do some knowledge transfer.

Libraries/Text/Text/RCTTextView.m

lyahdav · 2022-09-09T17:27:45Z

@shwanton another example of a change we might want to make this to PR: D28557126

Libraries/Text/Text/RCTTextView.m

… macOS

Libraries/Text/Text.js

mischreiber

Gotta fix the failing checks first, of course.

shwanton · 2022-10-19T18:53:27Z

@mischreiber Fixed!

christophpurrer · 2022-10-20T07:10:12Z

nice work!

* 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]>

Summary: isHighlighted is only used for iOS. Even macOS disables it (see microsoft#1346). This change ensures that the isHighlighted prop is only updated for iOS. [General] [Fixed] - Avoids re-renders during text selection on desktop platforms by limiting native-only `isHighlighted` prop to iOS Reviewed By: lenaic, sammy-SC Differential Revision: D47800845 fbshipit-source-id: cadfe0cd64de8d98a10e3f24b3ae17b9bada8b3a

Summary: Pull Request resolved: facebook#38642 isHighlighted is only used for iOS. Even macOS disables it (see microsoft#1346). This change ensures that the isHighlighted prop is only updated for iOS. ## Changelog: [General] [Fixed] - Avoids re-renders during text selection on desktop platforms by limiting native-only `isHighlighted` prop to iOS Reviewed By: lenaic, sammy-SC Differential Revision: D47800845 fbshipit-source-id: 9888db039f5b955da0c3dfa43dd304f3f4d01f19

Summary: Pull Request resolved: #38642 isHighlighted is only used for iOS. Even macOS disables it (see microsoft#1346). This change ensures that the isHighlighted prop is only updated for iOS. ## Changelog: [General] [Fixed] - Avoids re-renders during text selection on desktop platforms by limiting native-only `isHighlighted` prop to iOS Reviewed By: lenaic, sammy-SC Differential Revision: D47800845 fbshipit-source-id: af109be17027b2fbc9408e2ec9e1b841c709fe35

Summary: Pull Request resolved: facebook#38642 isHighlighted is only used for iOS. Even macOS disables it (see microsoft#1346). This change ensures that the isHighlighted prop is only updated for iOS. ## Changelog: [General] [Fixed] - Avoids re-renders during text selection on desktop platforms by limiting native-only `isHighlighted` prop to iOS Reviewed By: lenaic, sammy-SC Differential Revision: D47800845 fbshipit-source-id: af109be17027b2fbc9408e2ec9e1b841c709fe35

Summary: Pull Request resolved: facebook#38642 isHighlighted is only used for iOS. Even macOS disables it (see #1346). This change ensures that the isHighlighted prop is only updated for iOS. ## Changelog: [General] [Fixed] - Avoids re-renders during text selection on desktop platforms by limiting native-only `isHighlighted` prop to iOS Reviewed By: lenaic, sammy-SC Differential Revision: D47800845 fbshipit-source-id: af109be17027b2fbc9408e2ec9e1b841c709fe35 Co-authored-by: Eric Rozell <[email protected]>

Summary: Pull Request resolved: facebook#38642 isHighlighted is only used for iOS. Even macOS disables it (see microsoft#1346). This change ensures that the isHighlighted prop is only updated for iOS. ## Changelog: [General] [Fixed] - Avoids re-renders during text selection on desktop platforms by limiting native-only `isHighlighted` prop to iOS Reviewed By: lenaic, sammy-SC Differential Revision: D47800845 fbshipit-source-id: af109be17027b2fbc9408e2ec9e1b841c709fe35

Saadnajmi added the Partner: Facebook label Aug 10, 2022

shwanton changed the title ~~[RFC] Support arbitrary selectable text in Text component~~ Support arbitrary selectable text in Text component Aug 10, 2022

christophpurrer reviewed Aug 10, 2022

View reviewed changes

Libraries/Text/Text/RCTTextView.h Outdated Show resolved Hide resolved

christophpurrer reviewed Aug 10, 2022

View reviewed changes

packages/rn-tester/js/components/RNTesterPage.js Outdated Show resolved Hide resolved

Shawn Dempsey added 3 commits August 10, 2022 14:22

Fix GH tags

c8917ad

Revert RNTesterPage style change

13ac427

Merge remote-tracking branch 'microsoft/main' into text-selection

a4d3720

shwanton marked this pull request as ready for review August 10, 2022 21:27

shwanton requested a review from a team as a code owner August 10, 2022 21:27

appden and others added 2 commits August 17, 2022 11:07

Merge remote-tracking branch 'microsoft/main' into text-selection

a4d20e0

Merge remote-tracking branch 'microsoft/main' into text-selection

007317f

Merge branch 'main' into text-selection

4a64054

lyahdav reviewed Sep 9, 2022

View reviewed changes

Libraries/Text/Text/RCTTextView.m Show resolved Hide resolved

Libraries/Text/Text/RCTTextView.m Show resolved Hide resolved

Merge remote-tracking branch 'microsoft/main' into text-selection

7d3b7ee

Saadnajmi assigned mischreiber Sep 12, 2022

Merge remote-tracking branch 'microsoft/main' into text-selection

c3231cf

shwanton commented Sep 28, 2022

View reviewed changes

Libraries/Text/Text/RCTTextView.m Show resolved Hide resolved

Shawn Dempsey added 3 commits September 28, 2022 16:12

Fix macos tags

96421cb

Merge remote-tracking branch 'microsoft/main' into text-selection

a9b9d13

Remove iOS only highlighted prop that was causing re-render issues on…

e8c32de

… macOS

shwanton commented Oct 5, 2022

View reviewed changes

Libraries/Text/Text.js Show resolved Hide resolved

Merge remote-tracking branch 'microsoft/main' into text-selection

42af687

mischreiber approved these changes Oct 18, 2022

View reviewed changes

Shawn Dempsey added 2 commits October 18, 2022 16:22

Merge remote-tracking branch 'microsoft/main' into text-selection

feb63ab

yarn lint

9f49a9c

Saadnajmi merged commit cf48f8d into microsoft:main Oct 19, 2022

This was referenced Oct 19, 2022

Text selectable does not allow normal cursor text selection #906

Closed

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

Closed

shwanton deleted the text-selection branch November 3, 2022 23:20

shwanton mentioned this pull request Nov 4, 2022

Add cursor style to Text #1469

Merged

4 tasks

shwanton mentioned this pull request Nov 17, 2022

Only forward click events to NSTextview #1515

Merged

4 tasks

Saadnajmi mentioned this pull request Jul 6, 2023

[Key handling] pass through all keys; allow specifying modifiers for validKeys[Down|Up] #1867

Merged

4 tasks

rozele mentioned this pull request Jul 26, 2023

Only apply isHighlighted native prop on iOS facebook/react-native#38642

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support arbitrary selectable text in Text component #1346

Support arbitrary selectable text in Text component #1346

shwanton commented Aug 10, 2022 •

edited

Loading

lyahdav commented Aug 22, 2022

Saadnajmi commented Aug 23, 2022

lyahdav commented Aug 23, 2022

Saadnajmi commented Aug 23, 2022

lyahdav commented Aug 23, 2022

christophpurrer commented Aug 23, 2022

shwanton commented Aug 24, 2022

shwanton commented Aug 30, 2022

shwanton commented Sep 8, 2022

Saadnajmi commented Sep 8, 2022

lyahdav commented Sep 9, 2022

lyahdav commented Sep 9, 2022

mischreiber left a comment

shwanton commented Oct 19, 2022

christophpurrer commented Oct 20, 2022

Support arbitrary selectable text in Text component #1346

Support arbitrary selectable text in Text component #1346

Conversation

shwanton commented Aug 10, 2022 • edited Loading

Please select one of the following

Summary

Changelog

Test Plan

lyahdav commented Aug 22, 2022

Saadnajmi commented Aug 23, 2022

lyahdav commented Aug 23, 2022

Saadnajmi commented Aug 23, 2022

lyahdav commented Aug 23, 2022

christophpurrer commented Aug 23, 2022

shwanton commented Aug 24, 2022

shwanton commented Aug 30, 2022

shwanton commented Sep 8, 2022

Saadnajmi commented Sep 8, 2022

lyahdav commented Sep 9, 2022

lyahdav commented Sep 9, 2022

mischreiber left a comment

Choose a reason for hiding this comment

shwanton commented Oct 19, 2022

christophpurrer commented Oct 20, 2022

shwanton commented Aug 10, 2022 •

edited

Loading