Fixed: Chat header goes out of the view after opening keyboard

[s77rt]

@mananjadhav @pecanoro

Details

• Added VisualViewport onscroll listener
• ReportScreen: update updateViewportOffsetTop on VisualViewport onscroll listener callback

Fixed Issues

$ #13491
PROPOSAL: #13491 (comment)

Tests

1. Login to any account
2. Open any chat
3. Tap on the composer to open the keyboard
4. Verify that the header is still visible at the top

• Verify that no errors appear in the JS console

Offline tests

N.A

QA Steps

1. Login to any account
2. Open any chat
3. Tap on the composer to open the keyboard
4. Verify that the header is still visible at the top

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mp4

Mobile Web - Chrome

mweb-chrome.mp4

Mobile Web - Safari

mweb-safari.mp4

Desktop

desktop.mp4

iOS

ios.mp4

Android

android.mp4

[melvin-bot]

@pecanoro @mananjadhav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[mananjadhav]

@s77rt The Android error means that you already have another upgraded version installed. Can you uninstall the existing one in your device/emulator and attempt to run it again ?

[mananjadhav]

Changes look fine, I am going to test these today.

[s77rt]

@mananjadhav Indeed uninstalling the App did solve the Android installation issue. I was not expecting this kind of issue since I'm expecting to upgrade the App not downgrade it. Anyway, I have updated the test video for Android.

[mananjadhav]

@s77rt, Thanks for the quick PR here.

@pecanoro
I did the testing for the platforms except mobile web (most critical), because none of my existing whitelisted IPs work. I am blocked with the auth and rate limit issue!

[pecanoro]

@mananjadhav Can you complete the reviewer list and add videos for the other platforms? I will add whatever is missing due to the problems with the IPs.

[mananjadhav]

Okay sure rest I did check everything, so I'll update the checklist and screencasts in a few mins.

[pecanoro]

I get a weird glitch with scrolling:

Monosnap.screencast.2022-12-28.20-51-50.mp4

[s77rt]

@pecanoro I think that's expected to happen since you are scrolling the view. Can you try scroll a chat with messages and scroll the messages and not the view

[mallenexpensify]

I get a weird glitch with scrolling:

@mananjadhav are you able to reproduce the weird glitch?

[pecanoro]

I think that's expected to happen since you are scrolling the view. Can you try scroll a chat with messages and scroll the messages and not the view

@s77rt I don't see the same behavior in any of the platforms, so I would say it is not expected. Can you see the thing happening on the native apps? I couldn't but maybe I missed something.

[s77rt]

I meant it's expected based on the implemented code.

Other platforms don't have same behavior because only android Chrome shifts the view on scroll. On Safari the view will be shifted on resize event so scrolling have no effect.

Did you test this with a chat with messages? If you scroll the messages you should not experience that weird behavior.

I don't think we can prevent the behavior but maybe we should wrap the call in a debounce function so the header is only set on top after you stop scrolling (maybe after 1 second delay).

[s77rt]

@pecanoro In ReportScreen.js
Replace

        this.updateViewportOffsetTop = this.updateViewportOffsetTop.bind(this);

With

        this.debouncedUpdateViewportOffsetTop = _.debounce(this.updateViewportOffsetTop.bind(this), 1000);

And

        this.removeViewportResizeListener = VisualViewport.addViewportResizeListener(this.updateViewportOffsetTop);
        this.removeViewportScrollListener = VisualViewport.addViewportScrollListener(this.updateViewportOffsetTop);

With

        this.removeViewportResizeListener = VisualViewport.addViewportResizeListener(this.debouncedUpdateViewportOffsetTop);
        this.removeViewportScrollListener = VisualViewport.addViewportScrollListener(this.debouncedUpdateViewportOffsetTop);

---

• I didn't test this since all the chats I have do have messages and I can't make a new chat due to the ongoing issue.
• I set the debounce timer to 1000ms which may be too much, you can try lower the value as long as you don't notice the weird behaviour much

[mananjadhav]

So I was finally able to test this on mobile web. @s77rt I am able to reproduce the glitch that @pecanoro shared the video of.

While applying _.debounce works, I think our solution is not solid enough. Here's my testing on iOS device with Chrome.

chat-header-ios-chrome-scroll-issues.MP4

Please check the behavior of the Header when the keyboard hides/shows or we scroll.

[s77rt]

@mananjadhav I don't think we have much options here. For now I think debounce with minimal timer should be enough (1000ms is too much, try with 100ms)

Maybe bring this to slack?

[pecanoro]

Here is a video of a chat with more messages, and the same weird glitch. Though I am getting a weird behavior with the keyboard and I am not sure if it's coming from main or it's just the emulator.

Monosnap.screencast.2023-01-04.12-01-08.mp4

[s77rt]

@pecanoro You are still scrolling the view.
The messages are barely scroll-able try add more.

Are you testing after applying the debounce function?

PS: The space between the keyboard and the composer is another issue, I'm sure I seen it in GH not sure what we decided on it though.

[mananjadhav]

You are still scrolling the view

@s77rt Are you suggesting this exists in the main/staging/production too? If yes then we can ignore, but if not then whether we scroll View/Header/Message, etc. we don't want unwanted surprises with our change here.

[s77rt]

@mananjadhav What I meant is scrolling the view will trigger the header to be set on top (apply marginTop).

If we don't trigger the callback on scrolling the view then the header will stay out of view (recreating the issue)

[thesahindia]

You are still scrolling the view.

It used to work fine earlier. This should work correctly, doesn't matter if the user is scrolling the view. I have tested this on a real device and it is definitely not expected.

[s77rt]

@thesahindia Are you saying that the chat header was always staying at the top (on Android Chrome)?
If so can you please provide us at which point this was happening so we can track what went wrong.

If that's not what you meant then here what I have in mind since I can't reproduce the scroll issue reliability, create a slack discussion to see how we should move forward:

1. Do nothing (ignore the issue)
2. Fix the issue with my proposal as is
3. Fix the issue with my updated proposal (use a debounce function)
4. Reopen the issue for more proposals
5. ... (add any other solutions you see fit)

[mananjadhav]

@s77rt I agree with what @thesahindia mentioned. I think @pecanoro and I've posted videos along with the behaviours. Can you please help us with the updates on fixes for the same?

[s77rt]

@mananjadhav Isn't the debounce fix enough? I don't have any other solution in mind especially that I can't really reproduce the behaviour on my side

[mananjadhav]

@s77rt the video that I posted here is after using the denounce fix

[s77rt]

@pecanoro Yes, but if it's not much trouble can you actually try apply that PR (it's only one line change) so we can be sure it will fix the glitch. Otherwise putting on hold is okay

[pecanoro]

@s77rt Would you mind merging this PR with the main branch so anyone can test this and see it's working? It's been a month so let's make sure the code is up to date.

[s77rt]

@pecanoro Updated the branch. Looks there is no conflicts here.

[pecanoro]

@mananjadhav When you have some time, can you test the PR and fill out the reviewer list again? Also last time we were having some problems with the IPs and you should be able to add screenshots for all platforms.

@s77rt Do you mind updating the description with updated videos to show it's working without glitches now, please?

[mananjadhav]

@pecanoro I did a quick round of testing on iOS Chrome and it worked fine as commented here. I'll finish the checklist in an hour or two.

[s77rt]

@pecanoro I never experienced the glitch from my side and the emulator is not effected by this issue at all.. Can we just relay on @mananjadhav 's checklist videos for this one?

[pecanoro]

@s77rt I was looking at the videos you posted originally, and you can briefly see how the header jumps when you open the keyboard so the video needs to be updated so we can see that's not happening anymore.

[s77rt]

@pecanoro Ah yes, that's something that has been changed. Alright will update the videos soon.

[s77rt]

Updated the videos (keyboard related)
I'm still experiencing the jump behaviour on mWeb - Chrome. Can you verify if you can reproduce the glitch?

Tests must be done on physical devices. On Simulators/Emulators works fine.

cc @mananjadhav @pecanoro

[mananjadhav]

I've tested and this works fine for me. I didn't see the glitch on my Android device (but my device is a bit slower).

@pecanoro I've uploaded the screencasts and completed the checklist. All yours.

[mananjadhav]

Reviewer Checklist

• I have verified the author checklist is complete (all boxes are checked off).
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the steps for local testing are in the Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I checked that screenshots or videos are included for tests on all platforms
• I included screenshots or videos for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web-chat-header-view.mov

Mobile Web - Chrome

mweb-android-chrome-chat-header-view.mp4

Mobile Web - Safari

mweb-ios-chrome-chat-header-view.mp4

mweb-safari-chat-header-view.mp4

Desktop

desktop-chat-header-view.mov

iOS

ios-chat-header-view.mov

Android

android-chat-header-view.mov

[pecanoro]

I deleted the comment of the previous checklist because I think GH actions are getting confused 🤔

[pecanoro]

It seems to work fine for me without glitches except for Safari on iOS. However it's not the same as before, the header disapears a moment to be placed properly again. Funny thing is that only happens the first time I open the keyboard, after the first one, everything works as expected. I am not sure if it's a glitch only happening on emulator or a real device as well.

[s77rt]

@mananjadhav @pecanoro What's the status on this? And what about the timeline, did it reset after the other mentioned PR got merged?

[pecanoro]

Let me retest again, I think I still find that super small glitch on Safari, but not sure if it's just Safari behavior. Either way, we have conflicts, if you can fix them, it would be great.

[pecanoro]

This is interesting, but I think this new code does not change the behavior at all. Behavior in main is the same as this PR, am I wrong? I might be missing something here.

[s77rt]

@pecanoro Resolved the conflicts.

Last time I checked it seemed to work fine, and looking at @mananjadhav uploaded videos it seems fixed too. Can you double check from your side?

[pecanoro]

I can see the small jump on the keyboard even on @mananjadhav's video on Safari, right when you open it or close it, then it works just fine. Though again, I think this PR didn't change the behavior of main.

[s77rt]

@pecanoro Just noticed. I think that's out of the scope of the issue. This was the case for the very first time the page was implemented. I don't think we can get rid of the jump behaviour, if that was possible we wouldn't even had to add the updateViewportOffsetTop function.

[pecanoro]

@s77rt Either way, even if we omit that detail, these changes don't change any behavior from main, it is actually the same.

[s77rt]

@pecanoro This PR targets Android. I think the behaviour is different on Android. Can you please test on that instead?

[pecanoro]

Yes, nothing changes for Android, it works just fine on main.

[s77rt]

@pecanoro I understand that the chat header remains on view after opening the keyboard, is that right? If so then we can close both this PR and the issue. But let's get a confirmation from @mananjadhav for the same. Also per the latest comment here we may end up holding the issue again before making any further decision.