-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix: Android keyboard not opening on Magic Code screen #83229
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import {useFocusEffect} from '@react-navigation/native'; | ||
| import type {ForwardedRef} from 'react'; | ||
| import React, {useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react'; | ||
| import {AccessibilityInfo, View} from 'react-native'; | ||
| import {InteractionManager, AccessibilityInfo, View} from 'react-native'; | ||
| import type {StyleProp, ViewStyle} from 'react-native'; | ||
| import Button from '@components/Button'; | ||
| import DotIndicatorMessage from '@components/DotIndicatorMessage'; | ||
|
|
@@ -13,13 +13,15 @@ import Text from '@components/Text'; | |
| import ValidateCodeCountdown from '@components/ValidateCodeCountdown'; | ||
| import type {ValidateCodeCountdownHandle} from '@components/ValidateCodeCountdown/types'; | ||
| import {useWideRHPState} from '@components/WideRHPContextProvider'; | ||
| import useAutoFocusInput from '@hooks/useAutoFocusInput'; | ||
| import useLocalize from '@hooks/useLocalize'; | ||
| import useNetwork from '@hooks/useNetwork'; | ||
| import useOnyx from '@hooks/useOnyx'; | ||
| import useStyleUtils from '@hooks/useStyleUtils'; | ||
| import useTheme from '@hooks/useTheme'; | ||
| import useThemeStyles from '@hooks/useThemeStyles'; | ||
| import {isMobileSafari} from '@libs/Browser'; | ||
| import * as DeviceCapabilities from '@libs/DeviceCapabilities'; | ||
| import {getLatestErrorField, getLatestErrorMessage} from '@libs/ErrorUtils'; | ||
| import {isValidValidateCode} from '@libs/ValidationUtils'; | ||
| import {clearValidateCodeActionError} from '@userActions/User'; | ||
|
|
@@ -121,15 +123,19 @@ function BaseValidateCodeForm({ | |
| const [formError, setFormError] = useState<ValidateCodeFormError>({}); | ||
| const [validateCode, setValidateCode] = useState(''); | ||
| const [isCountdownRunning, setIsCountdownRunning] = useState(true); | ||
| const [focusCycle, setFocusCycle] = useState(0); | ||
|
|
||
| const inputValidateCodeRef = useRef<MagicCodeInputHandle>(null); | ||
| const [account = getEmptyObject<Account>()] = useOnyx(ONYXKEYS.ACCOUNT); | ||
| const {inputCallbackRef, inputRef} = useAutoFocusInput(false); | ||
| const [account = getEmptyObject<Account>()] = useOnyx(ONYXKEYS.ACCOUNT, { | ||
| canBeMissing: true, | ||
| }); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- nullish coalescing doesn't achieve the same result in this case | ||
| const shouldDisableResendValidateCode = !!isOffline || account?.isLoading; | ||
| const focusTimeoutRef = useRef<NodeJS.Timeout | null>(null); | ||
| const [canShowError, setCanShowError] = useState<boolean>(false); | ||
| const [validateCodeAction] = useOnyx(ONYXKEYS.VALIDATE_ACTION_CODE); | ||
| const [validateCodeAction] = useOnyx(ONYXKEYS.VALIDATE_ACTION_CODE, {canBeMissing: true}); | ||
| const validateCodeSent = useMemo(() => hasMagicCodeBeenSent ?? validateCodeAction?.validateCodeSent, [hasMagicCodeBeenSent, validateCodeAction?.validateCodeSent]); | ||
| const latestValidateCodeError = getLatestErrorField(validateCodeAction, validateCodeActionErrorField); | ||
| const defaultValidateCodeError = getLatestErrorField(validateCodeAction, 'actionVerified'); | ||
|
|
@@ -163,29 +169,48 @@ function BaseValidateCodeForm({ | |
|
|
||
| useFocusEffect( | ||
| useCallback(() => { | ||
| if (!inputValidateCodeRef.current) { | ||
| return; | ||
| } | ||
| const focusInput = () => { | ||
| inputValidateCodeRef.current?.focusLastSelected(); | ||
| }; | ||
|
|
||
| // Force a fresh input instance on each screen focus to avoid stale native focus state on reopen. | ||
| setFocusCycle((prev) => prev + 1); | ||
|
|
||
| const runFocusWithRetry = () => { | ||
| focusInput(); | ||
| focusTimeoutRef.current = setTimeout(() => { | ||
| if (inputRef.current?.isFocused?.()) { | ||
| return; | ||
| } | ||
| focusInput(); | ||
| }, CONST.ANIMATED_TRANSITION); | ||
| }; | ||
|
Comment on lines
+177
to
+187
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since runFocusWithRetry is called in two places, focusInput ends up being triggered four times. This seems like a hardcoded workaround. @twinkle-react what happens on the latest main with the initial solution using the useAutoFocusInput hook?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the feedback! You're right — the current implementation introduces redundant focus logic. I’m refactoring this to rely on a single focus mechanism using useAutoFocusInput and limiting the fix to Android only. Also removing the remount-based approach and simplifying retries. |
||
|
|
||
|
Comment on lines
+172
to
+188
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @twinkle-react Which use cases do we need to handle with this change?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @suneox I fixed the Magic Code keyboard issue by improving how input focus is handled when the screen gains focus. The input is re-initialized and programmatically focused, with a retry after screen transitions to handle Android focus timing issues. This ensures the keyboard opens consistently on both initial and subsequent visits, while behavior on other platforms remains unchanged. |
||
| if (focusTimeoutRef.current) { | ||
| clearTimeout(focusTimeoutRef.current); | ||
| } | ||
|
|
||
| // Keyboard won't show if we focus the input with a delay, so we need to focus immediately. | ||
| if (!isMobileSafari()) { | ||
| focusTimeoutRef.current = setTimeout(() => { | ||
| inputValidateCodeRef.current?.focusLastSelected(); | ||
| }, CONST.ANIMATED_TRANSITION); | ||
| } else { | ||
| inputValidateCodeRef.current?.focusLastSelected(); | ||
| if (DeviceCapabilities.canUseTouchScreen()) { | ||
| focusTimeoutRef.current = setTimeout(runFocusWithRetry, 0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This issue only occurs on Android, and runFocusWithRetry also runs below we have 2 places try to run focus |
||
|
|
||
|
Comment on lines
+193
to
+195
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This change removes the old synchronous Mobile Safari focus path and now always defers focus on touch devices via Useful? React with 👍 / 👎. |
||
| return () => { | ||
| if (!focusTimeoutRef.current) { | ||
| return; | ||
| } | ||
| clearTimeout(focusTimeoutRef.current); | ||
| }; | ||
| } | ||
|
|
||
| const interactionTask = InteractionManager.runAfterInteractions(runFocusWithRetry); | ||
twinkle-react marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| return () => { | ||
| interactionTask.cancel(); | ||
| if (!focusTimeoutRef.current) { | ||
| return; | ||
| } | ||
| clearTimeout(focusTimeoutRef.current); | ||
| }; | ||
| }, []), | ||
| }, [inputRef]), | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
|
|
@@ -196,6 +221,7 @@ function BaseValidateCodeForm({ | |
| countdownRef.current?.resetCountdown(); | ||
| }, [isCountdownRunning]); | ||
|
|
||
| // ✅ Accessibility announce (added from Code 2) | ||
| useEffect(() => { | ||
| if (!validateCodeSent) { | ||
| return; | ||
|
|
@@ -207,9 +233,9 @@ function BaseValidateCodeForm({ | |
| if (!validateCodeSent) { | ||
| return; | ||
| } | ||
| // Delay prevents the input from gaining focus before the RHP slide-out animation finishes, | ||
| // which would cause issues with the RHP sliding out smoothly and flickering of the wide RHP in the background. | ||
| if ((wideRHPRouteKeys.length > 0 && !isMobileSafari()) || isInPageModal) { | ||
| const shouldDelayClear = !DeviceCapabilities.canUseTouchScreen() && ((wideRHPRouteKeys.length > 0 && !isMobileSafari()) || isInPageModal); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
|
|
||
| if (shouldDelayClear) { | ||
| focusTimeoutRef.current = setTimeout(() => { | ||
| inputValidateCodeRef.current?.clear(); | ||
| }, CONST.ANIMATED_TRANSITION); | ||
|
|
@@ -218,47 +244,32 @@ function BaseValidateCodeForm({ | |
| } | ||
| }, [validateCodeSent, wideRHPRouteKeys.length, isInPageModal]); | ||
|
|
||
| /** | ||
| * Request a validate code / magic code be sent to verify this contact method | ||
| */ | ||
| const resendValidateCode = () => { | ||
| sendValidateCode(); | ||
| inputValidateCodeRef.current?.clear(); | ||
| countdownRef.current?.resetCountdown(); | ||
| setIsCountdownRunning(true); | ||
| }; | ||
|
|
||
| /** | ||
| * Handle text input and clear formError upon text change | ||
| */ | ||
| const onTextInput = useCallback( | ||
| (text: string) => { | ||
| setValidateCode(text); | ||
| setFormError({}); | ||
|
|
||
| if (!isEmptyObject(validateError) || !isEmptyObject(latestValidateCodeError)) { | ||
| // Clear flow specific error | ||
| clearError(); | ||
|
|
||
| // Clear "incorrect magic code" error | ||
| clearValidateCodeActionError(validateCodeActionErrorField); | ||
| } | ||
| }, | ||
| [validateError, clearError, latestValidateCodeError, validateCodeActionErrorField], | ||
| ); | ||
|
|
||
| /** | ||
| * Check that all the form fields are valid, then trigger the submit callback | ||
| */ | ||
| const validateAndSubmitForm = useCallback(() => { | ||
| // Clear flow specific error | ||
| clearError(); | ||
|
|
||
| // Clear "incorrect magic" code error | ||
| clearValidateCodeActionError(validateCodeActionErrorField); | ||
|
|
||
| clearDefaultValidationCodeError(); | ||
| setCanShowError(true); | ||
|
|
||
| if (!validateCode.trim()) { | ||
| setFormError({validateCode: 'validateCodeForm.error.pleaseFillMagicCode'}); | ||
| return; | ||
|
|
@@ -289,12 +300,12 @@ function BaseValidateCodeForm({ | |
| setIsCountdownRunning(false); | ||
| }, []); | ||
|
|
||
| // latestValidateCodeError only holds an error related to bad magic code | ||
| // while validateError holds flow-specific errors | ||
| const finalValidateError = !isEmptyObject(latestValidateCodeError) ? latestValidateCodeError : validateError; | ||
|
|
||
| return ( | ||
| <> | ||
| <MagicCodeInput | ||
| key={focusCycle} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Setting Useful? React with 👍 / 👎. |
||
| autoComplete={autoComplete} | ||
| ref={inputValidateCodeRef} | ||
| name="validateCode" | ||
|
|
@@ -304,6 +315,7 @@ function BaseValidateCodeForm({ | |
| hasError={canShowError && !isEmptyObject(finalValidateError)} | ||
| onFulfill={validateAndSubmitForm} | ||
| autoFocus={false} | ||
| inputCallbackRef={inputCallbackRef} | ||
| /> | ||
| {shouldShowTimer && ( | ||
| <View style={[styles.mt5, styles.flexRow, styles.renderHTML]}> | ||
|
|
@@ -313,6 +325,7 @@ function BaseValidateCodeForm({ | |
| /> | ||
| </View> | ||
| )} | ||
|
|
||
| <OfflineWithFeedback | ||
| pendingAction={validateCodeAction?.pendingFields?.validateCodeSent} | ||
| errorRowStyles={[styles.mt2]} | ||
|
|
@@ -336,12 +349,13 @@ function BaseValidateCodeForm({ | |
| </View> | ||
| )} | ||
| </OfflineWithFeedback> | ||
|
|
||
| {/* ✅ accessibility wrapper added (from Code 2) */} | ||
| <View accessibilityLiveRegion="polite"> | ||
| {!!validateCodeSent && ( | ||
| <DotIndicatorMessage | ||
| type="success" | ||
| style={[styles.mt6, styles.flex0]} | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| messages={{0: translate('validateCodeModal.successfulNewCodeRequest')}} | ||
| /> | ||
| )} | ||
|
|
@@ -368,7 +382,6 @@ function BaseValidateCodeForm({ | |
| style={[styles.mt4]} | ||
| success={false} | ||
| large | ||
| sentryLabel={CONST.SENTRY_LABEL.VALIDATE_CODE.SKIP} | ||
| /> | ||
| )} | ||
| {!hideSubmitButton && ( | ||
|
|
@@ -379,9 +392,7 @@ function BaseValidateCodeForm({ | |
| style={[shouldShowSkipButton ? styles.mt3 : styles.mt4]} | ||
| success | ||
| large | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| isLoading={account?.isLoading || isLoading} | ||
| sentryLabel={CONST.SENTRY_LABEL.VALIDATE_CODE.VERIFY} | ||
| /> | ||
| )} | ||
| </OfflineWithFeedback> | ||
|
|
@@ -391,4 +402,4 @@ function BaseValidateCodeForm({ | |
|
|
||
| export type {ValidateCodeFormProps, ValidateCodeFormHandle}; | ||
|
|
||
| export default BaseValidateCodeForm; | ||
| export default BaseValidateCodeForm; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ CONSISTENCY-3 (docs)
useAutoFocusInputalready provides comprehensive focus management internally -- it tracks screen transitions viatransitionEndlistener, usesInteractionManager.runAfterInteractions, handles splash screen state, popover visibility, and side panel transitions, then callsinputRef.current?.focus()at the right moment. However, theuseFocusEffectcallback below (lines ~170-214) implements a second, parallel focus management system with its ownInteractionManager.runAfterInteractionscall (line 204),setTimeoutretry logic,DeviceCapabilitiesbranching, and immediatefocusLastSelected()calls -- all targeting the same input.Two independent systems racing to focus the same
MagicCodeInputmakes behavior hard to predict and debug.Consolidate into a single approach:
useAutoFocusInputis the right tool, remove the manual focus logic fromuseFocusEffectand let the hook handle everything.focus()but you needfocusLastSelected()), either extend the hook or remove it and keep only the manual approach.Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.