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] [$1000] mWeb - mWeb - Keyboard opens automatically after going back from attachment #23489

Closed
1 of 6 tasks
kbecciv opened this issue Jul 24, 2023 · 25 comments
Closed
1 of 6 tasks
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

Comments

@kbecciv
Copy link

kbecciv commented Jul 24, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Go to any chat
  2. Send any image as attachment.
  3. Click on image to view it.
  4. Click back arrow to go back to chat.
  5. Keyboard opens automatically

Expected Result:

Keyboard should not open automatically.

Actual Result:

Keyboard opens automatically

Workaround:

Unknown

Platforms:

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

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

Version Number:
Reproducible in staging?:
Reproducible in production?:
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

WhatsApp.Video.2023-07-23.at.11.21.06.PM.mp4
Screen_Recording_20230724_103858_Chrome.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Talha345
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690147484102679

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011c973cf5ceb3c3f7
  • Upwork Job ID: 1683583847311867904
  • Last Price Increase: 2023-07-31
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 24, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 24, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 24, 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

@samh-nl
Copy link
Contributor

samh-nl commented Jul 24, 2023

Proposal

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

The keyboard automatically opens when returning from selecting an attachment.

What is the root cause of that problem?

When the window regains focus, as measured by this.props.isFocused, the composer is focused again:

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();
}

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

We should prevent this behavior for mobile web users, similar to the behavior on native where this.willBlurTextInputOnTapOutside is false. On mobile web, we can determine this using !DeviceCapabilities.canUseTouchScreen().

I do question whether the correct variable is used here and whether it should not be this.shouldFocusInputOnScreenFocus instead, as we are trying to focus here and not blur the input.

This variable has the value !DeviceCapabilities.canUseTouchScreen() (web/mWeb) and false on native, which would lead to a correct situation also.

What alternative solutions did you explore? (Optional)

We could implement specific logic for preventing this behavior when returning from selecting an attachment. However, a general solution may be more desired as this behavior seems unwanted on mobile.

@stephanieelliott
Copy link
Contributor

Was able to repro.

@stephanieelliott stephanieelliott added the External Added to denote the issue can be worked on by a contributor label Jul 24, 2023
@melvin-bot melvin-bot bot changed the title mWeb - mWeb - Keyboard opens automatically after going back from attachment [$1000] mWeb - mWeb - Keyboard opens automatically after going back from attachment Jul 24, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 24, 2023

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

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

melvin-bot bot commented Jul 24, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 24, 2023

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

@s-alves10
Copy link
Contributor

@stephanieelliott @fedirjh

Refocusing on main composer opens the keyboard. I think this is an expected behavior

@Talha345
Copy link
Contributor

@stephanieelliott @fedirjh

Refocusing on main composer opens the keyboard. I think this is an expected behavior

Nope, as we had the same issue on native platforms previously and we disabled it.This is not the expected behaviour.Only when the user taps the composer, only then the keyboard should open.

@samh-nl
Copy link
Contributor

samh-nl commented Jul 26, 2023

On native mobile this.willBlurTextInputOnTapOutside is defined as false, so the behavior should not be present there. It would seem logical to apply this consistently to mWeb also.

@situchan
Copy link
Contributor

I think composer autofocus issues on mWeb can be handled in #23382.

@fedirjh
Copy link
Contributor

fedirjh commented Jul 26, 2023

@situchan It looks like they are separate bugs, However, for this issue, I think this is the expected behavior. cc @stephanieelliott, looking at this comment, it seems like this is the expected, Focusing the input will open the keyboard on non-native platforms.

// 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.

@Talha345
Copy link
Contributor

@situchan It looks like they are separate bugs, However, for this issue, I think this is the expected behavior. cc @stephanieelliott, looking at this comment, it seems like this is the expected, Focusing the input will open the keyboard on non-native platforms.

// 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 you read the comment,it is mentioned that this same issue existed on native platforms and was solved using this code.If we are fixing it on native platforms,why not on mWeb?

@samh-nl
Copy link
Contributor

samh-nl commented Jul 26, 2023

It seems to be a regression. I suspected the wrong variable was used here (see proposal, this.shouldFocusInputOnScreenFocus adds the correct behavior on native/mWeb).

Originally this variable was used:
https://github.com/Expensify/App/blame/4884bdc5fd3cc6889bb14286e6b137bc5c075504/src/pages/home/report/ReportActionCompose.js#L180-L183

Then later it was changed to this.shouldOverrideRnFocusBehavior and that was renamed to this.willBlurTextInputOnTapOutside.

However, other usages of the old variable this.shouldOverrideRnFocusBehavior are currently using this.shouldFocusInputOnScreenFocus.

@melvin-bot melvin-bot bot added the Overdue label Jul 28, 2023
@stephanieelliott
Copy link
Contributor

This feels like expected behavior to me too... I don't know enough about the RN variable to be certain though. @fedirjh based on the above, do you still agree this is not a bug?

@melvin-bot melvin-bot bot removed the Overdue label Jul 28, 2023
@Talha345
Copy link
Contributor

This feels like expected behavior to me too... I don't know enough about the RN variable to be certain though. @fedirjh based on the above, do you still agree this is not a bug?

@stephanieelliott Think from the perspective of a user using the platform.
They open a chat and scroll through it and every time they open an image attachment to preview it and close it,the keyboard pops up.Feels like buggy behavior to me at least.

@fedirjh
Copy link
Contributor

fedirjh commented Jul 30, 2023

based on the above, do you still agree this is not a bug?

I don’t think this is a bug.

Feels like buggy behavior to me at least.

The behavior should be fixed by #15992 , So I believe we should hold this one for [Tracking] Fix Composer Component Focus Issues

Edit: This is a similar issue #16596 that was held for #15992.

@melvin-bot
Copy link

melvin-bot bot commented Jul 31, 2023

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

@stephanieelliott
Copy link
Contributor

I agree with @fedirjh, it feels like polish if anything. Taking Help Wanted off this one and adding it to the tracking issue.

@stephanieelliott stephanieelliott added Monthly KSv2 and removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 1, 2023
@stephanieelliott stephanieelliott changed the title [$1000] mWeb - mWeb - Keyboard opens automatically after going back from attachment [HOLD] [$1000] mWeb - mWeb - Keyboard opens automatically after going back from attachment Aug 1, 2023
@melvin-bot melvin-bot bot added the Overdue label Aug 3, 2023
@stephanieelliott stephanieelliott removed the Daily KSv2 label Aug 4, 2023
@stephanieelliott
Copy link
Contributor

Not overdue, just removed the Daily label since this is on hold

@melvin-bot melvin-bot bot removed the Overdue label Aug 4, 2023
@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2023
@fedirjh
Copy link
Contributor

fedirjh commented Sep 4, 2023

Still on hold.

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2023
@melvin-bot melvin-bot bot added the Overdue label Oct 5, 2023
@stephanieelliott
Copy link
Contributor

Still on hold.

@melvin-bot melvin-bot bot removed the Overdue label Oct 6, 2023
@melvin-bot melvin-bot bot added the Overdue label Nov 6, 2023
@fedirjh
Copy link
Contributor

fedirjh commented Nov 6, 2023

Still on hold for #15992

@melvin-bot melvin-bot bot removed the Overdue label Nov 6, 2023
@melvin-bot melvin-bot bot added the Overdue label Dec 8, 2023
@stephanieelliott
Copy link
Contributor

Still on hold

@melvin-bot melvin-bot bot removed the Overdue label Dec 8, 2023
@stephanieelliott
Copy link
Contributor

Closing this as I don't think it is a priority. It is linked to the master tracking issue for composer component polish (which has also been deprioritized), so we may reopen it when we decide to focus on that issue.

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
Projects
None yet
Development

No branches or pull requests

7 participants