App icon indicating copy to clipboard operation
App copied to clipboard

[$500] mWeb - Chat - The keyboard appears for a moment when leaving the thread

Open kbecciv opened this issue 1 year ago • 9 comments

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: v1.4.34-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: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Issue found when executing PR https://github.com/Expensify/App/pull/35202

Action Performed:

  1. Go to URL https://staging.new.expensify.com/ on Android
  2. Login with any account
  3. Navigate to any conversation
  4. Start a thread and send some messages
  5. Click on the three-dot menu and hit "Leave thread"

Expected Result:

The keyboard doesn't appear for a moment when leaving the thread

Actual Result:

The keyboard appears for a moment when leaving the thread

Workaround:

Unknown

Platforms:

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

  • [ ] Android: Native
  • [x] Android: mWeb Chrome
  • [ ] iOS: Native
  • [ ] iOS: mWeb Safari
  • [ ] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/a5af76fc-b522-4efe-93de-d2573919715b

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01278478c2e7ed2136
  • Upwork Job ID: 1752743923246346240
  • Last Price Increase: 2024-01-31

kbecciv avatar Jan 31 '24 17:01 kbecciv

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

melvin-bot[bot] avatar Jan 31 '24 17:01 melvin-bot[bot]

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] avatar Jan 31 '24 17:01 melvin-bot[bot]

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

melvin-bot[bot] avatar Jan 31 '24 17:01 melvin-bot[bot]

We think that this bug might be related to #vip-vsb CC @quinthar

kbecciv avatar Jan 31 '24 17:01 kbecciv

I can reproduce this based on the steps in the OP. I think this can be External so waiting for proposals.

Christinadobrzyn avatar Jan 31 '24 19:01 Christinadobrzyn

Proposal

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

The keyboard appears for a moment when leaving the thread

What is the root cause of that problem?

When we are in a thread and the composer is focused the keyboard going to be displayed, but when we click on the three dots the keyboard get hide (as part of the behavior of this component), here no matter what option we select the keyboard going to be displayed again after three dots menu get hide, because of this when we leave the thread the keyboard seems to appear (expected behavior) and then disappears when the leave action makes we navigates back to main thread because the composer loose the focus.

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

We can keep the focus on the composer when navigating backwards and the keyboard is displayed.

To do this we should modify the ReportScreen Component to add a variable that determine if the user got to the screen using the navigation component with path (this let us know if the user is not getting to the Report from LHN), like:

const isReportOpenedFromNavigationPath = Boolean(lodashGet(route, 'path') || false);

And pass this to ReportFooter as a prop:

<ReportFooter
                                       report={report}
                                       pendingAction={addWorkspaceRoomOrChatPendingAction}
                                       isComposerFullSize={isComposerFullSize}
                                       listHeight={listHeight}
                                       isEmptyChat={isEmptyChat}
                                       lastReportAction={lastReportAction}
                                       isReportOpenedFromNavigationPath={isReportOpenedFromNavigationPath}
                                   />

With the prop isReportOpenedFromNavigationPath we can now determinate when to force the input focus, to do this we can pass as a prop the result of this variable on ReportActionCompose, like:

 const forceFocusComposer = props.isReportOpenedFromNavigationPath;
.
.
.
<ReportActionCompose
                            onSubmit={onSubmitComment}
                            reportID={props.report.reportID}
                            report={props.report}
                            isEmptyChat={props.isEmptyChat}
                            lastReportAction={props.lastReportAction}
                            pendingAction={props.pendingAction}
                            isComposerFullSize={props.isComposerFullSize}
                            listHeight={props.listHeight}
                            isReportReadyForDisplay={props.isReportReadyForDisplay}
                            shouldForceComposerFocus={forceFocusComposer}
                        />

Finally we should receive the new prop and use it to focus the composer when is true inside of ReportActionCompose component, like:

useEffect(() => {
        if (!shouldForceComposerFocus) {
            return;
        }
        focus();
    }, [shouldForceComposerFocus]);

Result:

https://github.com/Expensify/App/assets/5216036/9718af6e-46a5-43ff-8ef9-e23dcc6eb1c5

samilabud avatar Jan 31 '24 21:01 samilabud

collecting proposals!

Christinadobrzyn avatar Feb 01 '24 23:02 Christinadobrzyn

@samilabud Thanks for your proposal

https://github.com/Expensify/App/blob/ed95bc03c14e81d6ea78508b2d318ac5231d9b97/src/components/ThreeDotsMenu/index.tsx#L102-L108

Above code block was introduced by this PR. As per the discussion in the PR, the change was made to refocus the text input after closing the modal.

I was testing it and in any other case, if the composer was focused and we open any modal or menu, it get's refocused on going back, so we should do the same with the 3 dots!

We refocus the text input after closing the modal in Native and mWeb. If we remove the code, the text input will not get refocused even if we close the modal without taking any action.

sobitneupane avatar Feb 05 '24 12:02 sobitneupane

Still accepting proposals!

Christinadobrzyn avatar Feb 06 '24 00:02 Christinadobrzyn

Proposal

Updated

@sobitneupane I have added the details of alternative proposal, please check it out.

samilabud avatar Feb 07 '24 14:02 samilabud

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Feb 07 '24 16:02 melvin-bot[bot]

@sobitneupane can you review this proposal when possible? https://github.com/Expensify/App/issues/35494#issuecomment-1932120773

Christinadobrzyn avatar Feb 08 '24 23:02 Christinadobrzyn

@sobitneupane, @Christinadobrzyn Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Feb 12 '24 15:02 melvin-bot[bot]

I will review the proposal asap.

sobitneupane avatar Feb 12 '24 15:02 sobitneupane

@samilabud Thanks for the update.

But I am quite confused about the solution. It looks like a work around to me.

What we want is to prevent the keyboard to close and reopen after leaving the thread. Can you please re-sate the problem, the root cause and how your solution is going to deal with the root cause. Please do explain your solution as well.

sobitneupane avatar Feb 13 '24 11:02 sobitneupane

Proposal

Updated

@sobitneupane I have explained the solution better, please let me know if it makes sense to you.  🙏🏼

samilabud avatar Feb 13 '24 13:02 samilabud

@sobitneupane @Christinadobrzyn this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Feb 14 '24 15:02 melvin-bot[bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Feb 14 '24 16:02 melvin-bot[bot]

hi @sobitneupane can you take a peek at the updated proposal or let me know if you'd like me to raise the price to get more proposals?

Christinadobrzyn avatar Feb 14 '24 18:02 Christinadobrzyn

@samilabud Thanks for your proposal. I am not sure if we want to autofocus the Composer of the report after user leaves the thread.

@Christinadobrzyn Do we want to autofocus the Composer of the report after user leaves the thread on mWeb?

sobitneupane avatar Feb 15 '24 16:02 sobitneupane

Hi @sobitneupane oh that is a good question, it looks like we autofocus the composer on the other platforms so I think that would be the expectation on Android too. Example on NewDot app

https://github.com/Expensify/App/assets/51066321/baccce2f-e272-4e79-b459-de379f545761

Christinadobrzyn avatar Feb 16 '24 20:02 Christinadobrzyn

@Christinadobrzyn I think we should compare it with Native apps rather than Web. I will test it on Android and update here.

sobitneupane avatar Feb 20 '24 09:02 sobitneupane

In Android/Native, we don't open keyboard on leaving the thread.

https://github.com/Expensify/App/assets/25876548/d4adfe6b-d542-43ba-8f8b-8eaa902757a7

sobitneupane avatar Feb 20 '24 09:02 sobitneupane

Oh perfect - good thinking @sobitneupane! Thank you for testing!

Christinadobrzyn avatar Feb 20 '24 15:02 Christinadobrzyn

@sobitneupane @Christinadobrzyn this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] avatar Feb 21 '24 15:02 melvin-bot[bot]

@Christinadobrzyn We would want this behavior in mWeb as well, right?

sobitneupane avatar Feb 21 '24 15:02 sobitneupane

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Feb 21 '24 16:02 melvin-bot[bot]

yeah I think we want the same behaviour across all platforms. But I think this 'keyboard issue' would only affect the mobile apps right?

Christinadobrzyn avatar Feb 21 '24 21:02 Christinadobrzyn

Just checking in, do you need anything @sobitneupane?

Christinadobrzyn avatar Feb 24 '24 00:02 Christinadobrzyn

hi @sobitneupane can you lend an update? Should I reach out to SWM or Callstack to see if they might be able to help?

Christinadobrzyn avatar Feb 26 '24 21:02 Christinadobrzyn