[HOLD for payment 2022-05-20] $500 - Pressing enter/return on request call page is calling onPress function - reported by @Tushu17

[mvtglobally]

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. Open Workspace then general settings
2. Go to Get assitance page
3. Click on Request a setup Call
4. Select any field and press Enter/Return

Expected Result:

Pressing Enter/return should press the Call me button

Actual Result:

Workspace general settings Save button is getting pressed on enter.

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• Desktop App
• Mobile Web

Version Number: 1.1.37-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:
Issue reported by: @Tushu17
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1643573796542919

View all open jobs on GitHub

post here

[MelvinBot]

Triggered auto assignment to @timszot (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Tushu17]

Proposal

We need to pass pressOnEnter in Button

App/src/pages/RequestCallPage.js

Lines 352 to 356 in 78d6e68

	<Button
	success
	onPress={this.onSubmit}
	style={[styles.w100]}
	text={this.props.translate('requestCallPage.callMe')}

[MelvinBot]

Triggered auto assignment to @jboniface (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[jboniface]

post here

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to @thienlnam (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[parasharrajat]

This creates an interesting case here. Pressing enter run an action on the backgrounded Page. This could be a problem in many places.

So with regard to the change that @Tushu17 has suggested. I think it would be great if pressOnEnter only acts when the page is active/focused. Thoughts @rushatgabhane ?

[rushatgabhane]

@parasharrajat I had the same thinking but the action on backgrounded page isn't run if we go with @Tushu17's proposal.

So I'm suggesting that we create a seperate issue for fixing pressOnEnter. Is this okay?
IIRC, Matt had brought this up a few months ago, but it was decided to do nothing 🤷‍♂️

1.New.Expensify.-.Google.Chrome.2022-02-08.23-48-36.mp4

[rushatgabhane]

This could be a problem in many places.
pressOnEnter should only act when the page is active/focused

Thanks for bringing this up. I agree that we should fix this because it leads to unexpected behavior like in the video below, and ofc this issue's actual result.

Video - The button shouldn't have been pressed for the underlying IOU page.

1.New.Expensify.-.Google.Chrome.2022-02-08.23-51-56.mp4

[thienlnam]

Agreed, a fix should encapsulate the problem here which is that

Pressing enter run an action on the backgrounded Page

[rushatgabhane]

Okay, that works too.

@Tushu17 feel free to post an updated proposal which prevents pressOnEnter for backgrounded pages. Thanks!

[rushatgabhane]

waiting for proposals

[Varanu]

Proposal

I would suggest a method which uses React SyntheticEvent onKeyPress={this.handeKeyPress} and pass it as a prop to the <TextInput/> and <FullNameInputRow/> components. The function will not do anything if a key, other than Enter/Return is pressed and it will trigger the onSubmit() function which is used for Call Me button.

RequestCallPage.js file

In the RequestCallPage.js file add onKeyPress={this.handeKeyPress} to both <TextInput/> components used for Phone number and Extension(Optional) and also to the <FullNameInputRow/> like bellow

<FullNameInputRow
                                firstName={this.state.firstName}
                                firstNameError={PersonalDetails.getMaxCharacterError(this.state.hasFirstNameError)}
                                lastName={this.state.lastName}
                                lastNameError={PersonalDetails.getMaxCharacterError(this.state.hasLastNameError)}
                                onChangeFirstName={firstName => this.setState({firstName})}
                                onChangeLastName={lastName => this.setState({lastName})}
                                style={[styles.mv4]}
+                               onKeyPress={this.handleKeyPress}
                            />

<TextInput
                                        label={this.props.translate('common.phoneNumber')}
                                        autoCompleteType="off"
                                        autoCorrect={false}
                                        value={this.state.phoneNumber}
                                        placeholder="2109400803"
                                        errorText={this.state.phoneNumberError}
                                        onBlur={this.validatePhoneInput}
+                                       onKeyPress={this.handleKeyPress}
                                        onChangeText={phoneNumber => this.setState({phoneNumber})}
                                    />

The handleKeyPress needs to be binded in the constructor

this.onSubmit = this.onSubmit.bind(this);
+ this.handleKeyPress = this.handleKeyPress.bind(this);
this.getPhoneNumber = this.getPhoneNumber.bind(this);

In the handleKeyPress(e) function the e.preventDefault() will make the input remain in focus when an error is returned, for example, and then calls the this.onSubmit() to process the submit request. Also it will check if a key, other than Enter is pressed and return nothing.

handleKeyPress(e) {
        if (e.key !== 'Enter') {
            return;
        }
        e.preventDefault();
        this.onSubmit();
    }

FullNameInputRow.js file

Lastly, in the FullNameInputRow.js file, the function onKeyPress received as a prop will be added to both <TextInput/> components used for First Name and Last Name

 /** Called when a key is pressed */
    onKeyPress: PropTypes.func,

<TextInput
                    label={props.translate('common.lastName')}
                    value={props.lastName}
                    errorText={props.lastNameError}
                    onChangeText={props.onChangeLastName}
                    placeholder={props.translate('profilePage.doe')}
+                   onKeyPress={props.onKeyPress}
                />

Because this component is also used in the ProfilePage.js the onKeyPress will have the default value null.

const defaultProps = {
+   onKeyPress: null,
    firstName: '',

Thank you for reading! :)

[kidroca]

Since this (WorkspaceSettingsPage) is a screen component it receives relevant navigation context

You can just change this

App/src/pages/workspace/WorkspaceSettingsPage.js

Lines 127 to 135 in 50496a0

	<FixedFooter style={[styles.w100]}>
	<Button
	success
	isLoading={this.props.policy.isPolicyUpdating}
	text={this.props.translate('workspace.editor.save')}
	onPress={this.submit}
	pressOnEnter
	/>
	</FixedFooter>

to:

<Button
    success
    pressOnEnter
    isLoading={this.props.policy.isPolicyUpdating}
    isDisabled={!this.props.navigation.isFocused()} // add this
    text={this.props.translate('workspace.editor.save')}
    onPress={this.submit}
/>

Or:

<Button
    success
    isLoading={this.props.policy.isPolicyUpdating}
    text={this.props.translate('workspace.editor.save')}
    onPress={this.submit}
    pressOnEnter={this.props.navigation.isFocused()}
/>

If the handling of the pressOnEnter prop is modified a bit - moved inside the event handler here:

App/src/components/Button.js

Lines 119 to 130 in 50496a0

	if (!this.props.pressOnEnter) {
	return;
	}
	const shortcutConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
	// Setup and attach keypress handler for pressing the button with Enter key
	this.unsubscribe = KeyboardShortcut.subscribe(shortcutConfig.shortcutKey, (e) => {
	if (this.props.isDisabled || this.props.isLoading || (e && e.target.nodeName === 'TEXTAREA')) {
	return;
	}
	this.props.onPress();

[kidroca]

Or maybe it would be best to add the check here:

App/src/pages/workspace/WorkspaceSettingsPage.js

Lines 92 to 95 in 50496a0

	submit() {
	if (this.props.policy.isPolicyUpdating || !this.validate()) {
	return;
	}

 submit() { 
     if (this.props.policy.isPolicyUpdating || !this.props.navigation.isFocused() || !this.validate()) { 
         return; 
     } 

This way isFocused() is evaluated just in time
(Otherwise it depends the component re-rendering, so that isFocused() is called and re-evaluated)

[parasharrajat]

Good points overall. But the motive behind doing that change was to cover all pages. This is a generic issue so I thought of handling it everywhere I have another idea.

As we changed the keyboard shortcut implementation now, we can just stop the even propagation at the Button handler. Then it will not propagate to other elements.

We are running the handler in the capture phase, it won't block other immediate targets from receiving it. All the local event handlers are attached in the bubble phase.

[kidroca]

Good points overall. But the motive behind doing that change was to cover all pages. This is a generic issue so I thought of handling it everywhere I have another idea.

Ok so it seems it's never desired to have 2 or more buttons with the pressOnEnter prop
Or if there are more than one, only the last button that appeared should be triggered on enter
Is that correct?

As we changed the keyboard shortcut implementation now, we can just stop the even propagation at the Button handler. Then it will not propagate to other elements.

I don't understand how that would happen
The bug here is happening because the event handler of the "Save" button from the previous page is triggered first
And it's the one stopping propagation - the "Enter" event is not triggered at all on the "Request a setup Call"

Events are triggered in order of subscription, for this to work they have to be iterated in reverse order
Is that what you're proposing?

[parasharrajat]

Is that correct?

I think yes. pressOnEnter is meant for CTA. I guess there would be only one per page.

Events are triggered in order of subscription, for this to work they have to be iterated in reverse order
Is that what you're proposing?

Oh yeah, didn't think of this. I was just seen at an alternative. I guess this will not work. Let me see what else is possible without refactoring too much code.

[parasharrajat]

I will soon update you here on the status.

[parasharrajat]

Ok, I tried to think of a solution that works with all the buttons.

• If we want to apply the logic without touching those pages, then we will have to go with my old PR. To remove the regression, I will create a HOC then will catch the error and return true for isFocused when NavigationContext is not available.
• Other solution would be use

<Button
    success
    isLoading={this.props.policy.isPolicyUpdating}
    text={this.props.translate('workspace.editor.save')}
    onPress={this.submit}
    pressOnEnter={this.props.navigation.isFocused()}
/>

approach to applying the solution to all pages. But it will require refactoring of all pages. Not recommended, we still don't know if this will be a problem in the future. Only if there are stacked pages and when the topmost page does not have pressOnEnter, will show this issue.

• Lastly, I can just add the pressOnEnter on the RequestCallPage and wrap up.

cc: @thienlnam

[mdneyazahmad]

@parasharrajat Do you think it is a good approach?

We might not need to explicitly register a global event listener for Enter key. TextInput already has onSubmitEditing prop which will be called when user presses Enter.

We have some more custom form component eg. Checkbox. We can pass a props like onSubmitEditing say onEnterPress. The component will internally listen for Enter and calls onEnterPress. We have to do the same thing to other form element if we have.

TextInput does the same thing when calling onSubmitEditing. However, it stops the event propagation and prevents its default behaviour or else we can do this instead on web.

<View accessibilityRole="form" onSubmit={SubmitFunc}>
    // here goes form element
</View>

https://github.com/necolas/react-native-web/blob/dc9ecfbc84c7e6cbf0c36c537ac1b0a86b7e43c1/packages/react-native-web/src/exports/TextInput/index.js#L260-L292

[parasharrajat]

Hmm, @mdneyazahmad. Seems like too much work for the issue.

[kidroca]

This pressOnEnter seems to be an entirely web/desktop thing as KeyboardShortcut.subscribe does nothing on native

If it's intended to work on mobile then yes onSubmitEditing should be used. Then if you press "Enter/Return" on the onscreen keyboard the form would be submitted (though this is usually enabled only on the last field of the form, and "Return" moves the focus to the next field

So if this is indeed a web/desktop only behavior the suggestion above would submit only the form that is currently focused:

<View accessibilityRole="form" onSubmit={SubmitFunc}>
    // here goes form element
</View>

• the <Button> component would need to have type="submit"

---

On other apps that I've worked I have a <Form> component with an onSubmit prop that intercepts submit events and triggers the callback

• <Form> accepts the name of the field that triggers submit on enter (or fallbacks to the last field if not provided)
• <Form> triggers the submit handler if the submit button is pressed
• it also takes care of tabbing to the next field from the software keyboard

---

One thing to note, this is not working on native and it might be intended or not, but it's not clear that pressOnEnter works only on web/desktop

    /** Call the onPress function when Enter key is pressed */
    pressOnEnter: PropTypes.bool,

I would not expect that pressing enter/return would submit a form unless I have currently focused a field inside it

[parasharrajat]

Good points, It could be a extension to our Form component and soon we will refactor all of our forms to use that.

[thienlnam]

If we want to apply the logic without touching those pages, then we will have to go with my old PR. To remove the regression, I will create a HOC then will catch the error and return true for isFocused when NavigationContext is not available.

This will likely work. I like the idea of checking if the current screen is focused before triggering the pressOnEnter. But when/why is the NavigationContext not available?

Only if there are stacked pages and when the topmost page does not have pressOnEnter, will show this issue.

I also agree we should solve this main issue instead of updating each page so this will be solved

[parasharrajat]

But when/why is the NavigationContext not available?

when we use the button in the components that are out of React navigator. 99% of the pages are used in the navigator except for the Error Page. It has two buttons that crash the app.

[thienlnam]

Gotcha, let's go ahead with that solution then @parasharrajat

[parasharrajat]

Tried one implementation and it has few problems which I am trying to solve.

[parasharrajat]

Working it now.

[melvin-bot]

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

• Block activation of primary buttons on background screens #8802

If no regressions arise, payment will be issued on 2022-05-20. 🎊

[jboniface]

@Tushu17 and @parasharrajat the old job post died, let's use this one https://www.upwork.com/jobs/~01b7767e126c5ee6ae

[jboniface]

paid