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

Scrollbar appears in the middle of conversation '"View details" option #3637

Closed
isagoico opened this issue Jun 17, 2021 · 20 comments · Fixed by #3709
Closed

Scrollbar appears in the middle of conversation '"View details" option #3637

isagoico opened this issue Jun 17, 2021 · 20 comments · Fixed by #3709
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement.

Comments

@isagoico
Copy link

isagoico commented Jun 17, 2021

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. Install 1.0.70 build
  2. Login with gmail account B
  3. Open Chat with account C
  4. Click the tab "View Details"
  5. Scroll down the "Details" tab

Expected Result:

The scrollbar should appear on the right side of the conversation

Actual Result:

Scrollbar appears in the middle of conversation inside in '"View details" option

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

Web
iOS ✔️
Android
Desktop App
Mobile Web

Version Number: 1.0.70-0

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

Notes/Photos/Videos: Any additional supporting documentation

Bug5116985_Scrol_down.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

@MelvinBot
Copy link

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

@MonilBhavsar
Copy link
Contributor

Can we know on which environment, device and iOS version this issue is occurring.

@Julesssss
Copy link
Contributor

Weirdly I'm unable to reproduce this issue on 1.0.70-0 a physical iPhone 8 running iOS 11. Perhaps this is specific to a certain iOS version?

RPReplay_Final1624013578.MP4

@isagoico
Copy link
Author

Issue was reproduced in Apple iPhone XS Max - iOS 14.0.1

@MonilBhavsar MonilBhavsar added Weekly KSv2 and removed Daily KSv2 labels Jun 18, 2021
@MonilBhavsar
Copy link
Contributor

Changing priority to weekly since occurring on specific combination of iOS version and device

@MonilBhavsar
Copy link
Contributor

Since it's not a universal bug, though of looking at React Native issues and found an issue similar to this which is still open.
Someone also commented -

Same issue here. It's not possible to reproduce with 100% success, but so far we noticed this on iPhone Xs running iOS 13.0 and 13.1 on expo@35.0.0 & react-native@0.59.8

@MonilBhavsar MonilBhavsar added the Improvement Item broken or needs improvement. label Jun 18, 2021
@MonilBhavsar
Copy link
Contributor

Good to go in the pool

@MonilBhavsar MonilBhavsar removed their assignment Jun 18, 2021
@Julesssss
Copy link
Contributor

I see two potential fixes in that issue: 1 & 2.

Maybe we should open this issue up to contributors

@Julesssss Julesssss added the External Added to denote the issue can be worked on by a contributor label Jun 18, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Jun 18, 2021
@rdjuric
Copy link
Contributor

rdjuric commented Jun 18, 2021

Proposal

I've confirmed that setting scrollIndicatorInsets={{ right: 1 }} works correctly and has no meaningful difference on Android.

Also, this happens in our other modals too. So adding this to our BaseModal is a good ideia.

@Julesssss
Copy link
Contributor

I think that either would be fine, but perhaps the SafeAreaView fix would be preferred as we already use SafeAreaView elsewhere in the app?

@bfitzexpensify
Copy link
Contributor

Job post on Upwork.

@MelvinBot
Copy link

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

@bfitzexpensify
Copy link
Contributor

Ah, many thanks GH for the appropriate assignment

@Julesssss
Copy link
Contributor

@rdjuric please go ahead with the proposal.

I think Safe area would be preferable. If that doesn't work the scrollIndicator fix is also fine, but could you include a comment linking to the RN issue. Thanks!

@rdjuric
Copy link
Contributor

rdjuric commented Jun 18, 2021

I think Safe area would be preferable. If that doesn't work the scrollIndicator fix is also fine, but could you include a comment linking to the RN issue. Thanks!

Ok, I'll make a few more tests and if it works equally well I'll use SafeAreaView in the PR. Including the issue in a comment is a good idea!

@bfitzexpensify I just sent the upwork proposal too

@isagoico
Copy link
Author

isagoico commented Jun 21, 2021

Issue not reproducible during KI retests (First week)

@parasharrajat
Copy link
Member

parasharrajat commented Jun 22, 2021

Did anybody notice we already have SafarArea applied inside BaseModal? https://github.com/Expensify/Expensify.cash/blob/d1b79b0cc6de88722199fffc2340b60fbf4b7441/src/components/Modal/BaseModal.js#L128

cc: @Julesssss

@rdjuric
Copy link
Contributor

rdjuric commented Jun 22, 2021

@parasharrajat Yea! I spent way too much time trying to figure out what was happening, but since we don't know the root cause of this RN bug is hard to understand why our SafeAreaInsetsContext isn't enough.

Maybe something to do with SafeAreaInsetsContext not updating the insets synchronously?
Things get more confusing because our Modal content goes through the custom gesture handler from react-native-modal which already caused us scrolling problems before (#3487).

At the end of the day I just added the recommended workaround that made the issue irreproducible for me. scrollIndicatorInsets={{ right: 1 }} wasn't working consistently on attachment modals.

Here's a little table I made while I was testing this stuff

iOS Before After
12 irreproducible irreproducible
13.7 REPRODUCIBLE irreproducible
14.0 REPRODUCIBLE irreproducible
14.4 REPRODUCIBLE irreproducible
14.5 irreproducible irreproducible

Screen size seemed to influence it, but I didn't made logs of that. The issue was not consistently reproducible either.

@parasharrajat
Copy link
Member

Yeah this is hard to reproduce but then the same question comes to me that why other modals are working fine if there is an issue with SafeAreaInsetsContext. And to solve this issue we wrapped SafeAreaInsetsContext with another SafeAreaView. Don't know the reason yet but that's a question I am stuck with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants