[HOLD for payment 2024-03-11] [HOLD for payment 2024-02-20][$1000] Form: Inconsistent text input submitting experience between mWeb and Native

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.0-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation: @

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Tap profile icon
3. Tap profile
4. Tap Status
5. Enter any text
6. Tap enter
7. Note the user is navigated to "set your status" page
8. Launch app
9. Perform step2-step6

Expected Result:

Entering status message&tapping enter must show same behaviour in mweb and android.

Actual Result:

In mweb, entering status message and tapping enter, user is navigated to previous "set your status" page but in Android, user stays in the same status page.

Entering status message&tapping enter shows different behaviour in mweb and android.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6278848_1700128093817.status.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d451a9a0af00ce03
• Upwork Job ID: 1725114933864177664
• Last Price Increase: 2023-12-19
• Automatic offers:
• cubuspl42 | Reviewer | 28088569
• shubham1206agra | Contributor | 28088570

[melvin-bot]

Triggered auto assignment to @conorpendergrast (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01d451a9a0af00ce03

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @cubuspl42 (External)

[s-alves10]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Typing status and pressing Enter behaves differently in mWeb and android

What is the root cause of that problem?

As you can see below, we disabled the press on enter function in FormWrapper

App/src/components/Form/FormWrapper.js

Line 154 in 69a3358

disablePressOnEnter

How does 'Enter' key works on web? FormWrapper has FormSubmit

App/src/components/Form/FormWrapper.js

Lines 109 to 113 in 6ea4539

	<FormSubmit
	key={formID}
	ref={formContentRef}
	style={StyleSheet.flatten([style, safeAreaPaddingBottomStyle])}
	onSubmit={onSubmit}

And FormSubmit button has keydown handler on web as below

App/src/components/FormSubmit/index.js

Lines 22 to 26 in 6ea4539

	// ENTER is pressed on INPUT or SELECT element, call the submit callback.
	if (tagName === 'INPUT' || tagName === 'SELECT') {
	onSubmit();
	return;
	}

That's why pressing Enter calls onSubmit for web, but doesn't for native

This is the root cause

What changes do you think we should make in order to solve the problem?

This issue happens wherever FormProvider is used. We might need to check them all

In order to solve this issue, we can enable press on enter for status form

1. Add disablePressOnEnter props with default value true to the FormProvider and FormWrapper component
2. Update the above code to

                disablePressOnEnter={props.disablePressOnEnter}

3. Add disablePressOnEnter props here

        // const platform = getPlatform();
        // const isNative = platform === CONST.PLATFORM.IOS || platform === CONST.PLATFORM.ANDROID;
                disablePressOnEnter={!isNative}

This works as expected

Result

31433.mp4

What alternative solutions did you explore? (Optional)

[shubham1206agra]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Entering status message & tapping enter show different behaviour in mWeb and Android

What is the root cause of that problem?

When we tap Enter, it triggers the onSubmit function in the FormSubmit component, but the problem is the listener for Enter is only present in web implementation for some reason.

App/src/components/Form/FormWrapper.js

Lines 109 to 113 in 6ea4539

	<FormSubmit
	key={formID}
	ref={formContentRef}
	style={StyleSheet.flatten([style, safeAreaPaddingBottomStyle])}
	onSubmit={onSubmit}

What changes do you think we should make in order to solve the problem?

We can take advantage of the onSubmitEditing prop that will also provide the event in the case of TextInput prop, and we can implement the onSubmitEditing prop for the custom component we use in InputWrapper. We can then give the submit function inside the implementation of onSubmitEditing in registerInput.

This will take care of both web and native implementation

Complete Explanation

We have 2 ways to go about this

Option A (Nuking the submit logic inside FormSubmit, and using onSubmitEditing prop instead for submit logic handling)

This route will remove logic from

App/src/components/FormSubmit/index.tsx

Lines 12 to 66 in 1f8e365

	const submitForm = (event: KeyboardEvent) => {
	// ENTER is pressed with modifier key or during text composition, do not submit the form
	if (event.shiftKey || event.key !== CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey || isEnterWhileComposition(event)) {
	return;
	}
	const eventTarget = event.target as HTMLElement;
	const tagName = eventTarget?.tagName ?? '';
	// ENTER is pressed on INPUT or SELECT element, call the submit callback.
	if (tagName === 'INPUT' || tagName === 'SELECT') {
	onSubmit();
	return;
	}
	// Pressing Enter on TEXTAREA element adds a new line. When `dataset.submitOnEnter` prop is passed, call the submit callback.
	if (tagName === 'TEXTAREA' && (eventTarget?.dataset?.submitOnEnter ?? 'false') === 'true') {
	event.preventDefault();
	onSubmit();
	return;
	}
	// ENTER is pressed on checkbox element, call the submit callback.
	if (eventTarget?.role === 'checkbox') {
	onSubmit();
	}
	};
	const preventDefaultFormBehavior = (e: SubmitEvent) => e.preventDefault();
	useEffect(() => {
	if (!(ref && 'current' in ref)) {
	return;
	}
	const form = ref.current as HTMLFormElement | null;
	if (!form) {
	return;
	}
	// Prevent the browser from applying its own validation, which affects the email input
	form.setAttribute('novalidate', '');
	form.addEventListener('submit', preventDefaultFormBehavior);
	return () => {
	if (!form) {
	return;
	}
	form.removeEventListener('submit', preventDefaultFormBehavior);
	};
	}, [ref]);

and also onKeyDownCapture={submitForm}

This logic is getting removed as it is spitting image of react-native-web implementation of onSubmitEditing, except for multiline behavior, which will be taken care inside onSubmitEditing implementation described later.

And now instead will use onSubmitEditing prop which will be introduced inside

App/src/components/Form/FormProvider.js

Line 240 in 1f8e365

const registerInput = useCallback(

As registerInput function returns props to the component which we desire to use inside of the component of the Form Inputs.

For adhering to the guidelines of the FORMS.md, we will not pass onSubmitEditing prop when we use multiline or autoGrowHeight prop for TextInput (only exception is submitOnEnter is passed as true). See

App/src/components/TextInput/BaseTextInput/index.tsx

Lines 384 to 386 in 1f8e365

	// FormSubmit Enter key handler does not have access to direct props.
	// `dataset.submitOnEnter` is used to indicate that pressing Enter on this input should call the submit callback.
	dataSet={{submitOnEnter: isMultiline && submitOnEnter}}

(Extra Note - This can be condensed to disableSubmit prop as this can be easily defined on dev side when defining form inputs (not compulsory step))

Pros of this approach -

1. This approach will get rid of extra listener on FormSubmit, which will give some performance based boost on lower end device
2. This will fix the bug when we use StatePicker as form input, and press enter on search text input, it will submit the form which is not ideal here. This happens due to assumption that we define TextInput inside a form as a form element only.
3. This approach will be robust as custom component can use the onSubmitEditing prop as it will be available with all form input element. Examples are provided in [HOLD for payment 2024-03-11] [HOLD for payment 2024-02-20][$1000] Form: Inconsistent text input submitting experience between mWeb and Native  #31433 (comment).

Cons -
The only con will be we have to be extra careful as this can introduced unintentional bugs. I am hopeful that no bugs/ small bugs will arise as this logic is approximately 1.5 years old, and was used during old Form component, which are now being deprecated.

Test Branch - https://github.com/shubham1206agra/App/tree/test-form-sub-a

Option B (Relatively safe option - only using onSubmitEditing prop on native platforms only, and not touching submit logic inside FormSubmit)

This is relatively safe approach from option A, but will lose all the pros of option A.

Test Branch - https://github.com/shubham1206agra/App/tree/test-form-sub

Common to both - We can define returnKeyType to make button common to both platforms (web, and native).

What alternative solutions did you explore? (Optional)

NA

[cubuspl42]

@shubham1206agra Your proposal sounds interesting, but I'm not quite sure if I understand the whole plan. Could you expand on...

We can then give the submit function inside the implementation of onSubmitEditing in registerInput.

...?

[shubham1206agra]

@shubham1206agra Your proposal sounds interesting, but I'm not quite sure if I understand the whole plan. Could you expand on...

We can then give the submit function inside the implementation of onSubmitEditing in registerInput.

...?

See https://reactnative.dev/docs/textinput#onsubmitediting

In

App/src/components/Form/FormProvider.js

Line 202 in 8c909d1

const registerInput = useCallback(

We can give the onSubmitEditing function definition, which submits the form if required since registerInput function returns props to the component which we desire to render there.

For the custom component, we can create a custom onSubmitEditing prop, which gives us flexibility for the future.

@cubuspl42 If you have any specific questions, you can ask them here.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[conorpendergrast]

Looks like we're making our way through proposals here still 👍

[cubuspl42]

@shubham1206agra Do you maybe have a Git branch with your proposed changes? If so, would you share a link?

[shubham1206agra]

I don't have one right now, but I will make one and update you here

[conorpendergrast]

@shubham1206agra Do you have an ETA on the branch with the proposed changes? Thanks!

[cubuspl42]

@shubham1206agra The solution depending on onSubmitEditing sounds good, but I'm interested whether it was proven to work in practice. That's why I asked abut the Git branch. Thanks!

[MonilBhavsar]

We're awaiting checklist and payment here
Bumping to weekly

[MonilBhavsar]

@cubuspl42 can you please finish the checklist so we can pay and close this issue

[cubuspl42]

• The PR that introduced the bug has been identified. Link to the PR:
  • It seems that the bug was present in the relevant components since they were created
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • N/A
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • No need for additional discussion
• Determine if we should create a regression test for this bug.
  • Up to the QA team
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • On Android/iOS mWeb/Native, using on-screen keyboard:
    • Go to Settings > Profile > Personal Details > Legal Name
    • Enter any text to "Legal first name", tap "Legal last name"
    • Enter any text to "Legal last name", don't close the on-screen keyboard
    • Verify that the primary button in the bottom-right corner of the on-screen keyboard looks the same between mWeb and Native ("➞" / "Go")
    • Tap that bottom
    • Verify that the navigation behavior is the same between mWeb and Native (the form is accepted and you're navigated to the previous page)

[mallenexpensify]

Contributor: @cubuspl42 paid $1000 via Upwork.
Contributor+: @shubham1206agra paid $1000 via Upwork.

TestRail GH
https://github.com/Expensify/Expensify/issues/372511

Thanks!

[shubham1206agra]

@mallenexpensify Is there a reason I am not being paid here?

[cubuspl42]

It must be a misunderstanding, maybe because we're both from the C+ team?

@shubham1206agra was in the Contributor (PR author) role.

[mallenexpensify]

@shubham1206agra can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~0172c3ea6223afce29

Apologies for the oversight.

[shubham1206agra]

Done

[mallenexpensify]

All paid! Sorry again for missing ya @shubham1206agra
Payment comment above is updated
#31433 (comment)

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.46-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix form default behavior #36401

If no regressions arise, payment will be issued on 2024-03-11. 🎊

For reference, here are some details about the assignees on this issue:

• @cubuspl42 requires payment automatic offer (Reviewer)
• @shubham1206agra requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@cubuspl42 / @shubham1206agra] The PR that introduced the bug has been identified. Link to the PR:
• [@cubuspl42 / @shubham1206agra] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@cubuspl42 / @shubham1206agra] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@cubuspl42 / @shubham1206agra] Determine if we should create a regression test for this bug.
• [@cubuspl42 / @shubham1206agra] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: