Closed kbecciv closed 9 months ago
Triggered auto assignment to @johncschuster (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Platforms
in OP are ✅)Job added to Upwork: https://www.upwork.com/jobs/~011eacb8f4d46da0d6
Current assignee @johncschuster is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh (External
)
Picker (Selection fields) are still enabled after the Create Room button is clicked, which allows the user to change the values.
The root cause of this problem is on the WorkspaceNewRoomPage.js
file:
submit(values) {
const policyMembers = _.map(_.keys(this.props.allPolicyMembers[`${ONYXKEYS.COLLECTION.POLICY_MEMBERS}${values.policyID}`]), (accountID) => Number(accountID));
Report.addPolicyReport(values.policyID, values.roomName, values.visibility, policyMembers);
}
When the submit function is called, this doesn't block the Pickers in any way, so while the action is fired, they are still enabled. We also need to make the actions after the submit asynchronously, so that it doesn't block our state from updating.
We should add an extra state on the component like that:
constructor(props) {
super(props);
this.state = {
visibilityDescription: this.props.translate('newRoomPage.restrictedDescription'),
isFormSubmitting: false, // New property for form submission state
};
On the submit action, we should add the following:
submit(values) {
const policyMembers = _.map(_.keys(this.props.allPolicyMembers[`${ONYXKEYS.COLLECTION.POLICY_MEMBERS}${values.policyID}`]), (accountID) => Number(accountID));
this.setState({isFormSubmitting: true});
setTimeout(() => {
const policyMembers = _.map(_.keys(this.props.allPolicyMembers[`${ONYXKEYS.COLLECTION.POLICY_MEMBERS}${values.policyID}`]), (accountID) => Number(accountID));
Report.addPolicyReport(values.policyID, values.roomName, values.visibility, policyMembers);
}, 0);
}
// We use SetTimeout with 0 delay, otherwise our component won't be updated to disable the fields.
*EDIT: I initially used the isDisabled
prop on the Picker
, but I noticed that this renders it into a Text field and it shows the value instead of the label, which is something we don't want for the users. My new approach is to disable the Pickers via their View
parents
OLD IMPLEMENTATION:
<Picker
inputID="policyID"
label={this.props.translate('workspace.common.workspace')}
placeholder={{value: '', label: this.props.translate('newRoomPage.selectAWorkspace')}}
items={workspaceOptions}
isDisabled={this.state.isFormSubmitting}
/>
NEW IMPLEMENTATION:
<View style={styles.mb5} pointerEvents={this.state.isFormSubmitting ? 'none' : 'auto'}>
<Picker
inputID="policyID"
label={this.props.translate('workspace.common.workspace')}
placeholder={{value: '', label: this.props.translate('newRoomPage.selectAWorkspace')}}
items={workspaceOptions}
/>
</View>
This one disables the Select fields without changing the looks.
This way, the selection fields / inputs will be disabled while the room is being created
Not overdue, Melvin. We're just getting proposals now.
@fedirjh, what do you think of the above proposal?
@Thanos30 I think the root cause is that we don't disable the Form input's edition after submission. Ideally, we should fix the Form and block any interaction after submission.
Regarding the solution, we should avoid using setTimeout
as a workaround.
@fedirjh I could look into the Form inputs' disabling option if you prefer it this way.
The reason I am using the setTimeout, is because the Form submission is triggering an immediate navigation, and our issue is happening in between the navigation process. Within that small time space, the state update on the Form component isn't firing, since we are on the navigating process. That's why we use the setTimeout in order to do that on the background.
Feel free to test on your own environment if possible so you can check this first-hand.
Thanks for the feedback
Looks like something related to react-navigation
may have been mentioned in this issue discussion.
As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js
files should not be accepted.
Feel free to drop a note in #expensify-open-source with any questions.
I could look into the Form inputs' disabling option if you prefer it this way.`
We should fix it globally, it may occur on other forms as well, so it would be ideal to address the root cause.
the state update on the Form component isn't firing
Maybe we should investigate that further, this is feels tricky to me. Not sure , but we can switch the order of execution, update state then submit the form.
We should fix it globally, it may occur on other forms as well, so it would be ideal to address the root cause.
I checked, I can follow the same logic inside the Form component so that it works like that everywhere.
Maybe we should investigate that further, this is feels tricky to me. Not sure , but we can switch the order of execution, update state then submit the form.
I tried that, as I mentioned above, the state update is asynchronous, so it still doesn't trigger. I understand that it sounds a bit tricky, in real life scenarios the Navigation will probably never take long enough for the user to be able to click on the selection fields again.
Debouncing the creation of the room and the navigation with a delay of 0 will simply run the actions on the background asynchronously, which allows us to remain on the same state and be able to update it. If you think about it, in most forms, there is an asynchronous function following up, so it's not that strange.
@johncschuster, @fedirjh Whoops! This issue is 2 days overdue. Let's get this updated quick!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@fedirjh what are your thoughts on @Thanos30's comment above?
@johncschuster I think we should wait for better proposal , actually we don't allow setTimeout
solution and it seems like @Thanos30 has not updated his proposal to address that.
@fedirjh setTimeout
will be used with the _.debounce()
function which is broadly used throughout our codebase, but I do understand that it is used for different cases.
I couldn't figure out a way around this without debouncing the call so that it runs on the background (asynchronously) and not block the state update. If you want me to update my proposal to use the _debounce()
method please let me know. If you dislike this approach of course it's okay, hopefully someone can come up with a different approach.
Thank you for the feedback
Not overdue, Melvin. We're still waiting on proposals
Hey @fedirjh I tried testing, and it seems like the problem is only with pickers. Is it okay if the solution focuses solely on pickers?
@Nodebrute Let me know if you have a proposal for that.
@johncschuster @fedirjh this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
Waiting for proposals.
@fedirjh I believe that handling the navigation asynchronously (using the debounce) instead of synchronously is a good solution, as it won't block the state update. Do you disagree with that?
@Thanos30 I believe we can avoid that, for class components (WorkspaceNewRoomPage
), I think we can use state callbacks to achieve the same result. For functional component (Form
) I think we can use state + hook ( This should be tested ). I would like to explore both solutions.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
No satisfactory proposals yet.
Upwork job price has been updated to $2000
@fedirjh do I need a high traffic account to test this. I am unable to access the new room option on my FAB menu. Kindly advise on steps needed to replicate
@kmwamasali You just need to enable rooms inside Permissions.js : https://github.com/Expensify/App/blob/2ce65e33faea0b058d894bc8b8282d97c64f8fd4/src/libs/Permissions.js#L78
Android - Workspace is still selectable even after 'Create room' button has been pressed
Workspace is still selectable.
1. create a useState variable
const [formSub, setFormSub] = useState(false);
2. Change below line to https://github.com/Expensify/App/blob/9b205fbb26aeafea690b62412d00fd1aa5529bde/src/components/Form.js#L341
setFormSub(true)
3. Pass formSub as a dependency here https://github.com/Expensify/App/blob/9b205fbb26aeafea690b62412d00fd1aa5529bde/src/components/Form.js#L373
4. Wrap children in View
<View style={{pointerEvents: formSub ? 'none':'auto'}}>
{childrenWrapperWithProps(_.isFunction(children) ? children({inputValues}) : children)}
</View>
5. create a useEffect Hook
useEffect(()=>{
if (formSub) {
const timeout = setTimeout(() => {
submit();
}, 0);
return () => {
clearTimeout(timeout);
};
}
},[formSub])
6.Here we need to setFormSub to false in case data is not validated
setFormSub(false)
When formSub is changed, we need to rerender scrollViewContent and submit. Here, we need to ensure that scrollViewContent is rerendered first. I tried using two states, but React batches the states before rerendering, and it didn't work as intended.
We need to dismiss keyboard if it's open before submission otherwise user will be able to to enter values in input even though point events will be set to none.
Instead os using timeout we can use other approaches setImmediate, Promises
Workspace is still selectable even after 'Create room' button has been pressed
Form elements can still be clicked during the submission event, with only the submit button being disabled to avoid duplicate submissions. https://github.com/Expensify/App/blob/b9508b616f3a7229365c8bd7280fd1e34cfd2f9b/src/components/Form.js#L177-L181
By wrapping all form elements within <fieldset disabled>
all inputs can be blocked during submission
see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset#disabled_fieldset
We can use the already existing isLoading
prop to handle enabling/diabling the input fields
https://github.com/Expensify/App/blob/b9508b616f3a7229365c8bd7280fd1e34cfd2f9b/src/components/Form.js#L40-L43
Because introducing a higher level DOM element in the form may or maynot (pending testing) introduce some issues from event bubbling concerning the input elements.
We can optionally pass a disabled
attribute to the input elements which is also dependent on the form isLoading
prop to handle enabling/diabling. By passing the field to the childrenWrapperWithProps
which iterates over all input elements in the form
https://github.com/Expensify/App/blob/b9508b616f3a7229365c8bd7280fd1e34cfd2f9b/src/components/Form.js#L195-L204
@fedirjh what do you think of the proposals above?
@johncschuster @fedirjh this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
Reviewing shortly.
@fedirjh my proposal targets all forms so inputs will be generally disabled when the submission is being processed
@Nodebrute Can we avoid using setTimeout ?
@fedirjh Yes. We can achieve this without using setTimeout. We just need to ensure that scrollViewContent is re-rendered before submitting the form.
@kmwamasali I don’t think we have fieldset
component in react-native, that’s only supported for web.
Yes. We can achieve this without using setTimeout.
@Nodebrute Why do we need setTimeout ?
We just need to ensure that scrollViewContent is re-rendered before submitting the form.
How to achieve that ?
@fedirjh If I may add here, I researched this a bit, and it is not bad practice to handle the navigation asynchronously.
The reason we are using setTimeout
is because otherwise, the navigation blocks the state update, which blocks us from blocking the form fields. I explained it in more details above.
Thank you for the feedback 🙏
@fedirjh yes, it would require an npm package to work.
The input fields can still be passed the disabled field as I set in the alternative proposal. And by using the same loading state that blocks a second submission the input fields would also be disabled directly
@Nodebrute Why do we need setTimeout ?
I'll explain, correct me If I am wrong.
So we have a state variable
const [formSub, setFormSub] = useState(false);
Here, I want to set formSub to true before submitting the form, which I achieve by using a useEffect hook as mentioned above.
Now, why do we need setTimeout here?
When formSub is true, the scrollViewContent will be re-rendered. At the same time, the useEffect hook will trigger the submit action. If the submit action is triggered before the re-rendering of scrollViewContent, the re-rendering will be delayed, resulting in the pointer events staying in the "auto" state.
By using setTimeout, we ensure that the scrollViewContent gets re-rendered first. We can also use promises or state callbacks to achieve the same result.
I also attempted to use two separate useState hooks. One to update scrollViewContent and the second one to submit the form after the first state is true. However, React batches state updates before re-rendering, so this approach didn't work as expected. I will give it another try.
Android - Workspace is still selectable even after 'Create room' button has been pressed
Form still is clickable after submitting. This issue only see on phone because our native app is low performance :(. It's hard to see on desktop.
As @Nodebrute said, there is a blocking in re-render when sending API and merging data in Onyx. A similar issue is checking/unchecking completed task and move mouse quickly out of a report action but it still shows hoverable style.
Back to this issue, we need to call submit
after we disable click on form. We can use onLayout
event with one empty View
to trigger that. In Form
component, we do something like below
const [isSubmitting, setIsSubmitting] = useState(false);
const isFirstOnLayout = useRef(true);
function resetSubmitState() {
isFirstOnLayout.current = true;
setIsSubmitting(false);
}
const submit = useCallback(() => {
...
// Reset submit state if validating has error
resetSubmitState();
...
}, [...]);
// Need to hide keyboard to prevent enter input on mobile.
useEffect(() => {
if (isSubmitting) {
Keyboard.dismiss();
}
}, [isSubmitting])
// Currently, I don't see any forms that we set `isLoading` from FE or BE. This hook is for the case we set it.
useEffect(() => {
if (isSubmitting && props.formState.isLoading === false) {
resetSubmitState();
}
}, [isSubmitting, props.formState])
// For the case users go to form page by deeplink, submit, and click to form page again.
useEffect(() => {
resetSubmitState();
}, [props.children])
...
<FormSubmit
...
onSubmit={() => setIsSubmitting(true)}
>
<View>
{childrenWrapperWithProps(_.isFunction(children) ? children({inputValues}) : children)}
</View>
{isSubmitting && <View style={{position: 'absolute', width: '100%', height: '100%'}}
onLayout={() => {
// For the case when submitting, users resize browser, or rotate screen.
if (isFirstOnLayout.current) {
isFirstOnLayout.current = false
submit();
}
}}
onMouseDown={(e) => e.preventDefault()}
/>}
...
<FormAlertWithSubmitButton
...
onSubmit={() => setIsSubmitting(true)}
cc: @fedirjh
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@fedirjh what do you think of the above proposals?
The input fields can still be passed the disabled field as I set in the alternative proposal. And by using the same loading state that blocks a second submission the input fields would also be disabled directly
@kmwamasali It would be simpler to just disable the Form
View, no need to add extra package.
what do you think of the above proposals?
No ideal proposal yet , still waiting for more approaches.
@fedirjh that makes sense
@fedirjh Could you tell me which should be improved in my proposal https://github.com/Expensify/App/issues/21577#issuecomment-1641026338? Thank you.
@fedirjh I had tried using the default isDisabled prop on the Picker
but because all form elements are touched during validation, it still deactivates just before submission which is too late and the user experience is as documented in the bug. Will attempt to work on the Form
as suggested for an appropriate proposal. Thank you for the guidance
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:
Expected Result:
Workspace should not be selected when the 'create room' button has been pressed
Actual Result:
Workspace is still selectable even after 'Create room' button has been pressed
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.29-11 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
https://github.com/Expensify/App/assets/93399543/9d5fd6ae-444f-4d53-ae03-dcf7a01e3060
https://github.com/Expensify/App/assets/93399543/d299d0d5-70ce-4586-ab4b-8fbb36f34c8f
Expensify/Expensify Issue URL: Issue reported by: @priya-zha Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687430588059259
View all open jobs on GitHub
Upwork Automation - Do Not Edit