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.

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

[$150] Web - Workspace - Invite new members" field is not focused #44182

Closed
1 of 6 tasks
lanitochka17 opened this issue Jun 21, 2024 · 22 comments
Closed
1 of 6 tasks

[$150] Web - Workspace - Invite new members" field is not focused #44182

lanitochka17 opened this issue Jun 21, 2024 · 22 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Jun 21, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.0-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

  1. Log in to an account
  2. Go to workspace(Or create one) > Members
  3. Click on the "Invite member" button at the top. Notice the field is focused
  4. On the Invite members page click on the Question mark button to go to the "Get assistance"
  5. Click back to return to the Invite Members page

Expected Result:

Invite members page field is focused

Actual Result:

The invite members page field is not focused

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6520591_1718982403717.Invite_field_Issue.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bda364df69d71d2a
  • Upwork Job ID: 1805292410395622118
  • Last Price Increase: 2024-06-24
  • Automatic offers:
    • tsa321 | Contributor | 102885241
Issue OwnerCurrent Issue Owner: @rushatgabhane
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 21, 2024
Copy link

melvin-bot bot commented Jun 21, 2024

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@lanitochka17
Copy link
Author

@adelekennedy FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jun 21, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Web - Workspace - Invite new members" field is not focused

What is the root cause of that problem?

We are directly using useFocusEffect & useCallback which does not cover cases like isScreenTransitionEnded and due to this the input is not focused correctly.

/** Focuses the text input when the component comes into focus and after any navigation animations finish. */
useFocusEffect(
useCallback(() => {
if (textInputAutoFocus && shouldShowTextInput) {
if (shouldDelayFocus) {
focusTimeoutRef.current = setTimeout(focusTextInput, CONST.ANIMATED_TRANSITION);
} else {
focusTextInput();
}
}
return () => focusTimeoutRef.current && clearTimeout(focusTimeoutRef.current);
}, [shouldShowTextInput, textInputAutoFocus, shouldDelayFocus, focusTextInput]),
);

What changes do you think we should make in order to solve the problem?

We should use inputCallbackRef from useAutoFocusInput which is designed to focus on the input on every focus on the screen.

    useEffect(() => {
        if (!textInputAutoFocus || !shouldShowTextInput) {
            return;
        }
        inputCallbackRef(innerTextInputRef.current);
    }, [textInputAutoFocus, inputCallbackRef, shouldShowTextInput]);

    const prevTextInputValue = usePrevious(textInputValue);
    const prevSelectedOptionsLength = usePrevious(flattenedSections.selectedOptions.length);

What alternative solutions did you explore? (Optional)

Use InteractionManager.runAfterInteractions in focusTextInput function.

/** Function to focus text input */
const focusTextInput = useCallback(() => {
if (!innerTextInputRef.current) {
return;
}
innerTextInputRef.current.focus();
}, []);

    /** Function to focus text input */
    const focusTextInput = useCallback(() => {
        if (!innerTextInputRef.current) {
            return;
        }

        InteractionManager.runAfterInteractions(() => {
            innerTextInputRef.current.focus();
        });
    }, []);

@tsa321
Copy link
Contributor

tsa321 commented Jun 22, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Web - Workspace - Invite new members" field is not focused

What is the root cause of that problem?

Because of the newly added focusTrap and the Workspace_Invite screen is not included in SCREENS_WITH_AUTOFOCUS:

setReturnFocus: (element) => {
if (SCREENS_WITH_AUTOFOCUS.includes(activeRouteName)) {
return false;
}
return element;

When user returns to previous screen, focusTrap will refocus last focused element, return focus will return the ? button element and will get refocused. It is because, the ? button is last focused element before Get Assistance screen opened. The button element will be saved by focusTrap and will refocus when Get Assistace screen is closed. If we include the screen (SCREENS.WORKSPACE.INVITE) in SCREENS_WITH_AUTOFOCUS, the setReturnFocus will return false and won't refocus the ? button.

What changes do you think we should make in order to solve the problem?

We should add the Workspace_Invite (SCREENS.WORKSPACE.INVITE) screen in SCREENS_WITH_AUTOFOCUS

alternatively, if we want to disable refocusing last active element in previous screen we could return false here..

@dominictb
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

  • The invite members page field is not focused

What is the root cause of that problem?

  • Firstly, we need to note that, this bug only appears when user blurs the current screen by clicking on "Question mark" button. Other cases, like when we choose any user then click on "Next" to go to invite message screen, then go back, to search input is auto-focused properly. Below is the RCA.

  • We have a variable shouldSyncFocus with the usage:

    /**
    * Whether the focus on the element should be synchronized. For example it should be set to false when the text input above list items is currently focused.
    * When we type something into the text input, the first element found is focused, in this situation we should not synchronize the focus on the element because we will lose the focus from the text input.
    */
    shouldSyncFocus?: boolean;

and it is set to false if the search input is not focused:

shouldSyncFocus={!isTextInputFocusedRef.current}

  • This feature works properly when we click on the "Next" button (as I mentioned above) because when we click on it to go to invite message screen, the isTextInputFocusedRef is still true, so shouldSyncFocus is false, so when we go back to invite member page, the search input is auto-focused via logic:

    useFocusEffect(
    useCallback(() => {
    if (textInputAutoFocus && shouldShowTextInput) {
    if (shouldDelayFocus) {
    focusTimeoutRef.current = setTimeout(focusTextInput, CONST.ANIMATED_TRANSITION);
    } else {
    focusTextInput();
    }
    }
    return () => focusTimeoutRef.current && clearTimeout(focusTimeoutRef.current);
    }, [shouldShowTextInput, textInputAutoFocus, shouldDelayFocus, focusTextInput]),
    );

    then is not blurred after that.

  • But when we click on the "Question mark", the isTextInputFocusedRef is set to false, so the shouldSyncFocus is true. So when we go back to the invite member screen, the search input is focused and then blurred because the focused component now is something like <Button id="backButton ... /> (We can verify it by add the log console.log(DomUtils.getActiveElement()) to:

    if (textInputAutoFocus && shouldShowTextInput) {

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Jun 24, 2024
@adelekennedy
Copy link

I can reproduce, and while this is a small bug I think it's worth it to fix

@melvin-bot melvin-bot bot removed the Overdue label Jun 24, 2024
@adelekennedy adelekennedy added the External Added to denote the issue can be worked on by a contributor label Jun 24, 2024
@melvin-bot melvin-bot bot changed the title Web - Workspace - Invite new members" field is not focused [$250] Web - Workspace - Invite new members" field is not focused Jun 24, 2024
Copy link

melvin-bot bot commented Jun 24, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01bda364df69d71d2a

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 24, 2024
Copy link

melvin-bot bot commented Jun 24, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

@adelekennedy
Copy link

@rushatgabhane already three proposals above to review

@adelekennedy adelekennedy changed the title [$250] Web - Workspace - Invite new members" field is not focused [$150] Web - Workspace - Invite new members" field is not focused Jun 24, 2024
Copy link

melvin-bot bot commented Jun 24, 2024

Upwork job price has been updated to $150

@rushatgabhane
Copy link
Member

@tsa321's proposal LGTM

🎀 👀 🎀

Copy link

melvin-bot bot commented Jun 26, 2024

Triggered auto assignment to @srikarparsi, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 26, 2024
Copy link

melvin-bot bot commented Jun 26, 2024

📣 @tsa321 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jun 26, 2024
@tsa321
Copy link
Contributor

tsa321 commented Jun 27, 2024

@rushatgabhane PR is ready

Copy link

melvin-bot bot commented Jul 11, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Jul 22, 2024
Copy link

melvin-bot bot commented Jul 22, 2024

This issue has not been updated in over 15 days. @rushatgabhane, @srikarparsi, @adelekennedy, @tsa321 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@adelekennedy
Copy link

It looks like this was deployed to prod on the 15th and payment is due!

@adelekennedy
Copy link

adelekennedy commented Jul 22, 2024

Payouts due:

@adelekennedy
Copy link

Do we need a regression test here @rushatgabhane?

@rushatgabhane
Copy link
Member

@adelekennedy this was caught by QA so I don't think we do. And it's a very minor edge case bug.

@JmillsExpensify
Copy link

$150 approved for @rushatgabhane

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants