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 2023-03-01] [$1000] Android - Keyboard is not dismissed after tapping outside of email input #14569

Closed
1 task
kbecciv opened this issue Jan 25, 2023 · 64 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 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Jan 25, 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. Open NewDot app
  2. Tap email input box
  3. Tap outside of email input

Expected Result:

Keyboard should be dismissed

Actual Result:

Keyboard is not dismissed after tapping outside of email input
Verify that it does not revert this issue - #13920 (comment)
The language picker should still be clickable in sign in page

Workaround:

Unknown

Platforms:

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

  • Android / native

Version Number: 1.2.59.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/3101971

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5911751_Screenrecorder-2023-01-25-20-05-33-421-1.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/~010f80df51934e8bf6
  • Upwork Job ID: 1620601345048166400
  • Last Price Increase: 2023-02-01
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 25, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 25, 2023
@mateocole
Copy link

This looks like something we will want to fix, tagging engineering to take a look

@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label Feb 1, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 1, 2023
@melvin-bot melvin-bot bot changed the title Android - Keyboard is not dismissed after tapping outside of email input [$1000] Android - Keyboard is not dismissed after tapping outside of email input Feb 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Feb 1, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Feb 1, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Feb 1, 2023

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

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

melvin-bot bot commented Feb 1, 2023

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

@bernhardoj
Copy link
Contributor

I think this is expected #13920 (comment).

@situchan
Copy link
Contributor

situchan commented Feb 1, 2023

This came from #13954 and I agree with @bernhardoj.
In android, keyboard can be dismissed using device back button so it's not affecting user experience.
cc: @jasperhuangg @tgolen

@mollfpr
Copy link
Contributor

mollfpr commented Feb 1, 2023

Thanks, @bernhardoj and @situchan! Let’s wait for the clarification.

@vitaliygerasymchuk
Copy link

Hello. As stated above - this is expected behavior. But still it can be improved.
The solution:

After the touch event - we get the focused view, remove focus from it and hide the keyboard.

@RUPESH1439
Copy link

RUPESH1439 commented Feb 1, 2023

PROPOSAL:

  1. Wrap the screen component using TouchableWithoutFeedback with onPress handler to dismiss the active keyboard as (Keyboard.dismiss)

Code sample:

<TouchableWithoutFeedback onPress={Keyboard.dismiss}>
      <View style={styles.container}>
        {/* Your content here */}
      </View></TouchableWithoutFeedback>

@akhilesh-mourya
Copy link

akhilesh-mourya commented Feb 1, 2023

PROPOSAL:
We will have to wrap the input form inside KeyboardAwareScrollViewContainer according to this npm wrapper:
https://github.com/APSL/react-native-keyboard-aware-scroll-view

this is a maintained library with thousands of stars

We can also use Formik for form validation and other handlings.

@Naishadh8115
Copy link

PROPOSAL :
@mateocole There is one file missing for android index.android.js in one component called in TouchableDismissKeyboard. I have tested it by running it. I can help with this issue. I have fixed it at my end on android.

@thuyle04
Copy link

thuyle04 commented Feb 2, 2023

PROPOSAL
We can edit file index.js same as content of file index.ios.js . It uses TouchableWithoutFeedback and event onPress to dismiss keyboard when touching outside email input.

Files are fixed
image
image

@mollfpr
Copy link
Contributor

mollfpr commented Feb 2, 2023

@bernhardoj @situchan I think we should make the same behavior on native platforms, no?

@thienlnam What do you think about the comments #14569 (comment) and #14569 (comment)? Are we going close this or continue to fix the issue?

@Naishadh8115
Copy link

PROPOSAL : @mateocole There is one file missing for android index.android.js in one component called in TouchableDismissKeyboard. I have tested it by running it. I can help with this issue. I have fixed it at my end on android.
As mentioned above #14569 (comment)

Screen_Recording_20230202_175644_New.Expensify.mp4

@thienlnam thienlnam removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 2, 2023
@thienlnam
Copy link
Contributor

@mollfpr

I agree with you here, I see that it was removed to fix this

This is some weird quirk with RNPickerSelect where when you enclose them with Touchables it makes them untouchable

But like you mentioned, our philosophy is that it should be expected to work the same way on all platforms so we should figure out another fix for this

Whatever proposal you accept should also verify that the language picker is still clickable on the sign in page

@thienlnam thienlnam added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 2, 2023
@situchan
Copy link
Contributor

situchan commented Feb 2, 2023

Proposal:

It doesn't make sense that we make use of touchable onPress feature when keyboard is not shown.

Make changes in TouchableDismissKeyboard component:

  • rename index.ios.js to index.native.js
  • In new index.native.js:
const TouchableDismissKeyboard = (props) => {
    const dismissKeyboardWhenTappedOutsideOfInput = () => {
-       if (!props.isKeyboardShown) {
-           return;
-       }
        Keyboard.dismiss();
    };

    return (
-       <TouchableWithoutFeedback onPress={dismissKeyboardWhenTappedOutsideOfInput}>
+       <TouchableWithoutFeedback disabled={!props.isKeyboardShown} onPress={dismissKeyboardWhenTappedOutsideOfInput}>
            {props.children}
        </TouchableWithoutFeedback>
    );
};

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 22, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.75-0 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 2023-03-01. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@MelvinBot

This comment was marked as spam.

@MelvinBot
Copy link

MelvinBot commented Feb 22, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@mollfpr / @thienlnam] The PR that introduced the bug has been identified. Link to the PR:
  • [@mollfpr / @thienlnam] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@mollfpr / @thienlnam] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@situchan] Propose regression test steps to ensure the same bug will not reach production again.
  • [@mateocole] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@mollfpr
Copy link
Contributor

mollfpr commented Feb 23, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 1, 2023
@mollfpr
Copy link
Contributor

mollfpr commented Mar 3, 2023

@situchan could you propose the regression test?

@situchan
Copy link
Contributor

situchan commented Mar 3, 2023

Regression Test Proposal

  • Bug: [Android] Keyboard is not dismissed after tapping outside of email input

  • Proposed Test Steps:

    • Open NewDot app
    • Logout if already logged in
    • Tap email input box on Login page
    • Tap outside of email input
    • Verify that keyboard is dismissed
  • Bug: [Android] Sign in - Language picker is not clickable in sign in page

  • Proposed Test Steps:

    • Open NewDot app
    • Logout if already logged in
    • Tap email input box on Login page
    • Tap language picker at the bottom
    • Verify that language dropdown shows directly
  • Do we agree 👍 or 👎

@mollfpr
Copy link
Contributor

mollfpr commented Mar 3, 2023

👍

@melvin-bot melvin-bot bot added the Overdue label Mar 6, 2023
@MelvinBot
Copy link

@mollfpr, @mateocole, @thienlnam, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

1 similar comment
@MelvinBot
Copy link

@mollfpr, @mateocole, @thienlnam, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

@mollfpr
Copy link
Contributor

mollfpr commented Mar 7, 2023

Bump @mateocole

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 7, 2023
@thienlnam
Copy link
Contributor

Adding a note here for Matt, this issue is also eligible for the 50% bonus

(We updated the solution later but it was still within 3 business days that it was decided so all good)

@melvin-bot melvin-bot bot removed the Overdue label Mar 9, 2023
@mateocole
Copy link

creating a regression test!

@mateocole
Copy link

@situchan and @mollfpr just to confirm you have not applied for an upwork job yet, correct?

@situchan
Copy link
Contributor

situchan commented Mar 9, 2023

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

@mateocole This job is expired so cannot apply. Can you please post new job and invite me?

@mateocole
Copy link

@situchan
Copy link
Contributor

@mateocole applied. Thank you

@mateocole
Copy link

Okay! Contracts sent

@mateocole
Copy link

mateocole commented Mar 10, 2023

Alright both payments sent with bonus!

@melvin-bot melvin-bot bot added the Overdue label Mar 13, 2023
@melvin-bot melvin-bot bot removed the Overdue label Mar 13, 2023
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 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests