[HOLD for payment 2023-11-09] [HOLD for #24198] [$500] Pressing Enter on the state search page submits the form instead of only selecting the state.

[kavimuru]

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

---

Action Performed:

1-Go to settings -> workspaces -> your workspace -> add back 2- account -> connect manually.
3- Fill in the data.
4- Click on the "Connect" button.
5- Open the state search page.
6- Use the arrow keys to navigate to a state.
7- Press Enter.

Expected Result:

Pressing Enter should only select the state and return the user to the form.

Actual Result:

Pressing Enter selects the state and submits the form.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.59-1
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
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-08-21.at.2.52.27.PM.mov

Recording.1542.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ahmdshrif
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692629203062389

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bf4700c16d6a333f
• Upwork Job ID: 1699765868129120256
• Last Price Increase: 2023-09-21

[melvin-bot]

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

[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

[akinwale]

Proposal

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

Pressing Enter on the state search page submits the form instead of only selecting the state.

What is the root cause of that problem?

The form submit button in the CompanyStep component is receiving key press events even when the StatePicker modal is visible.

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

Add props to the StatePicker and AddressForm modules to handle show / hide events for the state picker modal, and then set the a flag using the state to indicate whether or not the form should be submitted.

We should also do this for the RequestorStep page, forwarding the onShowStatePicker / onHideStatePicker props through IdentityForm (since this contains the AddressForm).

The solution can be achieved with the following steps:

1. Add an onShow prop to StateSelectorModal, and set it to the onModalShow prop for the root Modal.

onShow: PropTypes.func, // prop type
...
onModalShow={onShow}

2. Add onShow and onClose props to the StatePicker component and update the prop types and default props accordingly. Also update the hidePickerModal handler.

<StateSelectorModal
    ...
    onShow={onShow}
/>
...
const hidePickerModal = () => {
    if (onClose) {
        onClose();
    }
    setIsPickerVisible(false);
};

3. Handle the onShow and onHide props for the StatePicker in the AddressForm component. Also add the corresponding prop types and default props (onShowStatePicker / onHideStatePicker).

<StatePicker
    ...
    onShow={props.onShowStatePicker}
    onHide={props.onHideStatePicker}
    ...
/>

4. Add onShow and onHide props for the StatePicker in the CompanyStep component, as well as the handlers.

const [shouldDisableFormSubmit, setShouldDisableFormSubmit] = useState(false);
const showStatePicker = () => {
    setShouldDisableFormSubmit(true);
}
const hideStatePicker = () => {
    setShouldDisableFormSubmit(false);
}
...
<StatePicker
    ...
    onShow={showStatePicker}
    onClose={hideStatePicker}
/>

5. Update the submit handler in the CompanyStep component.

const submit = (values) => {
      if (shouldDisableFormSubmit) {
          return;
      }
    // the rest of the submit handler implementation
}

What alternative solutions did you explore?

We could also consider making the button disabled when the state picker is visible, but this will require updating the Form and FormAlertWithSubmitButton components by adding the required prop and its prop type.

[tamdao]

Proposal

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

Pressing Enter on the state search page submits the form instead of only selecting the state

What is the root cause of that problem?

We have logic onKeyDownCapture to call onSubmit function when user type Enter key, but didn't check the input is belong form or not. So, any input event always calls onSubmit function if user type Enter and happen for all form has input not belong from.

In this case, the search input is not belonged to form, it uses for search state

App/src/components/FormSubmit/index.js

Lines 14 to 38 in a644050

	const submitForm = (event) => {
	// 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 tagName = lodashGet(event, 'target.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' && lodashGet(event, 'target.dataset.submitOnEnter', 'false') === 'true') {
	onSubmit();
	return;
	}
	// ENTER is pressed on checkbox element, call the submit callback.
	if (lodashGet(event, 'target.role') === 'checkbox') {
	onSubmit();
	}
	};

What alternative solutions did you explore?

Update childrenWrapperWithProps in Form component to add dataSet when call React.cloneElement line 294

App/src/components/Form.js

Lines 219 to 357 in 1b63c76

	const childrenWrapperWithProps = useCallback(
	(childNodes) => {
	const childrenElements = React.Children.map(childNodes, (child) => {
	// Just render the child if it is not a valid React element, e.g. text within a <Text> component
	if (!React.isValidElement(child)) {
	return child;
	}
	// Depth first traversal of the render tree as the input element is likely to be the last node
	if (child.props.children) {
	return React.cloneElement(child, {
	children: childrenWrapperWithProps(child.props.children),
	});
	}
	// Look for any inputs nested in a custom component, e.g AddressForm or IdentityForm
	if (_.isFunction(child.type)) {
	const childNode = new child.type(child.props);
	// If the custom component has a render method, use it to get the nested children
	const nestedChildren = _.isFunction(childNode.render) ? childNode.render() : childNode;
	// Render the custom component if it's a valid React element
	// If the custom component has nested children, Loop over them and supply From props
	if (React.isValidElement(nestedChildren) || lodashGet(nestedChildren, 'props.children')) {
	return childrenWrapperWithProps(nestedChildren);
	}
	// Just render the child if it's custom component not a valid React element, or if it hasn't children
	return child;
	}
	// We check if the child has the inputID prop.
	// We don't want to pass form props to non form components, e.g. View, Text, etc
	if (!child.props.inputID) {
	return child;
	}
	// We clone the child passing down all form props
	const inputID = child.props.inputID;
	let defaultValue;
	// We need to make sure that checkboxes have correct
	// value assigned from the list of draft values
	// https://github.com/Expensify/App/issues/16885#issuecomment-1520846065
	if (_.isBoolean(props.draftValues[inputID])) {
	defaultValue = props.draftValues[inputID];
	} else {
	defaultValue = props.draftValues[inputID] || child.props.defaultValue;
	}
	// We want to initialize the input value if it's undefined
	if (_.isUndefined(inputValues[inputID])) {
	inputValues[inputID] = defaultValue || '';
	}
	// We force the form to set the input value from the defaultValue props if there is a saved valid value
	if (child.props.shouldUseDefaultValue) {
	inputValues[inputID] = child.props.defaultValue;
	}
	if (!_.isUndefined(child.props.value)) {
	inputValues[inputID] = child.props.value;
	}
	const errorFields = lodashGet(props.formState, 'errorFields', {});
	const fieldErrorMessage =
	_.chain(errorFields[inputID])
	.keys()
	.sortBy()
	.reverse()
	.map((key) => errorFields[inputID][key])
	.first()
	.value() || '';
	return React.cloneElement(child, {
	ref: (node) => {
	inputRefs.current[inputID] = node;
	const {ref} = child;
	if (_.isFunction(ref)) {
	ref(node);
	}
	},
	value: inputValues[inputID],
	// As the text input is controlled, we never set the defaultValue prop
	// as this is already happening by the value prop.
	defaultValue: undefined,
	errorText: errors[inputID] || fieldErrorMessage,
	onBlur: (event) => {
	// Only run validation when user proactively blurs the input.
	if (Visibility.isVisible() && Visibility.hasFocus()) {
	// We delay the validation in order to prevent Checkbox loss of focus when
	// the user are focusing a TextInput and proceeds to toggle a CheckBox in
	// web and mobile web platforms.
	setTimeout(() => {
	setTouchedInput(inputID);
	if (props.shouldValidateOnBlur) {
	onValidate(inputValues);
	}
	}, 200);
	}
	if (_.isFunction(child.props.onBlur)) {
	child.props.onBlur(event);
	}
	},
	onTouched: () => {
	setTouchedInput(inputID);
	},
	onInputChange: (value, key) => {
	const inputKey = key || inputID;
	setInputValues((prevState) => {
	const newState = {
	...prevState,
	[inputKey]: value,
	};
	if (props.shouldValidateOnChange) {
	onValidate(newState);
	}
	return newState;
	});
	if (child.props.shouldSaveDraft) {
	FormActions.setDraftValues(props.formID, {[inputKey]: value});
	}
	if (child.props.onValueChange) {
	child.props.onValueChange(value, inputKey);
	}
	},
	});
	});
	return childrenElements;
	},
	[errors, inputRefs, inputValues, onValidate, props.draftValues, props.formID, props.formState, setTouchedInput, props.shouldValidateOnBlur, props.shouldValidateOnChange],
	);

like this:

dataSet: {formInputId: inputID},

Update BaseTextInput to set dataSet line 350

App/src/components/TextInput/BaseTextInput.js

Line 350 in 1b63c76

dataSet={{submitOnEnter: isMultiline && props.submitOnEnter}}

dataSet={{...props.dataSet, submitOnEnter: isMultiline && props.submitOnEnter}}

Update AddressSearch to set props.dataSet into textInputProps

App/src/components/AddressSearch/index.js

Lines 301 to 343 in 1b63c76

	textInputProps={{
	InputComp: TextInput,
	ref: (node) => {
	if (!props.innerRef) {
	return;
	}
	if (_.isFunction(props.innerRef)) {
	props.innerRef(node);
	return;
	}
	// eslint-disable-next-line no-param-reassign
	props.innerRef.current = node;
	},
	label: props.label,
	containerStyles: props.containerStyles,
	errorText: props.errorText,
	hint: displayListViewBorder ? undefined : props.hint,
	value: props.value,
	defaultValue: props.defaultValue,
	inputID: props.inputID,
	shouldSaveDraft: props.shouldSaveDraft,
	onBlur: (event) => {
	resetDisplayListViewBorderOnBlur(setDisplayListViewBorder, event, containerRef);
	props.onBlur();
	},
	autoComplete: 'off',
	onInputChange: (text) => {
	if (props.inputID) {
	props.onInputChange(text);
	} else {
	props.onInputChange({street: text});
	}
	// If the text is empty, we set displayListViewBorder to false to prevent UI flickering
	if (_.isEmpty(text)) {
	setDisplayListViewBorder(false);
	}
	},
	maxLength: props.maxInputLength,
	spellCheck: false,
	}}

Update submitForm function in FromSubmit component to check the formInputId dataset

App/src/components/FormSubmit/index.js

Lines 14 to 38 in 1b63c76

	const submitForm = (event) => {
	// 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 tagName = lodashGet(event, 'target.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' && lodashGet(event, 'target.dataset.submitOnEnter', 'false') === 'true') {
	onSubmit();
	return;
	}
	// ENTER is pressed on checkbox element, call the submit callback.
	if (lodashGet(event, 'target.role') === 'checkbox') {
	onSubmit();
	}
	};

like this

...

if (event.shiftKey || event.key !== CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey || isEnterWhileComposition(event)) { 
    return; 
} 

if (!lodashGet(event, 'target.dataset.formInputId')) {
    return;
}
...

[melvin-bot]

@sonialiap Huh... This is 4 days overdue. Who can take care of this?

[sonialiap]

Triaging to external ⏭️

[melvin-bot]

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

[melvin-bot]

Current assignee @sonialiap is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[ramachoon]

There should be listener what listens button actions.
When it listens Enter action of keyboard, it should check and validate, and next process will be submitting.
If you give me choice, I will fix it in this way.

[melvin-bot]

📣 @edward-potter818! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[sicarius97]

Proposal

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

Pressing enter while in state selector submits the form rather than selecting the state of choice

What is the root cause of that problem?

The state select dropdown doesn't lock the proper focus when open so actions that should occur inside the focus context of the dropdown instead happen outside of it, as is default behavior for forms, but not very good by accessibility standards

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

While some of the previously proposed solutions mentioned capturing the event and altering its logic to prevent the default behavior, I think it was being altered at the wrong level in the previous proposals in a way that was not as composable. There is already a component for capturing the focus of the arrow keys here:

App/src/components/ArrowKeyFocusManager.js

Line 35 in b119f9a

class ArrowKeyFocusManager extends Component {

Which is used by multiple selects to capture the focus of the arrow keys. This component can be extended to also capture the focus of the 'ENTER' key stroke and would fix this problem for not only this selection box, but any future selection boxes as well, making it a more extensible solution than previous proposals

[melvin-bot]

📣 @sicarius97! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[sicarius97]

Contributor details
Your Expensify account email: icroatt97@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01dbdc4055191721a0

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[melvin-bot]

@sonialiap, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@sonialiap, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[sonialiap]

@sobitneupane what do you think of the existing proposal?

[sobitneupane]

Thanks for the proposal everyone.

This component can be extended to also capture the focus of the 'ENTER' key stroke

@sicarius97 Can you please share more details on proposal. How are you planning to extend the component to manage ENTER key?

[melvin-bot]

@sonialiap, @sobitneupane Eep! 4 days overdue now. Issues have feelings too...

[sonialiap]

On hold for #24198

[sonialiap]

Still on hold for #24198

That issue isn't super close to being resolved and I'll be OOO until Oct 22 so I'm going to update this issue to "weekly" for now.

[sonialiap]

Still on hold for #24198

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• fixed auto submit form when enter key pressed in selection modals #30300

If no regressions arise, payment will be issued on 2023-11-09. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @sobitneupane does not require payment (Eligible for Manual Requests)
• @ahmdshrif requires payment

[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:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
• [@sobitneupane] 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:
• [@sobitneupane] 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:
• [@sobitneupane] Determine if we should create a regression test for this bug.
• [@sobitneupane] 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.
• [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sonialiap]

This is Dupe. Resolved by #24198