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

[HOLD for payment 2024-12-16] [$250] The Composer isn't getting focus again after the attachment modal is dismissed #52898

Closed
1 of 8 tasks
m-natarajan opened this issue Nov 21, 2024 · 23 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented Nov 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.65-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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
Expensify/Expensify Issue URL:
Issue reported by: @DylanDylann
Slack conversation (hyperlinked to channel name): ts_external_expensify_bugs

Action Performed:

  1. Go to any chat
  2. Click on Emoji button on composer
  3. Click outside to turn off emoji modal
  4. The main composer is focused again
  5. Click on Search button
  6. Click outside to turn off search modal
  7. The main composer is focused again
  8. Click on + Button > Add attachment
  9. Close the attachment modal

Expected Result:

The Composer should be focused again

Actual Result:

The Composer isn't getting focus again
The same issue also happens when turning off task creation RHP

Workaround:

Unknown

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug-focus-modal.mov
Bug-focus-RHP.mov
Recording.773.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021861194329606469814
  • Upwork Job ID: 1861194329606469814
  • Last Price Increase: 2024-11-25
  • Automatic offers:
    • DylanDylann | Reviewer | 105121364
    • FitseTLT | Contributor | 105121366
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @VictoriaExpensify
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 21, 2024
Copy link

melvin-bot bot commented Nov 21, 2024

Triggered auto assignment to @VictoriaExpensify (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.

@m-natarajan
Copy link
Author

Discovered this bug while reviewing this PR by @DylanDylann

@DylanDylann
Copy link
Contributor

@VictoriaExpensify Could I take over this issue if this is external? because of more context here

@melvin-bot melvin-bot bot added the Overdue label Nov 25, 2024
Copy link

melvin-bot bot commented Nov 25, 2024

@VictoriaExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@VictoriaExpensify
Copy link
Contributor

Thanks for the context @DylanDylann - It's all yours

@melvin-bot melvin-bot bot removed the Overdue label Nov 25, 2024
@VictoriaExpensify VictoriaExpensify added External Added to denote the issue can be worked on by a contributor Overdue labels Nov 25, 2024
Copy link

melvin-bot bot commented Nov 25, 2024

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

@melvin-bot melvin-bot bot changed the title The Composer isn't getting focus again after the attachment modal is dismissed [$250] The Composer isn't getting focus again after the attachment modal is dismissed Nov 25, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 25, 2024
Copy link

melvin-bot bot commented Nov 25, 2024

Current assignee @DylanDylann is eligible for the External assigner, not assigning anyone new.

@VictoriaExpensify
Copy link
Contributor

Not overdue

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 26, 2024

Edited by proposal-police: This proposal was edited at 2024-11-26 15:27:38 UTC.

Proposal

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

The Composer isn't getting focus again after the attachment modal is dismissed

What is the root cause of that problem?

  1. It doesn't auto-focus when the RHP has text input
    We already have a code to regaing focus on RHP modal hide here
    if (!((willBlurTextInputOnTapOutside || shouldAutoFocus) && !isNextModalWillOpenRef.current && !modal?.isVisible && isFocused && (!!prevIsModalVisible || !prevIsFocused))) {
    return;
    }
    if (editFocused) {
    InputFocus.inputFocusChange(false);
    return;
    }
    focus(true);

    but the focus here is called too early especially when there is a text input on the RHP

    2.Auto focus not working after closing attachment picker: We are not focusing the composer on closing of the attachment modal unlike emoji picker case where we focused onModalHide here

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

  1. We need to delay the focus call here to be called on the next rendering cycle
            setTimeout(() => textInput.focus());

We should pass focus for onCanceledAttachmentPicker of the attachment picker

 onCanceledAttachmentPicker={() => {
                                                if (!shouldFocusInputOnScreenFocus) {
                                                    return;
                                                }
                                                focus();
                                            }}

What alternative solutions did you explore? (Optional)

@DylanDylann
Copy link
Contributor

DylanDylann commented Nov 27, 2024

@FitseTLT Please detail your alternative solution. Note that we only apply this fix for the large screen

Additionally, you missed the second bug: The main composer focus again after turn off RHP strict create expense but not in other RHP

@FitseTLT
Copy link
Contributor

@DylanDylann Updated

@DylanDylann
Copy link
Contributor

I don't think setTimeout is recommended in our codebase

@FitseTLT
Copy link
Contributor

@DylanDylann what about runAfterInteraction?

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 28, 2024

@DylanDylann Let me explain it a bit for you:
For our modals we already have our method isReadyToFocus

Promise.all([ComposerFocusManager.isReadyToFocus(), isWindowReadyToFocus()]).then(() => {

which will dispatch the focus when the open modal has been dismissed
const handleDismissModal = () => {
ComposerFocusManager.setReadyToFocus(uniqueModalId);
};

The problem is RHP are not like our modals and it is opened by navigation and the modal value will be set to false when the current screen/RHP is blurred here
blur: () => {
Modal.setModalVisibility(false);

that will trigger the effect to focus the composer here
if (!((willBlurTextInputOnTapOutside || shouldAutoFocus) && !isNextModalWillOpenRef.current && !modal?.isVisible && isFocused && (!!prevIsModalVisible || !prevIsFocused))) {
return;
}
if (editFocused) {
InputFocus.inputFocusChange(false);
return;
}
focus(true);

But the onBlur is called too early immediately when the navigation away from the RHP starts even before the animation to close the RHP is finished so if there is a text input in the RHP our focus function will not focus the composer.
Therefore for a similar solution we used as isReadyToFocus one we should set some file-level variable on blur of the screen like didRHPDismissed and here
if (!promise) {
return Promise.resolve();

instead immediately resolving when there is no promise when the didRHPDismissed is true we will resolve it after some delay as CONST.ANIMATED_TRANSITION because the modal is correctly dismissed by that time. WDYT

@DylanDylann
Copy link
Contributor

@FitseTLT's #52898 (comment) looks good to me

After testing I see setTimeout gives a better output than runAfterInteraction. However, runAfterInteraction can be a safer option because we need to avoid using setTimeout as much as possible in our App.

Note in the PR phase: We need to make sure this change doesn't affect the current behavior on mobile web, app

🎀 👀 🎀 C+ Reviewed

Copy link

melvin-bot bot commented Nov 29, 2024

Triggered auto assignment to @grgia, 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 Nov 29, 2024
Copy link

melvin-bot bot commented Nov 29, 2024

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

Offer link
Upwork job

Copy link

melvin-bot bot commented Nov 29, 2024

📣 @FitseTLT 🎉 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 📖

@grgia
Copy link
Contributor

grgia commented Nov 29, 2024

Assigned!

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Nov 29, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 9, 2024
@melvin-bot melvin-bot bot changed the title [$250] The Composer isn't getting focus again after the attachment modal is dismissed [HOLD for payment 2024-12-16] [$250] The Composer isn't getting focus again after the attachment modal is dismissed Dec 9, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Dec 9, 2024
Copy link

melvin-bot bot commented Dec 9, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Dec 9, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.72-1 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 2024-12-16. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Dec 9, 2024

@DylanDylann @VictoriaExpensify @DylanDylann The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 16, 2024
@VictoriaExpensify
Copy link
Contributor

Payment Summary:
Contributor: @FitseTLT paid $250 via Upwork
C+: @DylanDylann paid $250 via Upwork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Development

No branches or pull requests

5 participants