From cfb9541658dca9cdb243719eabbada8543bc0c27 Mon Sep 17 00:00:00 2001 From: Yogesh Bhatt Date: Fri, 20 Oct 2023 18:40:08 +0530 Subject: [PATCH 1/4] Resolve Cursor Position Issue on Long-Press in Android Chrome --- src/pages/iou/steps/MoneyRequestAmountForm.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/pages/iou/steps/MoneyRequestAmountForm.js b/src/pages/iou/steps/MoneyRequestAmountForm.js index d84c4d9e3cd0..00024528c2c6 100644 --- a/src/pages/iou/steps/MoneyRequestAmountForm.js +++ b/src/pages/iou/steps/MoneyRequestAmountForm.js @@ -136,10 +136,19 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu if (!_.isEmpty(formError)) { setFormError(''); } + + // There was an issue where long-pressing the back button in Android Chrome removed the last digit but moved the cursor ahead two positions instead of one. This occurred because setCurrentAmount contains another setState, making it impure and error-prone. Various solutions were suggested, including merging currentAmount and selection; however, this solution introducing the hasSelectionBeenSet flag was chosen for its simplicity and lower risk of future errors https://github.com/Expensify/App/issues/23300. + + let hasSelectionBeenSet = false; setCurrentAmount((prevAmount) => { const strippedAmount = MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces); const isForwardDelete = prevAmount.length > strippedAmount.length && forwardDeletePressedRef.current; - setSelection((prevSelection) => getNewSelection(prevSelection, isForwardDelete ? strippedAmount.length : prevAmount.length, strippedAmount.length)); + if (!hasSelectionBeenSet) { + setSelection((prevSelection) => { + hasSelectionBeenSet = true; + return getNewSelection(prevSelection, isForwardDelete ? strippedAmount.length : prevAmount.length, strippedAmount.length); + }); + } return strippedAmount; }); }, From 0289c3813da2b7b41e8cbe4139932caf3edb8f2b Mon Sep 17 00:00:00 2001 From: Yogesh Bhatt Date: Sat, 21 Oct 2023 00:49:33 +0530 Subject: [PATCH 2/4] Refactor MoneyRequestAmountForm setNewAmount --- src/pages/iou/steps/MoneyRequestAmountForm.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/pages/iou/steps/MoneyRequestAmountForm.js b/src/pages/iou/steps/MoneyRequestAmountForm.js index 00024528c2c6..bc4dab000d1e 100644 --- a/src/pages/iou/steps/MoneyRequestAmountForm.js +++ b/src/pages/iou/steps/MoneyRequestAmountForm.js @@ -137,17 +137,15 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu setFormError(''); } - // There was an issue where long-pressing the back button in Android Chrome removed the last digit but moved the cursor ahead two positions instead of one. This occurred because setCurrentAmount contains another setState, making it impure and error-prone. Various solutions were suggested, including merging currentAmount and selection; however, this solution introducing the hasSelectionBeenSet flag was chosen for its simplicity and lower risk of future errors https://github.com/Expensify/App/issues/23300. + // setCurrentAmount contains another setState making it error-prone. This solution introducing the hasSelectionBeenSet flag was chosen for its simplicity and lower risk of future errors https://github.com/Expensify/App/issues/23300. let hasSelectionBeenSet = false; setCurrentAmount((prevAmount) => { const strippedAmount = MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces); const isForwardDelete = prevAmount.length > strippedAmount.length && forwardDeletePressedRef.current; if (!hasSelectionBeenSet) { - setSelection((prevSelection) => { - hasSelectionBeenSet = true; - return getNewSelection(prevSelection, isForwardDelete ? strippedAmount.length : prevAmount.length, strippedAmount.length); - }); + hasSelectionBeenSet = true; + setSelection((prevSelection) => getNewSelection(prevSelection, isForwardDelete ? strippedAmount.length : prevAmount.length, strippedAmount.length)); } return strippedAmount; }); From f9e41e11917fe5e2ae915fdcd2f6ccdb1b0a41e6 Mon Sep 17 00:00:00 2001 From: Yogesh Bhatt Date: Sat, 21 Oct 2023 00:51:29 +0530 Subject: [PATCH 3/4] Comment update --- src/pages/iou/steps/MoneyRequestAmountForm.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/iou/steps/MoneyRequestAmountForm.js b/src/pages/iou/steps/MoneyRequestAmountForm.js index bc4dab000d1e..563399de0acf 100644 --- a/src/pages/iou/steps/MoneyRequestAmountForm.js +++ b/src/pages/iou/steps/MoneyRequestAmountForm.js @@ -137,7 +137,7 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu setFormError(''); } - // setCurrentAmount contains another setState making it error-prone. This solution introducing the hasSelectionBeenSet flag was chosen for its simplicity and lower risk of future errors https://github.com/Expensify/App/issues/23300. + // setCurrentAmount contains another setState making it error-prone. This solution introducing the hasSelectionBeenSet flag was chosen for its simplicity and lower risk of future errors https://github.com/Expensify/App/issues/23300#issuecomment-1766314724. let hasSelectionBeenSet = false; setCurrentAmount((prevAmount) => { From 4bae23fe1f10ee504ec0960776689ce66a63d36a Mon Sep 17 00:00:00 2001 From: Yogesh Bhatt <65986357+ygshbht@users.noreply.github.com> Date: Mon, 23 Oct 2023 17:45:56 +0530 Subject: [PATCH 4/4] Update comment src/pages/iou/steps/MoneyRequestAmountForm.js Co-authored-by: abdulrahuman5196 <46707890+abdulrahuman5196@users.noreply.github.com> --- src/pages/iou/steps/MoneyRequestAmountForm.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/iou/steps/MoneyRequestAmountForm.js b/src/pages/iou/steps/MoneyRequestAmountForm.js index 563399de0acf..8195d860a3c7 100644 --- a/src/pages/iou/steps/MoneyRequestAmountForm.js +++ b/src/pages/iou/steps/MoneyRequestAmountForm.js @@ -137,7 +137,7 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu setFormError(''); } - // setCurrentAmount contains another setState making it error-prone. This solution introducing the hasSelectionBeenSet flag was chosen for its simplicity and lower risk of future errors https://github.com/Expensify/App/issues/23300#issuecomment-1766314724. + // setCurrentAmount contains another setState(setSelection) making it error-prone since it is leading to setSelection being called twice for a single setCurrentAmount call. This solution introducing the hasSelectionBeenSet flag was chosen for its simplicity and lower risk of future errors https://github.com/Expensify/App/issues/23300#issuecomment-1766314724. let hasSelectionBeenSet = false; setCurrentAmount((prevAmount) => {