Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: App crashes after pressing Backspace/Delete key in Incorrect magic code page #23734

Merged
merged 4 commits into from
Jul 28, 2023
Merged

Fix: App crashes after pressing Backspace/Delete key in Incorrect magic code page #23734

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5033e49
used onFocus instead of onPress for MagicInput
Pujan92 Jul 27, 2023
e32c93f
focus input on change text
Pujan92 Jul 27, 2023
e7b3e97
removed not required setFocusedIndex call
Pujan92 Jul 27, 2023
5488702
minor adjustments to MagicCodeInput
Pujan92 Jul 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 4 additions & 33 deletions src/components/MagicCodeInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ function MagicCodeInput(props) {
};

const focusMagicCodeInput = () => {
setFocusedIndex(0);
inputRefs.current[0].focus();
};

Expand All @@ -120,9 +119,6 @@ function MagicCodeInput(props) {
focusMagicCodeInput();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will set the input to an empty string. I think we can remove the resetFocus method now as it's same as focus

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resetFocus is added here for this issue. Are you suggesting that we can use focus method there instead of resetFocus and remove resetFocus here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree that we don't need an extra method now, I just committed the changes.

},
clear() {
setInput('');
setFocusedIndex(0);
setEditIndex(0);
inputRefs.current[0].focus();
props.onChangeText('');
},
Expand Down Expand Up @@ -176,23 +172,13 @@ function MagicCodeInput(props) {
}, []);

/**
* Focuses on the input when it is pressed.
*
* @param {Object} event
* @param {Number} index
*/
const onFocus = (event) => {
event.preventDefault();
};

/**
* Callback for the onPress event, updates the indexes
* Callback for the onFocus event, updates the indexes
* of the currently focused input.
*
* @param {Object} event
* @param {Number} index
*/
const onPress = (event, index) => {
const onFocus = (event, index) => {
event.preventDefault();
setInput('');
setFocusedIndex(index);
Expand Down Expand Up @@ -224,8 +210,7 @@ function MagicCodeInput(props) {
let numbers = decomposeString(props.value, props.maxLength);
numbers = [...numbers.slice(0, editIndex), ...numbersArr, ...numbers.slice(numbersArr.length + editIndex, props.maxLength)];

setFocusedIndex(updatedFocusedIndex);
setInput(value);
inputRefs.current[updatedFocusedIndex].focus();
Comment on lines -227 to +205
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to set the input to value but with this change we won't, the focus method will set the input to an empty string. This does not seem right. Can you please fix this

Copy link
Contributor Author

@Pujan92 Pujan92 Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intentionally take out the value bcoz I think we need to reset it once the value has been given for the input box. Its impact can be seen in the last input box where the focusedIndex isn't changing if the full code isn't Fulfilled.

Screen.Recording.2023-07-28.at.12.27.09.AM.mp4


const finalInput = composeToString(numbers);
props.onChangeText(finalInput);
Expand Down Expand Up @@ -265,13 +250,6 @@ function MagicCodeInput(props) {
}

const newFocusedIndex = Math.max(0, focusedIndex - 1);

// Saves the input string so that it can compare to the change text
// event that will be triggered, this is a workaround for mobile that
// triggers the change text on the event after the key press.
setInput('');
setFocusedIndex(newFocusedIndex);
setEditIndex(newFocusedIndex);
props.onChangeText(composeToString(numbers));

if (!_.isUndefined(newFocusedIndex)) {
Expand All @@ -280,15 +258,9 @@ function MagicCodeInput(props) {
}
if (keyValue === 'ArrowLeft' && !_.isUndefined(focusedIndex)) {
const newFocusedIndex = Math.max(0, focusedIndex - 1);
setInput('');
setFocusedIndex(newFocusedIndex);
setEditIndex(newFocusedIndex);
inputRefs.current[newFocusedIndex].focus();
} else if (keyValue === 'ArrowRight' && !_.isUndefined(focusedIndex)) {
const newFocusedIndex = Math.min(focusedIndex + 1, props.maxLength - 1);
setInput('');
setFocusedIndex(newFocusedIndex);
setEditIndex(newFocusedIndex);
inputRefs.current[newFocusedIndex].focus();
} else if (keyValue === 'Enter') {
// We should prevent users from submitting when it's offline.
Expand Down Expand Up @@ -346,8 +318,7 @@ function MagicCodeInput(props) {
onChangeText(value);
}}
onKeyPress={onKeyPress}
onPress={(event) => onPress(event, index)}
onFocus={onFocus}
onFocus={(event) => onFocus(event, index)}
caretHidden={isMobileSafari}
inputStyle={[isMobileSafari ? styles.magicCodeInputTransparent : undefined]}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.TEXT}
Expand Down