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

Chat - Conversation doesn't expand to full view after completing editing a message #46743

Closed
1 of 6 tasks
lanitochka17 opened this issue Aug 2, 2024 · 9 comments · Fixed by kirillzyusko/react-native-keyboard-controller#539
Assignees
Labels
Engineering Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 2, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.16-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4807037
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

  1. Launch new Expensify
  2. Navigate to any chat with messages
  3. Tap and hold on a message you left > Tap Edit
  4. Make any changes and tap on the checkmark

Expected Result:

The edit is saved and keyboard is dismissed with the conversation expanding to the whole screen

Actual Result:

The edit is saved and keyboard is dismissed but an empty space is left

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6560325_1722615899708.RPReplay_Final1722615350.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp

@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API and removed Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

Triggered auto assignment to @thienlnam (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

github-actions bot commented Aug 2, 2024

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

@lanitochka17
Copy link
Author

Production:

RPReplay_Final1722622588.MP4

@Beamanator
Copy link
Contributor

@ishpaul777 maybe this one too? (possibly fixed by #46749)

@Beamanator Beamanator removed the DeployBlocker Indicates it should block deploying the API label Aug 2, 2024
@ishpaul777
Copy link
Contributor

Yes this is also fixed

Screen.Recording.2024-08-03.at.1.03.11.AM.mov

This comment was marked as off-topic.

@Beamanator Beamanator added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Aug 2, 2024
@Beamanator
Copy link
Contributor

Confirmed fixed in staging!

kirillzyusko added a commit to kirillzyusko/react-native-keyboard-controller that referenced this issue Aug 8, 2024
…om space (#539)

## 📜 Description

Fixed a case when `KeyboardAvoidingView` is not getting resized back
when keyboard is hidden instantly.

## 💡 Motivation and Context

The condition:

```tsx
if (e.duration === 0) {
  progress.value = e.progress;
  height.value = e.height;
}
```

prevented view to restore its initial position when keyboard is hidden
instantly. When such thing happens we have next events:

```bash
 LOG  onStart {"duration": 250, "eventName": "onKeyboardMoveStart", "height": 0, "progress": 0, "target": 7054} 1723043957135
 LOG  onEnd {"duration": 250, "eventName": "onKeyboardMoveEnd", "height": 0, "progress": 0, "target": -1} 1723043957142
```

So technically we had to set our `height`/`progress` to `0`. But we skip
it, because `event` actually has non-zero duration (it's 250ms).

We can't simply remove the code that I added before because it'll bring
old bug back, so I was thinking a lot on how to resolve the problem.

### 1️⃣ Add more conditions

The first idea was to modify condition to be like

```tsx
if (e.duration === 0 || e.target === -1) {}
```

However I don't like it because:
- it adds more complexity to the code;
- we have to remember about that case and use this condition in every
hook;
- this code doesn't feel cross-platform;
- I'm not sure if it's a correct fix, because we may have a new
situation, where this condition will lead to unexpected results;

So I started to think about other approaches.

### 2️⃣ Native code modification

At some of point of time I realized, that we expect `onEnd` to report an
actual keyboard height that is currently visible on the screen (at least
in `keyboardDidAppear` method). And if we rely on this fact then we can
remove the condition from `onEnd` handler.

So I decided to try to re-work the code and instead of re-forwarding the
data from a notification I decided to grab a real data from the
`keyboardVIew` (its real coordinates) and send them.

Turned out that this fixes old bug and doesn't introduce new one, so I
think I'll stick with this approach. Also all e2e tests passes without
any modification and I think it's a good sign that I do everything
properly 😅

> [!NOTE]
> It's also safe to read frame inside this particular handler, because
we know, that keyboard stopped its movement and has a final frame, so we
don't need to predict next frame based on a current one etc. - we can
simply read frame data and be sure it's exact number what user sees on
the screen.

A new fix for
#327

Also fixes: Expensify/App#46743 and
Expensify/App#46745

## 📢 Changelog

<!-- High level overview of important changes -->
<!-- For example: fixed status bar manipulation; added new types
declarations; -->
<!-- If your changes don't affect one of platform/language below - then
remove this platform/language -->

### JS

- removed `if (e.duration === 0)` condition in `KeyboardAvoidingView`
hook;

### iOS

- added new `framePositionInWindow` extension to `UIView`;
- take an actual keyboard height (based on `keyboardView`) in
`keyboardDidAppear` method.

## 🤔 How Has This Been Tested?

Tested manually on iPhone 15 Pro (iOS 17.4).

## 📸 Screenshots (if appropriate):

|Before|After|
|-------|-----|
|<video
src="https://github.com/user-attachments/assets/19b3ac39-bd8e-4d47-bc34-a6ba7f3478b3">|<video
src="https://github.com/user-attachments/assets/f7492e25-ccaa-42f0-870a-60eafe912a7d">|

I also checked that
#327
is not reproducible anymore.

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Reviewing Has a PR in review Weekly KSv2
Projects
None yet
5 participants