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

[$500] New chat - Composed box is displayed in the middle of page when open a user that don't have any messages with #11858

Closed
kbecciv opened this issue Oct 14, 2022 · 46 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Oct 14, 2022

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 the app
  2. Log in with any account
  3. Click on the green + button
  4. Click on the "New Chat" option
  5. Search for a user that you don't have any messages with and open the chat.

Note: this seems to occur with other input fields. For example, this was reproducible for me after editing the first-name input field in Settings.

Expected Result:

  • There should not be padding beneath the input

Actual Result:

  • There is keyboard-sized vertical padding beneath the input

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Android
  • iOS

Version Number: 1.2.15.2

Reproducible in staging?: Yes

Reproducible in production?: No

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

Bug5777091_new_group_1410.mp4

Screenshot 2022-10-25 at 11 35 16

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Oct 14, 2022
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Oct 14, 2022

Triggered auto assignment to @PauloGasparSv (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@PauloGasparSv
Copy link
Contributor

Could not reproduce the error locally or in my physical devices using the latest App version from TestFlight.
Closing this blocker for now!

cc @yuwenmemon

@thesahindia
Copy link
Member

@PauloGasparSv, can we reopen this as it's reproducible (it was also reported here)
It is not consistent so hard to repro.

@PauloGasparSv
Copy link
Contributor

Hey @thesahindia! I think we should keep it closed till the P.R. #10648 gets merged because it will also be tested against iOS if that's ok.

I'll link this issue to the P.R.

@kbecciv
Copy link
Author

kbecciv commented Oct 17, 2022

@thesahindia @PauloGasparSv Issue is reproduced in latest build 1.2.16.4

RPReplay_Final1666016404.MP4

IMG_5063

@kbecciv kbecciv reopened this Oct 17, 2022
@PauloGasparSv
Copy link
Contributor

PauloGasparSv commented Oct 17, 2022

Thanks @kbecciv!
As mentioned here we are reopening this, despite of the similar issue #10539 (that happens on Android) as the root cause may not be the same as this one and this iOS issue was reproduced again as we can see in the comment above!

Marking this as external!

@PauloGasparSv PauloGasparSv added the External Added to denote the issue can be worked on by a contributor label Oct 17, 2022
@PauloGasparSv PauloGasparSv removed their assignment Oct 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2022

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 Oct 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2022

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

@melvin-bot melvin-bot bot changed the title iOS -New chat - Composed box is displayed in the middle of page when open a user that don't have any messages with [$250] iOS -New chat - Composed box is displayed in the middle of page when open a user that don't have any messages with Oct 17, 2022
@sketchydroide sketchydroide removed the DeployBlockerCash This issue or pull request should block deployment label Oct 17, 2022
@sketchydroide
Copy link
Contributor

removing the blocker as this was reported on the currently prod version.

@ntrepanier
Copy link

On hold until #11586 is merged based on this slack convo.

@melvin-bot melvin-bot bot removed the Overdue label Nov 16, 2022
@JmillsExpensify
Copy link

Ideally Margelo or an internal engineer will be taking this on soon!

@JmillsExpensify
Copy link

Removed the iPad app testing steps since tablets are not an officially supported platform.

@madmax330
Copy link
Contributor

Picking this up

@madmax330 madmax330 self-assigned this Nov 21, 2022
@madmax330 madmax330 added Internal Requires API changes or must be handled by Expensify staff and removed 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 labels Nov 21, 2022
@flodnv flodnv added Daily KSv2 and removed Weekly KSv2 labels Nov 21, 2022
@JmillsExpensify
Copy link

Thanks Maxence. Looks like we have a relatively fresh reproduction from Applause, though let us know if you have any questions.

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Third week) Can we close this?

@madmax330
Copy link
Contributor

I'm able to reproduce this 😄

The repro steps are:

  • Open the LHN
  • Click on a chat
  • Go back to the LHN
  • Go to the search page
  • Close the search page (without doing anything on it)
  • Click on a chat
  • 💥

@Julesssss Julesssss changed the title [$500] iOS -New chat - Composed box is displayed in the middle of page when open a user that don't have any messages with [$500] New chat - Composed box is displayed in the middle of page when open a user that don't have any messages with Nov 23, 2022
@Julesssss
Copy link
Contributor

I can't find the issue, but those reproduction steps exactly match what I'm seeing occasionally on the related Android issue. Updating to show that this occurs on ANdroid and iOS

@madmax330
Copy link
Contributor

So I initially thought that this was introduced in this PR: https://github.com/Expensify/App/pull/12169/files
Where we set the height of the container here:

parentViewHeight={this.state.skeletonViewContainerHeight}

But that is just a post screen shrinking action, so I'm 90% sure the KAV still thinks the keyboard is open even though it isn't, which is weird because we call Keyboard.dismiss() when we navigate away from the search page

@madmax330
Copy link
Contributor

Ok so I did some further investigation and it seems to me like it's definitely the KAV that's causing this.
I passed a console.log through the onLayout prop here:

Which is then called here when the KAV view changes: https://github.com/facebook/react-native/blob/3eb69f24ddc281f4bff2f250f88cb875d6d721d0/Libraries/Components/Keyboard/KeyboardAvoidingView.js#L125

And I added a console.log here:

That will print out the height of the view.

And I noticed that in the event caught by the KAV in onLayout the height is 769 (normal height), but in the component it is 392 (bug height)

So what that tell me is that the KAV added the padding when we re-opened the reports page, which means that this function is not returning early and the function is not returning 0 for the padding height: https://github.com/facebook/react-native/blob/3eb69f24ddc281f4bff2f250f88cb875d6d721d0/Libraries/Components/Keyboard/KeyboardAvoidingView.js#L130

I'm not sure what to do from here though...

Thoughts @tgolen ??

I will post in #expensify-open-source as well

@madmax330
Copy link
Contributor

@marcaaron
Copy link
Contributor

Great summary @madmax330 and amazing job reproducing and debugging this issue. It seems like it will happen anytime we navigate to a chat while the LHN is open under a modal that triggers the KAV e.g. I was able to reproduce also by doing:

  • New Room
  • Dismiss
  • Tap chat in LHN
  • 💥

So it seems like when we are using the KAV in a modal view that it somehow affects the initial state of the KAV on the ReportScreen. You can switch between various chats after that and it's still stuck open.

@marcaaron
Copy link
Contributor

Traced this one back to the Freeze component (which we will probably deprecate as part of the App Navigation Reboot).

The ScreenWrapper being inside the react-freeze component prevents it from re-rendering and leaves it in a stuck state. This diff should fix it:

diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js
index 972dc0210f2..744a6d4fc2f 100644
--- a/src/pages/home/ReportScreen.js
+++ b/src/pages/home/ReportScreen.js
@@ -217,19 +217,17 @@ class ReportScreen extends React.Component {
         // There are no reportActions at all to display and we are still in the process of loading the next set of actions.
         const isLoadingInitialReportActions = _.isEmpty(this.props.reportActions) && this.props.report.isLoadingReportActions;
         return (
-            <Freeze
-                freeze={this.props.isSmallScreenWidth && this.props.isDrawerOpen}
-                placeholder={(
-                    <ScreenWrapper
-                        style={screenWrapperStyle}
-                    >
-                        <ReportHeaderSkeletonView />
-                        <ReportActionsSkeletonView containerHeight={this.state.skeletonViewContainerHeight} />
-                    </ScreenWrapper>
-                )}
+            <ScreenWrapper
+                style={screenWrapperStyle}
             >
-                <ScreenWrapper
-                    style={screenWrapperStyle}
+                <Freeze
+                    freeze={this.props.isSmallScreenWidth && this.props.isDrawerOpen}
+                    placeholder={(
+                        <>
+                            <ReportHeaderSkeletonView />
+                            <ReportActionsSkeletonView containerHeight={this.state.skeletonViewContainerHeight} />
+                        </>
+                    )}
                 >
                     <FullPageNotFoundView
                         shouldShow={!this.props.report.reportID}
@@ -310,8 +308,8 @@ class ReportScreen extends React.Component {
                             )}
                         </View>
                     </FullPageNotFoundView>
-                </ScreenWrapper>
-            </Freeze>
+                </Freeze>
+            </ScreenWrapper>
         );
     }
 }

@melvin-bot
Copy link

melvin-bot bot commented Nov 23, 2022

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@JmillsExpensify
Copy link

Awesome work guys! This is exactly the kind of digging into the codebase we need during WAQ.

@madmax330 madmax330 added the Reviewing Has a PR in review label Nov 28, 2022
@JmillsExpensify
Copy link

@rushatgabhane Mind applying to the Upwork job for C+ testing? Then we can close this one out as soon as the linked PR is deployed.

@JmillsExpensify
Copy link

Sent offer in Upwork. Once we close the loop there, then we should be good to close this issue out. Linked PR is on production.

Separately, I need to circle back on the regression test. It should be easy to include this in an existing test and at that point, we'll be done here.

@rushatgabhane
Copy link
Member

rushatgabhane commented Dec 5, 2022

@JmillsExpensify please offer $125 less, because I was paid extra by mistake last time.

@JmillsExpensify
Copy link

JmillsExpensify commented Dec 8, 2022

@rushatgabhane No worries, let's keep focused on squashing bugs. Your offer is appreciated though it's more work to rescind the offer than it is to fight the fight against bugs. Thank you!

@JmillsExpensify
Copy link

Rushat is paid out and the PR is already on production. I'm going to create a large keyboard behavior test following the completion of our KAV initiative, which I'm cataloguing in the linked regression test above. I'm closing this one out.

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 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests