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-09-04] [$4000] Keyboard shows again briefly when clicking "Add attachment" while the composer is focused #13922

Closed
2 tasks done
kavimuru opened this issue Jan 1, 2023 · 253 comments
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 Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Jan 1, 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. Open chat
  2. Focus the composer such that the keyboard is open
  3. click plus icon in the composer
  4. click Add Attachment

Expected Result:

They keyboard doesn't reopen briefly, such that it's a smooth animation to the next modal screen with the attachment upload options

Actual Result:

The keyboard shows again briefly before the modal with the attachment upload options appears, resulting in what looks like the screen flickering

Workaround:

Unknown

Platforms:

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

  • iOS / native
  • Android / Chrome

Version Number: v1.2.49.0
Reproducible in staging?: Y
**Reproducible in production?:**Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

RPReplay_Final1672422381.MP4
VEQI0550.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01eb1d86a3237e2dfd
  • Upwork Job ID: 1611147936271515648
  • Last Price Increase: 2023-07-27
  • Automatic offers:
    • situchan | Contributor | 26109669
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 1, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 1, 2023
@jasperhuangg jasperhuangg self-assigned this Jan 2, 2023
@jasperhuangg jasperhuangg removed their assignment Jan 2, 2023
@jasperhuangg
Copy link
Contributor

jasperhuangg commented Jan 2, 2023

Was a bit too quick to assign myself, @kavimuru does this only happen on iOS, or does this also happen on Android/Mobile Web?

I feel like this might be related to the logic here

cc @parasharrajat

@kavimuru
Copy link
Author

kavimuru commented Jan 3, 2023

@jasperhuangg not able to repro in Android and mWeb

az_recorder_20230103_125527.1.mp4

@grgia
Copy link
Contributor

grgia commented Jan 3, 2023

@kavimuru did you test android with the keyboard open as well?

@trjExpensify
Copy link
Contributor

Hm, what exactly is happening here? We should make the OP as descriptive as we can versus "janky". I can't quite catch what page is opening after the Add attachment button press?

Also, the screen recording above of Android seems to have the same problem of a brief flicker on press doesn't it?

I feel like we've had a similar problem before with the copy to clipboard modal momentarily showing, but it doesn't seem like that's what is showing here.

@trjExpensify
Copy link
Contributor

@jasperhuangg are you taking this one on?

@grgia
Copy link
Contributor

grgia commented Jan 5, 2023

Looks like the keyboard isn't closing when pressed and it's reopening for a single frame which is causing the flashing.

Upload.from.GitHub.for.iOS.MOV

@grgia
Copy link
Contributor

grgia commented Jan 5, 2023

If I scrub the video it's the keyboard during that flash

image

@trjExpensify trjExpensify changed the title Screen jumps when clicking add attachment while keyboard is open Keyboard shows again briefly when clicking "Add attachment" while the composer is focused Jan 5, 2023
@trjExpensify
Copy link
Contributor

Nice, okay. I've updated the OP and issue title to be more descriptive and reflect the problem at hand. @jasperhuangg, can you let us know today if you want to take this on? If not, let's open it up with the External label.

@jasperhuangg
Copy link
Contributor

Feel free to assign External! Have some customer issues I need to get to this week. Thanks for the bump!

@trjExpensify trjExpensify added the External Added to denote the issue can be worked on by a contributor label Jan 5, 2023
@melvin-bot melvin-bot bot unlocked this conversation Jan 5, 2023
@melvin-bot melvin-bot bot changed the title Keyboard shows again briefly when clicking "Add attachment" while the composer is focused [$1000] Keyboard shows again briefly when clicking "Add attachment" while the composer is focused Jan 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 5, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jan 5, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jan 5, 2023

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

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

melvin-bot bot commented Jan 5, 2023

Triggered auto assignment to @joelbettner (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@priyeshshah11
Copy link
Contributor

priyeshshah11 commented Jan 6, 2023

Proposal

Root cause

This issue occurs because iOS by default dismisses the Keyboard before opening any modals and it remembers the keyboard's state and reopens the keyboard when the modal is dismissed/closed which is ideal for most scenarios. However, this does not work well when you're switching between two modals as iOS tries to reopen the keyboard as soon as you dismiss the first modal and then tries to dismiss the keyboard again as soon as the second modal is opened which causes this "janky" effect.

Solution

I think the ideal scenario here is to dismiss the keyboard ourselves before we open the first modal, that way native code thinks that the keyboard was already closed prior to opening that modal and won't try to reopen & close the keyboard between modal transitions.

The downside of this is that the keyboard won't open back again automatically once the attachment is selected which IMO is not big issue and it comes down to choice. If we still want to open that keyboard back after the attachment being selected, we can do that by remembering the keyboard state ourselves and reopen it if it was open at the start. However, IMO it might be an overkill but happy to hear other's thoughts on it.

Solution 1 - Dismiss keyboard before modal opens

I tried to avoid setTimeout by using InteractionManager but even that doesn't work in this case thus had to resort to setTimeout with 500ms minimum required. The other alternative we can use is the keyboardDidHide event but that involves keeping track of the keyboard state.

diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index 7811d1c25..89a726099 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -4,6 +4,7 @@ import {
     View,
     TouchableOpacity,
     InteractionManager,
+    Keyboard,
 } from 'react-native';
 import _ from 'underscore';
 import lodashGet from 'lodash/get';
@@ -601,11 +602,12 @@ class ReportActionCompose extends React.Component {
                              <TouchableOpacity
                                   ref={el => this.actionButton = el}
                                   onPress={(e) => {
+                                       Keyboard.dismiss();
                                        e.preventDefault();

                                        // Drop focus to avoid blue focus ring.
                                        this.actionButton.blur();
-                                       this.setMenuVisibility(true);
+                                       setTimeout(() => this.setMenuVisibility(true), 500);
                                   }}
                                   style={styles.chatItemAttachButton}
                                   underlayColor={themeColors.componentBG}

Solution 2 - Dismiss keyboard before modal opens & reopen after selecting attachment action is completed

We can achieve this by keeping track of the keyboard state in this component by adding keyboard event listeners and then along with Solution 1 we can reopen the keyboard by focusing the composer input once attachment picker is closed.

I can post a code solution later if there's interest in this solution.

Video

keyboard.mov

@thesahindia @trjExpensify

@melvin-bot melvin-bot bot added the Overdue label Jan 9, 2023
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Aug 29, 2023
@ntdiary
Copy link
Contributor

ntdiary commented Aug 29, 2023

That PR fixes #25872, right? No doubt then

@situchan, yeah, but issue #25872 was found when testing PR #23994. I guess they felt the actual behavior on physical devices was inconsistent with the expected behavior we described in the PR #23994. For this situation, should you review the new PR #26221 again next? If not, I'm also not sure if a new reviewer would get paid the bounty for the new issue if they review it. Any thoughts would be much appreciated
image

cc @Christinadobrzyn

@situchan
Copy link
Contributor

Personally I don't think #25872 is worth fixing for now and suggest to add it to the composer focus tracking issue - #15992

@ntdiary
Copy link
Contributor

ntdiary commented Aug 31, 2023

Personally I don't think #25872 is worth fixing for now and suggest to add it to the composer focus tracking issue - #15992

@Christinadobrzyn, hi, please feel free to share your thoughts on this suggestion. If it's okay, we can go ahead and close PR #26221 for now. 🙂

@parasharrajat
Copy link
Member

The solution is pretty quick so let's fix that. Remember the app will be used on real devices, not simulators so it matters that we fix the behavior for real devices. The file input behavior is unlikely to be changed in the focus tracking issue so it is worth fixing.

cc: @situchan

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 Daily KSv2 labels Sep 4, 2023
@trjExpensify
Copy link
Contributor

trjExpensify commented Sep 7, 2023

Okay, so I think we're ready to settle up here now? It has a bit of a colourful history this issue, but this is where we ultimately landed to get it over the line:

$4,000 - @ntdiary for the bug fix
$4,000 - @situchan for the C+ review
Georgia reported it, so no bug reporter in play.
$500 - @tienifr for the original groundwork
$500 - @priyeshshah11 for the original groundwork.

Let me know and I'll send the offers :)

@parasharrajat
Copy link
Member

Note: @trjExpensify There is a pending PR above.

@ntdiary
Copy link
Contributor

ntdiary commented Sep 7, 2023

@trjExpensify, I'm fine with the bounty. And there is indeed another PR #26221 aimed at reopening the keyboard when returning from the gallery page. However it seems we have a feature freeze this week? So I hope it can be reviewed by @situchan next week asap, and then @Christinadobrzyn can test it again for issue #25872. 🙂

@priyeshshah11
Copy link
Contributor

Okay, so I think we're ready to settle up here now? It has a bit of a colourful history this issue, but this is where we ultimately landed to get it over the line:

$4,000 - @ntdiary for the bug fix $4,000 - @situchan for the C+ review Georgia reported it, so no bug reporter in play.

Let me know and I'll send the offers :)

@trjExpensify just checking, so no compensation for the initial solution & a prolonged effort in this case?

@trjExpensify
Copy link
Contributor

Note: @trjExpensify There is a pending PR above.

Noted, will hold off for that!

@trjExpensify just checking, so no compensation for the initial solution & a prolonged effort in this case?

Ordinarily there wouldn't be, because the PR was closed as an incomplete proposal due to the side effect with proposed solution. I do appreciate this was a long one, so I'm happy to proceed with 25% of the issue bounty for that. IIRC, you and @tienifr were splitting that, right?

@oakmegaeddie
Copy link

I'm looking for the solution for #38633, and I the issue was mentioned in this issue.

I'd like to know does the final solution still has this problem or not?

  1. When the keyboard is shown.
  2. Set a Modal to visible, it will dimiss the keyboard.
  3. Set the Modal to invisible.

After the 3rd step, although the keyboard is not show (maybe a little... I don't know), it will trigger following keyboard events: keyboardWillShow, keyboardDidShow, keyboardWillHide, keyboardDidHide.

@melvin-bot
Copy link

melvin-bot bot commented Sep 13, 2023

📣 @oakmegaeddie! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@oakmegaeddie
Copy link

Contributor details
Your Expensify account email: eddie.hsu@oakmega.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01cddf25d16f8c4eb1

@melvin-bot
Copy link

melvin-bot bot commented Sep 13, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

@ntdiary, @joelbettner, @trjExpensify, @priyeshshah11, @thesahindia, @situchan, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

@situchan
Copy link
Contributor

Pending PR was deployed to production

@trjExpensify
Copy link
Contributor

Cool, thanks! Following through with the payments listed here, offers have been sent to you all.

@situchan
Copy link
Contributor

@trjExpensify sorry, I already have contract accepted from Melvin offer - #13922 (comment)

@trjExpensify
Copy link
Contributor

Yeah, the job is closed though unfortunately, so I'd have to rehire you from that anyway. Can you accept the new one please?

@trjExpensify
Copy link
Contributor

@situchan - paid!

@priyeshshah11
Copy link
Contributor

@trjExpensify just checking, so no compensation for the initial solution & a prolonged effort in this case?

Ordinarily there wouldn't be, because the PR was closed as an incomplete proposal due to the side effect with proposed solution. I do appreciate this was a long one, so I'm happy to proceed with 25% of the issue bounty for that.

Thanks @trjExpensify but sorry I disagree as there have been issues where people have been paid more than this, when a mix of ideas have lead to the final solution but its ok I'll accept that.

IIRC, you and @tienifr were splitting that, right?

And yes @tienifr & I agreed to share the issue but his input stopped since March whereas I worked on it till mid July.

@tienifr
Copy link
Contributor

tienifr commented Sep 20, 2023

Cool, thanks! Following through with the payments listed #13922 (comment), offers have been sent to you all.

Accepted, thanks!

And yes @tienifr & I agreed to share the issue but his input stopped since March whereas I worked on it till mid July.

I think most of our work are from early Jan beginning up until March 31. After that there's mostly back-and-forth communication with no additional solution being given.

@trjExpensify
Copy link
Contributor

@ntdiary - paid
@tienifr - paid

Thanks @trjExpensify but sorry I disagree as there have been issues where people have been paid more than this, when a mix of ideas have lead to the final solution but its ok I'll accept that.

Sure, there have also been issues where people have been paid less or $0 when the PR is closed for ultimately not being the right solution. It's the nature of being a discretionary payment, and a relatively rare occurrence in the grand scheme of things. Thanks for accepting!

@trjExpensify
Copy link
Contributor

Settled up, closing!

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 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests