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

[WAITING ON SLACK] [$1000] ANDROID CHROME - Missing focus when going from LHN to the conversation page #23382

Closed
2 of 6 tasks
lanitochka17 opened this issue Jul 21, 2023 · 18 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 21, 2023

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:

  1. Go to http://staging.new.expensify.com/
  2. Tap on any conversation from LHN
    Another scenario
  3. Go to http://staging.new.expensify.com/
  4. Tap on any conversation from LHN
  5. Type a letter in compose box
  6. Navigate to LHN
  7. Return to compose box

Expected Result:

While navigating from LHN to chat page, focus on compose box, with cursor and keypad must be shown

Actual Result:

While navigating from LHN to chat page, no focus on compose box, no cursor and no keypad displayed

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • Windows / Chrome
  • MacOS / Desktop

Version Number: 1.3.44.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug6129109_aa_compress_1.mp4
Bug6129109_cursor.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01256fc687c8baf62f
  • Upwork Job ID: 1682564295686647808
  • Last Price Increase: 2023-07-29
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 21, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 21, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@makiour
Copy link
Contributor

makiour commented Jul 22, 2023

Proposal

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

When clicking on a chat in the sidebar, the focus is not anymore shown.

What is the root cause of that problem?

This is actually the normal behavior of ReactDOM. The focus event listener is triggered automatically when clicking anywhere in the 'document'. This causes the focus to clear itself from the composer and tries to focus where we have the mouse clicked event.
Basically, the issue is that we are not handling manually to keep the focus when clicking on the sidebar.

The lines of code that trigger the click event on the OptionRowLHN or Chat is in the SidebarLinks file where we navigate to the report and open it when we click:

