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

Fix disable scrolling for multiline text input #1244

Merged

Conversation

shwanton
Copy link

@shwanton shwanton commented Jul 8, 2022

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

RN iOS supports disabling Multiline text input scrolling with scrollEnabled prop. https://reactnative.dev/docs/0.68/textinput#scrollenabled-ios

Since macOS does not use UITextView which inherits from UIScrollView, the RCTBaseTextInputViewManager scrollEnabled remap was not used.

On macOS, RCTMultilineTextInputView creates a RCTUIScrollView which is where we need to disable scrolling.

_scrollView = [[RCTUIScrollView alloc] initWithFrame:self.bounds]; // TODO(macOS ISS#3536887)

RCTUIScrollView inherits from NSScrollView which does not have a scrollEnabled property, but there is a scrollWheel callback api we can use to disable scrolling when scrollEnabled is false.

UPDATE:

This approach was problematic since overriding scrollWheel for all scrollViews caused issues when pasting large amounts of text.
This also did not disable keyboard scrolling with up/down keys.

Another solution is to disable scrolling for TextInput is to create a custom clipview & constrain scrolling.

#1366 added vertical scrollbars to all multiline text inputs. This introduced a layout regression where the scrollbar padding was not taken into account. The scrollbar should also be optional since scrolling can still happen w/o the vertical scrollbar.

Will add follow on diff to move this to configurable prop.

Changelog

[macOS] [Fixed] - Scrolling on Multiline Text Input can be disabled
Fixes #558 & #925

Usage

NOTE: Only works with multiline={true}

<TextInput multiline={true} scrollEnabled={false} ... />

Test Plan

Added Example to RNTester TextInput -> Multiline

CleanShot.2022-09-26.at.10.15.22-converted.mp4

@ghost ghost added the Area: TextInput label Jul 8, 2022
@ghost
Copy link

ghost commented Jul 8, 2022

CLA assistant check
All CLA requirements met.

Shawn Dempsey added 2 commits July 12, 2022 15:58
Summary:
# Context

RN iOS supports disabling scrolling via `scrollEnabled` prop for multiline text inputs.
https://reactnative.dev/docs/0.68/textinput#scrollenabled-ios

This does [not work](microsoft#925) on MacOS

Since MacOS does not use `UITextView` which inherits from `UIScrollView`, the view manager property was set but not actually passed down to the underlying scroll view.

`RCTMultilineTextInputView` creates a `RCTUIScrollView` which is where we need to disable scrolling.
https://www.internalfb.com/code/archon_react_native_macos/[fde4113acd89fb13ee11636c48b59eac49c21bae]/Libraries/Text/TextInput/Multiline/RCTMultilineTextInputView.m?lines=31

`RCTUIScrollView` inherits from `NSScrollView` which does not have a `scrollEnabled` property, but there is an api we can use to disable scrolling when the property is set.

https://developer.apple.com/documentation/appkit/nsscrollview/1403494-scrollwheel

# Usage

NOTE: Only works with `multiline={true}`

```
<TextInput multiline={true} scrollEnabled={false} ... />
```

# Change

* Only expose the `scrollEnabled` property on `RCTMultilineTextInputView`.
  * `RCTSinglelineTextInputView` does not have scrolling so this property is unused.
* `RCTMultilineTextInputView` delegates the `scrollEnabled` to it's underlying `_scrollView`
* `RCTUIScrollView` defaults initial `scrollEnabled` to `YES`
* `RCTUIScrollView` disables scrolling when the `scrollEnabled` is `NO`
@shwanton shwanton force-pushed the shwanton/fix-multiline-textview-scroll-enabled branch from 506ed17 to 338e0e6 Compare July 12, 2022 22:58
@shwanton shwanton marked this pull request as ready for review July 12, 2022 22:59
@shwanton shwanton requested a review from a team as a code owner July 12, 2022 22:59
@Saadnajmi
Copy link
Collaborator

@mischreiber Could I get a second pair of eyes on this PR?

@Saadnajmi
Copy link
Collaborator

And maybe also @chiuam who was looking at TextInput recently

@Saadnajmi
Copy link
Collaborator

We're waiting on the "disable keyboard scrolling" bit to move this PR forward.

@shwanton
Copy link
Author

shwanton commented Aug 9, 2022

Quick update. There is a better solution that disables scroll wheel & text that uses a custom clip view.

Here is a native repro of how it should work: https://github.com/shwanton/DisabledScrollingTextViewExample
CleanShot 2022-08-09 at 11 48 00

There is some weird behavior with how we create textview, so overriding the clip view has unexpected behavior.
Once I figure this out, I'll update the PR.

@ghost
Copy link

ghost commented Aug 16, 2022

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@Saadnajmi
Copy link
Collaborator

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

Uh... Don't close?

@ghost ghost removed the no-recent-activity label Aug 16, 2022
@shwanton
Copy link
Author

Still working on this fix!

@ghost ghost removed the Needs: Author Feedback label Aug 16, 2022
@christophpurrer
Copy link

Will this change now conflict with
#1366
?

Shawn Dempsey added 4 commits September 23, 2022 16:20
…line-textview-scroll-enabled

# Conflicts:
#	Libraries/Text/TextInput/Multiline/RCTMultilineTextInputView.h
#	Libraries/Text/TextInput/Multiline/RCTMultilineTextInputView.m
#	React/Base/RCTUIKit.h
#	React/Base/macOS/RCTUIKit.m
# Conflicts:
#	React/Base/macOS/RCTUIKit.m
@shwanton
Copy link
Author

I've updated the description with the latest implementation details for disabling scrolling in multiline textview.
This solution solves the outstanding issues w/ overriding scrollWheel

The vertical scrollbar should be optional, I will follow up #1368 with a change we made.

CleanShot.2022-09-26.at.10.15.22-converted.mp4

@shwanton
Copy link
Author

@Saadnajmi The updated fix here is ready to be merged!

@chiuam chiuam enabled auto-merge (squash) November 1, 2022 00:31
@chiuam chiuam merged commit 01a7b5a into microsoft:main Nov 1, 2022
@shwanton shwanton deleted the shwanton/fix-multiline-textview-scroll-enabled branch November 3, 2022 23:22
shwanton added a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TextInput] scrollEnabled is not working for the multiline inputs
6 participants