[HOLD for payment 2023-09-21] Web - Input field not focused on click of back button from Get assistance page #25893
Comments
izarutskaya
added
DeployBlockerCash
|
Triggered auto assignment to @kadiealexander ( |
Bug0 Triage Checklist (Main S/O)
|
|
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open
|
|
Triggered auto assignment to @grgia ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.This is happening after #22622 this PR, we don't do any focus on the input when navigating back. What is the root cause of that problem?After getting merged of #25894 which will allow us to manually focus the input, we can use the updated ref and would be focusing depends on route focus in Screen const isFocused = props.navigation.isFocused()
useEffect(() => {
if(selectionListRef.current && isFocused){
selectListRef.current.focusTextInput()
}
},[isFocused, selectionListRef])Would be giving the focus to the input whenever we navigate back. What changes do you think we should make in order to solve the problem?NA |
|
@b4s36t4 we should implement "focus input on navigation focus" in BaseSelectionList which is base component, similar to BaseOptionsSelector here: App/src/components/OptionsSelector/BaseOptionsSelector.js Lines 101 to 108 in 0b7ae7f |
|
@situchan gotcha. I think the same approach would do the work, except in context of functional component and the way we use the props. |
|
This is still reproducible in 1.3.57-5 |
|
@situchan I tried that actually for a related issue – I tried this solution and it works great for pushing new routes onto the stack, but doesn't work for popping routes off the stack because, as Satya explains here, In this case we want to run an effect on the new top card on the stack only after |
|
This solution seems to work well for pushing new cards onto the stack as well as popping them: diff --git a/src/components/SelectionList/BaseSelectionList.js b/src/components/SelectionList/BaseSelectionList.js
index 046b64e9e5..74083c4ad8 100644
--- a/src/components/SelectionList/BaseSelectionList.js
+++ b/src/components/SelectionList/BaseSelectionList.js
@@ -1,7 +1,8 @@
-import React, {useEffect, useMemo, useRef, useState} from 'react';
+import React, {useMemo, useRef, useState, useCallback} from 'react';
import {View} from 'react-native';
import _ from 'underscore';
import lodashGet from 'lodash/get';
+import {useFocusEffect} from '@react-navigation/native';
import SectionList from '../SectionList';
import Text from '../Text';
import styles from '../../styles/styles';
@@ -266,23 +267,20 @@ function BaseSelectionList({
);
};
- /** Focuses the text input when the component mounts. If `props.shouldDelayFocus` is true, we wait for the animation to finish */
- useEffect(() => {
- if (shouldShowTextInput) {
- if (shouldDelayFocus) {
+ /** Focuses the text input when the screen this component is included in comes into focus and any transitions to open that screen are completed. */
+ useFocusEffect(
+ useCallback(() => {
+ if (shouldShowTextInput) {
focusTimeoutRef.current = setTimeout(() => textInputRef.current.focus(), CONST.ANIMATED_TRANSITION);
- } else {
- textInputRef.current.focus();
- }
- }
-
- return () => {
- if (!focusTimeoutRef.current) {
- return;
}
- clearTimeout(focusTimeoutRef.current);
- };
- }, [shouldDelayFocus, shouldShowTextInput]);
+ return () => {
+ if (!focusTimeoutRef.current) {
+ return;
+ }
+ clearTimeout(focusTimeoutRef.current);
+ };
+ }, [shouldShowTextInput]),
+ );
/** Selects row when pressing Enter */
useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, {
The only downside (and it's pretty minor...) is that we're still using |
|
Started a draft PR with this implementation: #25983 That said, I don't think this needs to be a deploy blocker. Demoting this. |
roryabraham
added
Daily
|
PR is ready to review here: #26415 |
melvin-bot
bot
added
Reviewing
|
PR is on staging |
melvin-bot
bot
added
Weekly
melvin-bot
bot
changed the title
|
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.69-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-09-21. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
|
@roryabraham seems like no one is due payments based on this comment from you, can you please confirm if that's still correct? |
|
Correct, no payments due. Going to close this out. |
|
@roryabraham this is eligible for reporting bonus |
|
Sorry about that. Looks like reporting bonus should apply here |
|
Sent you a contract @gadhiyamanan! https://www.upwork.com/ab/applicants/1706896515002716160/hired for my own reference |
|
@kadiealexander offer accepted, thanks! |
|
Nice! All paid :) |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
input field not focused on click of back button
Actual Result:
input field should be focused on click of back button from Get assistance page
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: v1.3.57-2
Reproducible in staging?: Y
Reproducible in production?: N
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
Screen.Recording.2023-08-23.At.1.42.51.Pm.mp4
Recording.1320.mp4
Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692778407152369
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: