feat: support maxLength prop in web #505

dominictb · 2024-09-26T08:37:10Z

Details

This PR includes the implementation of maxLength prop in web

Related Issues

Expensify/App#46766

Manual Tests

Paste this text file (over 400k characters)
default.txt
Check that the text input in both web and native is cut off to 30k length

Linked PRs

Pending App PR

…gth-prop-web

Skalakid · 2024-09-26T10:14:59Z

Hello, I'm testing your changes and I found couple of bugs:

The input value isn't matching the input content. The value (in the picture the text under the input) should be the same as the text inside the component

Skalakid · 2024-09-26T10:21:06Z

There are some problems with entering text when we reach the maxLength limit. Steps:

move the cursor to the end of the text (on the maxLength index)
start typing
notice that the old text is being replaced

Screen.Recording.2024-09-26.at.12.16.40.mov

Skalakid · 2024-09-26T10:44:22Z

Pasting text at the end of the maxLength limit is replacing previous text:

go to the end of the text
copy previous text
start pasting it
notice that the value is changing

Screen.Recording.2024-09-26.at.12.27.45.mov

dominictb · 2024-09-26T10:45:31Z

@Skalakid in the latest commit, I have made themaxLength behavior consistent between web and native, which include:

For normal input event type, the text input won't change the state (value and cursor position) if the text length is larger than limit
For paste event, the final text value will be cut off.

To test the behavior, please modify the maxLength in the example App to small value (like 20) to test

For maxLength=30000, we can see that the performance is significantly improved with this change #504

in pasteText event

dominictb · 2024-09-26T11:15:35Z

Small change on the latest commit: I tested the paste text in native, and it seems like the app will cut off the inserted text until it fits the maxLength, or do not paste anything at all (keep the old text), so I updated the web implementation to reflect that.

Skalakid · 2024-09-26T11:29:18Z

Okay, thank you. Now everything works fine :D

There is one more thing that I wanted to fix there. How about adding value cutting on the first rerender? Currently, when you pass the long text in the value prop and the app loads, the maxLength prop isn't cutting the input value:

(In the image, I have maxLength={5} and default text passed in the value prop)

I know the same thing happens on the default RN TextInput, but I think it's worth fixing in our case. What do you think about it?

both in native and web

dominictb · 2024-09-27T07:55:08Z

@Skalakid the last 2 commits contain this change as you suggested. Tested on web, Android and iOS.

Skalakid · 2024-09-27T11:46:13Z

Bug: Text isn't being replaced when you reach the limit of maxLength prop

Screen.Recording.2024-09-27.at.13.37.50.mov

Expected behavior:

Screen.Recording.2024-09-27.at.13.45.04.mov

…suffix paste event - web platform

dominictb · 2024-09-30T03:29:33Z

@Skalakid fixed!

Skalakid

Overall, it looks good :D Left some small comments

Skalakid · 2024-09-30T09:40:03Z

src/MarkdownTextInput.web.tsx

          inputType === 'pasteText' ? pasteContent.current || '' : parseInnerHTMLToText(e.target as MarkdownTextInputElement, inputType, contentSelection.current.start),
        );

        if (pasteContent.current) {
          pasteContent.current = null;
        }

+        if (typeof maxLength === 'number' && parsedText.length > maxLength) {


Suggested change

if (typeof maxLength === 'number' && parsedText.length > maxLength) {

if (maxLength !== undefined && parsedText.length > maxLength) {

Skalakid · 2024-09-30T09:40:23Z

src/MarkdownTextInput.web.tsx

+        let availableLength = text.length;
+        const prefix = divRef.current.value.substring(0, contentSelection.current.start);
+        const suffix = divRef.current.value.substring(contentSelection.current.end);
+        if (typeof maxLength === 'number') {


Skalakid · 2024-09-30T09:41:42Z

src/MarkdownTextInput.tsx

  const IS_FABRIC = 'nativeFabricUIManager' in global;

  const markdownStyle = React.useMemo(() => processMarkdownStyle(props.markdownStyle), [props.markdownStyle]);

+  const processedText = React.useMemo(() => {
+    if (typeof maxLength === 'number') {


Suggested change

if (typeof maxLength === 'number') {

if (maxLength !== undefined) {

Skalakid · 2024-09-30T09:42:06Z

src/MarkdownTextInput.tsx

+  }, [value, maxLength]);
+
+  React.useLayoutEffect(() => {
+    if (typeof maxLength !== 'number' || !value || value.length <= maxLength || !onChangeText) {


Suggested change

if (typeof maxLength !== 'number' || !value || value.length <= maxLength || !onChangeText) {

if (maxLength === undefined || !value || value.length <= maxLength || !onChangeText) {

…gth-prop-web

Skalakid

LGTM

tomekzaw · 2024-10-01T06:36:51Z

src/MarkdownTextInput.tsx

+        value={processedText}
+        onChangeText={onChangeText}


What if the component was supposed to be uncontrolled? I don't really like this logic here

What do you suggest otherwise? For me it looks completely fine. The intention is to cut off the initial value props if it is provided, and if possible, use onChangeText to update the state in the parent component.

If the component was supposed to be uncontrolled then the value props should be undefined, hence we don't have to worry about this case.

…gth-prop-web

dominictb · 2024-10-03T04:42:43Z

@tomekzaw do you have any further concerns? Could we merge this PR?

tomekzaw · 2024-10-03T07:11:15Z

@dominictb Why can't we just pass maxLength prop to TextInput in the native implementation?

dominictb · 2024-10-03T07:57:05Z

@tomekzaw it's coming from here: #505 (comment). Basically for value > maxLength, native component will just render the whole value without cutting off anything, but the user couldn't edit the text box unless they delete a portion of text so that the length is below the limit.

Now we have 2 options:

Keep the behavior of the native TextInput, and implement the web to follow that.
Update implementation on both web and native to cut off the value text if it exceeds maxLength

@Skalakid favors the second option, so I need to update the native implementation accordingly

tomekzaw · 2024-10-03T10:31:58Z

@dominictb Thanks for the response

Basically for value > maxLength, native component will just render the whole value without cutting off anything

Isn't it something we should fix directly in react-native? Or, alternatively, fix this inside E/App for Android and iOS? I'd like MarkdownTextInput to mimic TextInput as closely as possible. So the behavior of maxLength prop should be analogous for TextInput and MarkdownTextInput. If the implementation of maxLength for TextInput is incorrect, we should fix it either in E/App (just like we would do for a regular TextInput) or directly at the source (i.e. in react-native).

tomekzaw · 2024-10-03T10:45:15Z

Actually, a regular <input type="text"> on web also behaves the same way as currently – if you set maxlength="10" value="1234567890abcde", all 15 characters are visible but you can't add or remove characters (but you can select all characters and replace the contents as long as the new text is no longer than 10 characters). I think this is the expected behavior.

dominictb · 2024-10-03T10:46:44Z

Alright, I can revert the change. One moment.

Skalakid · 2024-10-03T10:53:30Z

Okay, @tomekzaw convinced me, let's revert changes on the native side and focus on implementing the maxLength prop on the web only. Let's make sure it will work the same as the native one and won't introduce any new bug

tomekzaw · 2024-10-03T11:01:34Z

Basically, maxLength prop for MarkdownTextInput should work the same way as for TextInput.

For Android and iOS, let's just pass this prop to TextInput just like we already do (via {...props}).

For web, we'll need to implement maxLength logic since we're using contenteditable which doesn't support maxlength out-of-the-box.

All other logic should be moved to E/App.

dominictb · 2024-10-04T05:26:52Z

done!

tomekzaw · 2024-10-04T06:30:51Z

@dominictb Thanks!

Skalakid

LGTM

dominictb · 2024-10-04T08:13:22Z

Resolved conflicts!

dominictb · 2024-10-07T08:11:13Z

Waiting for your final say @Skalakid.

dominictb added 2 commits September 26, 2024 15:34

feat: support maxLength prop in web

45995c3

Merge remote-tracking branch 'origin/main' into feat/app-46766/maxLen…

1506afd

…gth-prop-web

fix: unify maxLength behavior on both web and native

ed71ef1

fix: update maxLength behavior in web to match with native

7750fd8

in pasteText event

dominictb added 2 commits September 27, 2024 14:52

feat: cut off initial value if it is over maxLength limit

5a8f1f7

both in native and web

fix: revert the example/App.tsx change

2c0d0e0

fix: calculate avail length of inserted text by remaining prefix and …

8d85c2a

…suffix paste event - web platform

Skalakid self-requested a review September 30, 2024 09:27

Skalakid reviewed Sep 30, 2024

View reviewed changes

fix: check maxLength against undefined value

1f69021

dominictb requested a review from Skalakid September 30, 2024 17:16

dominictb added 2 commits October 1, 2024 00:17

Merge remote-tracking branch 'origin/main' into feat/app-46766/maxLen…

1b26a57

…gth-prop-web

chore: update Podfile.lock

ef63f73

Skalakid previously approved these changes Oct 1, 2024

View reviewed changes

tomekzaw requested changes Oct 1, 2024

View reviewed changes

dominictb requested a review from tomekzaw October 2, 2024 07:50

Merge remote-tracking branch 'origin/main' into feat/app-46766/maxLen…

f81a4bd

…gth-prop-web

dominictb dismissed Skalakid’s stale review via f81a4bd October 2, 2024 14:32

fix: remove maxLength cut for value props

872fb83

tomekzaw requested a review from Skalakid October 4, 2024 06:30

Skalakid previously approved these changes Oct 4, 2024

View reviewed changes

Merge branch 'main' into feat/app-46766/maxLength-prop-web

df13406

dominictb dismissed Skalakid’s stale review via df13406 October 4, 2024 08:13

tomekzaw approved these changes Oct 4, 2024

View reviewed changes

dominictb requested a review from Skalakid October 4, 2024 08:23

Skalakid merged commit e81fe7e into Expensify:main Oct 7, 2024
7 checks passed

dominictb mentioned this pull request Oct 8, 2024

[$500] Android - Chat - Trying to paste onyx data of #applause.expensifail.com chat app freezes Expensify/App#46766

Open

6 tasks

	if (typeof maxLength === 'number' && parsedText.length > maxLength) {
	if (maxLength !== undefined && parsedText.length > maxLength) {

	if (typeof maxLength === 'number') {
	if (maxLength !== undefined) {

	if (typeof maxLength !== 'number' \|\| !value \|\| value.length <= maxLength \|\| !onChangeText) {
	if (maxLength === undefined \|\| !value \|\| value.length <= maxLength \|\| !onChangeText) {

feat: support maxLength prop in web #505

feat: support maxLength prop in web #505

Conversation

dominictb commented Sep 26, 2024

Details

Related Issues

Manual Tests

Linked PRs

Skalakid commented Sep 26, 2024

Skalakid commented Sep 26, 2024

Skalakid commented Sep 26, 2024

dominictb commented Sep 26, 2024

dominictb commented Sep 26, 2024

Skalakid commented Sep 26, 2024 • edited Loading

dominictb commented Sep 27, 2024 • edited Loading

Skalakid commented Sep 27, 2024

dominictb commented Sep 30, 2024

Skalakid left a 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

Skalakid Sep 30, 2024 • edited Loading

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

Skalakid left a 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

dominictb commented Oct 3, 2024

tomekzaw commented Oct 3, 2024

dominictb commented Oct 3, 2024

tomekzaw commented Oct 3, 2024 • edited Loading

tomekzaw commented Oct 3, 2024 • edited Loading

dominictb commented Oct 3, 2024

Skalakid commented Oct 3, 2024 • edited Loading

tomekzaw commented Oct 3, 2024

dominictb commented Oct 4, 2024

tomekzaw commented Oct 4, 2024

Skalakid left a comment

Choose a reason for hiding this comment

dominictb commented Oct 4, 2024 • edited Loading

dominictb commented Oct 7, 2024

Skalakid commented Sep 26, 2024 •

edited

Loading

dominictb commented Sep 27, 2024 •

edited

Loading

Skalakid Sep 30, 2024 •

edited

Loading

tomekzaw commented Oct 3, 2024 •

edited

Loading

tomekzaw commented Oct 3, 2024 •

edited

Loading

Skalakid commented Oct 3, 2024 •

edited

Loading

dominictb commented Oct 4, 2024 •

edited

Loading