showReportPage(option) {
// Prevent opening Report page when clicking LHN row quickly after clicking FAB icon
// or when clicking the active LHN row
// or when continuously clicking different LHNs, only apply to small screen
// since getTopmostReportId always returns on other devices
if (this.props.isCreateMenuOpen || this.props.currentReportID === option.reportID || (this.props.isSmallScreenWidth && Navigation.getTopmostReportId())) {
return;
}
Navigation.navigate(ROUTES.getReportRoute(option.reportID));
this.props.onLinkClick();

Dealing with the composer in independent of openingReport request and Report rendering component. We can't pass something to let the composer present here that it needs to be focused.

<Composer
checkComposerVisibility={() => this.checkComposerVisibility()}
autoFocus={this.shouldAutoFocus}
multiline
ref={this.setTextInputRef}
textAlignVertical="top"
placeholder={inputPlaceholder}
placeholderTextColor={themeColors.placeholderText}
onChangeText={(comment) => this.updateComment(comment, true)}
onKeyPress={this.triggerHotkeyActions}
style={[styles.textInputCompose, this.props.isComposerFullSize ? styles.textInputFullCompose : styles.flex4]}
maxLines={maxComposerLines}
onFocus={() => this.setIsFocused(true)}
onBlur={() => {
this.setIsFocused(false);
this.resetSuggestions();
}}
onClick={() => {
this.shouldBlockEmojiCalc = false;
this.shouldBlockMentionCalc = false;
}}
onPasteFile={displayFileInModal}
shouldClear={this.state.textInputShouldClear}
onClear={() => this.setTextInputShouldClear(false)}
isDisabled={isBlockedFromConcierge || this.props.disabled}

Thus, the solution here is that we can work it out through the ReportActionComposeFocusManager that keeps track of the focusCallback. When navigating to the selected report, we can add the focus listener to the compose which will keep it on!

In addition, another reason here is the we are re-navigating to report when we click on the chat as well. This is causing the component to update one again and loosing the focus, so we can manually trigger it to focus again *Will discuss it in the alternative solutions.

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

In this part of the SidebarLinks:

showReportPage(option) {
// Prevent opening Report page when clicking LHN row quickly after clicking FAB icon
// or when clicking the active LHN row
// or when continuously clicking different LHNs, only apply to small screen
// since getTopmostReportId always returns on other devices
if (this.props.isCreateMenuOpen || this.props.currentReportID === option.reportID || (this.props.isSmallScreenWidth && Navigation.getTopmostReportId())) {
return;
}
Navigation.navigate(ROUTES.getReportRoute(option.reportID));
this.props.onLinkClick();

We need to add:
ReportActionComposeFocusManager.focus(); which will trigger the composer to focus each time we click on the OptionRowLHN

I hope I made it as clear as possible for you!

Result

bugFocus.webm

What alternative solutions did you explore? (Optional)

As I stated above, we can handle this as well from the component when it is updated in React, we can trigger it to focus once again in here:

componentDidUpdate(prevProps) {
// We want to focus or refocus the input when a modal has been closed or the underlying screen is refocused.
// We avoid doing this on native platforms since the software keyboard popping
// open creates a jarring and broken UX.
if (this.willBlurTextInputOnTapOutside && !this.props.modal.isVisible && this.props.isFocused && (prevProps.modal.isVisible || !prevProps.isFocused)) {
this.focus();
}
// Value state does not have the same value as comment props when the comment gets changed from another tab.
// In this case, we should synchronize the value between tabs.
const shouldSyncComment = prevProps.comment !== this.props.comment && this.state.value !== this.props.comment;
// As the report IDs change, make sure to update the composer comment as we need to make sure
// we do not show incorrect data in there (ie. draft of message from other report).
if (this.props.report.reportID === prevProps.report.reportID && !shouldSyncComment) {
return;
}
this.updateComment(this.props.comment);

We can add this.focus() and ignoring the condition, and this will mitigate the issue in here as well.

@jliexpensify
Copy link
Contributor

Yep, I can replicate it on 1.3.44.0 (Android Chrome, Pixel 3a). The behaviour on Mac OS is that the cursor re-appears when you navigate back into the same compose field in the same chat.

@jliexpensify jliexpensify changed the title Chat - Missing focus when going from LHN to the conversation page ANDROID CHROME - Missing focus when going from LHN to the conversation page Jul 22, 2023
@jliexpensify jliexpensify added the External Added to denote the issue can be worked on by a contributor label Jul 22, 2023
@melvin-bot melvin-bot bot changed the title ANDROID CHROME - Missing focus when going from LHN to the conversation page [$1000] ANDROID CHROME - Missing focus when going from LHN to the conversation page Jul 22, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 22, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 22, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 22, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 22, 2023

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

@Nathan-Mulugeta
Copy link

Hey guys I think I have reported this issue long ago here

@samh-nl
Copy link
Contributor

samh-nl commented Jul 22, 2023

Is it good UX to automatically open the keyboard on mobile, which takes up a large chunk of the screen? The first thing the user probably wants to do is read the messages, rather than replying (which feels like should be an explicit action) that now we're kind of forcing onto the user.

@jliexpensify
Copy link
Contributor

@Nathan-Mulugeta - thanks for letting me know! @situchan - let me bring this one up internally. I reviewed Nathan's bug report and Matt closed it as not being a bug, but my understanding is our behaviours should be the same across all platforms.

I think this is a bug since we can see the behaviour on Mac OS, but I'll circle back with a final response.

@jliexpensify
Copy link
Contributor

@jliexpensify jliexpensify changed the title [$1000] ANDROID CHROME - Missing focus when going from LHN to the conversation page [WAITING ON SLACK] [$1000] ANDROID CHROME - Missing focus when going from LHN to the conversation page Jul 23, 2023
@melvin-bot melvin-bot bot added the Overdue label Jul 26, 2023
@jliexpensify
Copy link
Contributor

Still awaiting consensus

@melvin-bot melvin-bot bot removed the Overdue label Jul 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 29, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Jul 29, 2023
@jliexpensify
Copy link
Contributor

Sorry, not overdue - we're discussing auto-focusing in Slack. Catching up on the threads today.

@melvin-bot melvin-bot bot removed the Overdue label Jul 31, 2023
@jliexpensify
Copy link
Contributor

@situchan just curious: does Android support auto-focusing in this particular instance (i.e. the chat composer)?

@situchan
Copy link
Contributor

@situchan just curious: does Android support auto-focusing in this particular instance (i.e. the chat composer)?

yes, it should be auto-focused when open new (no chat history yet) report

@jliexpensify
Copy link
Contributor

@situchan as an FYI, I've posted publicly here - https://expensify.slack.com/archives/C01GTK53T8Q/p1690436065927919

It looks like we never really defined the rules around auto-focus so I was trying to get consensus around it. Feel free to review!

@jliexpensify
Copy link
Contributor

@Nathan-Mulugeta @situchan - sorry for the delay: we're going to close this one as both iOS and Android don't intentionally auto-focus on the chat composer, and we want to keep it this way.

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

6 participants