fix: hide overflow of html's body to fix scroll bug on safari ios

[shonsirsha]

cc @Santhosh-Sellavel

Details

On safari (iOS), sometimes scrolling locks. This means when a user scrolls, the whole page gets scrolled, but not the actual list that the user intends to scroll - causing the scrolling to not be useful at all hence it's called "locked". Check issue below to see the bug visually.

Fixed Issues

$ #13599
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

1. Open LHN but don't scroll anywhere
2. Scroll up on the LHN list, verify that the list should be scrolling up, and not the entire page (scroll body locking)
3. Scroll down on the LHN list, verify that the list should be scrolling down, and not the entire page (scroll body locking)
4. Related to 3, if list is longer than device's screen, verify that we should still be able to see the last item on the list.

• Verify that no errors appear in the JS console

Offline tests

Same as tests.

QA Steps

1. Open LHN but don't scroll anywhere
2. Scroll up on the LHN list, verify that the list should be scrolling up, and not the entire page (scroll body locking)
3. Scroll down on the LHN list, verify that the list should be scrolling down, and not the entire page (scroll body locking)
4. Related to 3, if list is longer than device's screen, verify that we should still be able to see the last item on the list.

• 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

DESKTOP.mov

Mobile Web - Chrome

ANDROID_CHROME.mov

Mobile Web - Safari

WEB_IOS.mp4

Desktop

MAC_DESKTOP.mov

iOS

IOS_APP.mov

Android

ANDROID_APP.mov

[melvin-bot]

@danieldoglas Please 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]

[danieldoglas]

@pecanoro , you should be assigned to this PR review since it was created from an issue you're the CME, right?

[Santhosh-Sellavel]

@danieldoglas Yes assign me & @pecanoro as reviewers.

[shonsirsha]

@pecanoro , you should be assigned to this PR review since it was created from an issue you're the CME, right?

Yeah, unfortunately I can't change the reviewers (lacking permissions). I assume you could re-assign it to @pecanoro and @Santhosh-Sellavel though. 🙂

[Santhosh-Sellavel]

@shonsirsha

The issue of not being linked properly is the problem here.

Should be linked as follows
$ https://github.com/Expensify/App/issues/13599

which would show as $ #13599

Please update the link, and keep a note of it. This is important as it's used for automating certain things, i.e assigning the correct reviewers, tracking PR deployment status on issues, etc

[Santhosh-Sellavel]

Adding my self as reviewer

[shonsirsha]

@Santhosh-Sellavel ok, done updated. Added a "#" infront of the issue's number. Haven't had this problem before (closed 2 PRs), so thanks for letting me know. Hope it could be reviewed now?

[Santhosh-Sellavel]

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

Screen.Recording.2023-01-09.at.11.13.37.PM.mov

Mobile Web - Chrome

Screen_Recording_20230109_230000_One.UI.Home.mp4

Mobile Web - Safari

RPReplay_Final1673284971.MP4

Desktop

Screen.Recording.2023-01-09.at.11.14.52.PM.mov

iOS

Android

[shonsirsha]

Hello there 👋

Can we get the review going or are we waiting on smt else? 🙂 cc @Santhosh-Sellavel

[Santhosh-Sellavel]

@shonsirsha Thanks for the bump I failed to post this one yesterday, actually something is off in android, can you check?

I tried to scroll down to the end, few more empty scroll down even after reaching the end of the list. Now trying to scroll back it's not working right away

Screen_Recording_20230108_024006_New.Expensify.mp4

[shonsirsha]

@Santhosh-Sellavel No worries.

Android you meant android app right? If so, the issue you mentioned also exists in main:

k.mov

Actually this issue also exists in production:

IMG_5826.mp4

Tagging @pecanoro, would you be able to test it (if this issue exists in prod/main) to make sure that it's a separate issue (not related to this PR)?

Thx!

[Santhosh-Sellavel]

@shonsirsha Yeah it exists in the main, this feels like the same issue but on a different platform. So fix is expected here as well, but I let @pecanoro make the final call on it.

[shonsirsha]

Thanks @Santhosh-Sellavel, will wait on their final call then. Just my 2 cents, the problem that we have here on Android isn't exactly the same. Moreover, the issue was focusing on iOS scroll problem, and Android (app) wasn't mentioned at all in the issue, It's an mWeb issue on iOS. (iOS mWeb - Scroll behavior on some pages is inconsistent and broken).

While it may look like that it's the same "scroll bug" on the surface, they aren't the same - I could try describing it this way:

1. On iOS the issue was body locking, which Ive explained in the issue & in the PR body (in short: sometimes, ios scrolls the actual html body, and not the list). This is on mWeb iOS.
2. Here on android (app, not mWeb!), from my brief observation, the issue is user can't start scrolling up after spamming a scroll down to the list. It already scrolls the list, not the html body (this is where it's different than ios'). A bit more explanation: the issue on Android here, to me it looks like when user dragged / scrolled the list to the most bottom and still scrolling some more, something (maybe an animation) will last for lets say 1s, and user should wait for 1s before they can scroll up. If the user spams their scrolling (scrolling down while already at the most bottom), lets say 10 times, they'd then have to wait ~10 seconds before they can scroll up without any problems.

Frankly, these 2 sound like they're completely different issues to me.. 🤔

[Santhosh-Sellavel]

@pecanoro

The main issue discussed here is fixed, tests well. I run into another issue here it looks related and slightly different form the issue we are addressing here.

If you think both are different please merge this one as the main issue is solved and tests well, or do we need to wait to solve the other one too?, thanks!

[pecanoro]

I can attest the Android glitch is on prod as well, it's a bit of an edge glitch though since you have to consciously spam scrolling down. @Santhosh-Sellavel Can you post the bug on Slack? Maybe the team agrees to create an issue to fix it.

I will finish testing the PR and I will merge it if I can't find any other issues.

[Santhosh-Sellavel]

I'll report it on slack, thanks @pecanoro!

[pecanoro]

Unless I missed something, it seems to be working well!

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	712.959 ms → 723.538 ms (+10.579 ms, +1.5%)
App start nativeLaunch	9.867 ms → 20.194 ms (+10.327 ms, +104.7%) 🟡
App start runJsBundle	194.645 ms → 202.688 ms (+8.042 ms, +4.1%)
App start regularAppStart	0.014 ms → 0.022 ms (+0.008 ms, +55.5%) 🟡
Open Search Page TTI	592.662 ms → 591.194 ms (-1.469 ms, ±0.0%)

Show details

Name	Duration
App start TTI	Baseline Mean: 712.959 ms Stdev: 43.402 ms (6.1%) Runs: 622.6705419998616 643.7582339998335 654.8134249998257 655.5639570001513 670.067687000148 671.2705560000613 679.0015319995582 680.9791400004178 687.4435029998422 692.3763669999316 694.8657950004563 696.2237560003996 700.1406209999695 704.2559590004385 706.0855940002948 706.189210999757 710.0833900002763 710.2128299996257 716.4388060001656 724.2199410004541 728.0014620004222 733.6161409998313 737.3372969999909 740.2064610002562 743.3609710000455 744.2691519996151 745.6265590004623 748.989725000225 765.3112110001966 778.8852599998936 804.7800059998408 817.6554709998891 Current Mean: 723.538 ms Stdev: 38.274 ms (5.3%) Runs: 660.829946000129 672.9971510004252 678.3025219999254 683.3525860002264 684.4441309999675 689.8298150002956 691.0943160001189 693.0047810003161 693.1802909998223 697.1058620000258 698.1814130004495 700.7377319997177 702.5611669998616 704.7761059999466 705.1094610001892 711.0486840000376 715.8255150001496 723.8832890000194 725.9553370000795 726.2004270004109 729.4683240000159 743.3003979995847 748.1408550003543 753.3963810000569 758.1050349995494 759.6859379997477 762.5017579998821 763.5093430001289 780.3500189995393 789.192303000018 798.7816740004346 808.3670770004392
App start nativeLaunch	Baseline Mean: 9.867 ms Stdev: 1.310 ms (13.3%) Runs: 8 8 8 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 10 10 11 11 11 11 11 12 12 14 Current Mean: 20.194 ms Stdev: 1.786 ms (8.8%) Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 21 21 21 21 21 21 22 22 22 22 23 23 23 24
App start runJsBundle	Baseline Mean: 194.645 ms Stdev: 22.531 ms (11.6%) Runs: 159 159 167 171 174 176 177 179 181 182 182 183 184 186 187 189 191 192 194 196 204 207 211 211 214 216 217 225 229 235 256 Current Mean: 202.688 ms Stdev: 21.914 ms (10.8%) Runs: 170 172 172 174 183 183 184 185 185 186 188 189 189 196 197 198 201 206 207 207 212 212 213 215 217 219 226 228 234 238 246 254
App start regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (6.0%) Runs: 0.012248000130057335 0.012776999734342098 0.013020000420510769 0.013101999647915363 0.013468999415636063 0.013469000346958637 0.013672000728547573 0.013794000260531902 0.013834000565111637 0.0138349998742342 0.013875000178813934 0.014038000255823135 0.014120000414550304 0.01416000071913004 0.014240999706089497 0.014281999319791794 0.014322999864816666 0.014405000023543835 0.014567000791430473 0.01464799977838993 0.014770000241696835 0.014770999550819397 0.014852000400424004 0.014852000400424004 0.014852000400424004 0.015095999464392662 0.015096000395715237 0.01603199914097786 0.016154000535607338 Current Mean: 0.022 ms Stdev: 0.002 ms (8.7%) Runs: 0.017822000198066235 0.019286999478936195 0.01932699978351593 0.020101000554859638 0.020181999541819096 0.020711000077426434 0.02075199969112873 0.020874000154435635 0.020913999527692795 0.021402999758720398 0.021728000603616238 0.021769000217318535 0.02180900052189827 0.022013000212609768 0.022053999826312065 0.022095000371336937 0.022175999358296394 0.02254300005733967 0.022704999893903732 0.02274599950760603 0.022908000275492668 0.022948999889194965 0.02307100035250187 0.023072000592947006 0.023926000110805035 0.024291999638080597 0.025350000709295273 0.025837999768555164 0.026651999913156033
Open Search Page TTI	Baseline Mean: 592.662 ms Stdev: 27.573 ms (4.7%) Runs: 530.4602459995076 543.5930589996278 549.2548829996958 549.7313230000436 550.5642900001258 557.3299559997395 571.0021169995889 576.3798830006272 582.0365809993818 583.6855480000377 587.31827800069 588.7239180002362 589.909953000024 590.4574790000916 590.6377360001206 592.4097499996424 594.197061999701 596.2263190001249 599.780923999846 599.8270269995555 602.6924239993095 604.0796310007572 604.8326009996235 620.2275799997151 621.718180000782 625.0888269999996 625.1405029995367 625.306925999932 626.5959879998118 627.5560710001737 628.050904000178 630.3778079999611 Current Mean: 591.194 ms Stdev: 29.220 ms (4.9%) Runs: 538.6354170003906 544.7965090004727 546.3520510001108 550.530639000237 552.363078000024 555.7019450003281 563.4352620001882 564.0561530003324 574.3838700000197 574.4556070007384 574.8662919998169 579.1536459997296 583.2205409994349 585.493978000246 588.4751800000668 589.5305180000141 590.2057300005108 590.5054129995406 596.0698650004342 597.6475010002032 598.1923430003226 599.4286299999803 605.6273610005155 612.1601979993284 612.2331139994785 620.3297939999029 622.1164149995893 624.4651699997485 627.4106040000916 627.6962890001014 630.8687750007957 635.6681730002165 653.3096519997343

[OSBotify]

🚀 Deployed to staging by @pecanoro in version: 1.2.53-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[mvtglobally]

@shonsirsha @Santhosh-Sellavel @pecanoro this PR looks like we should QA it. Can you please add QA steps

[Santhosh-Sellavel]

🤦 Not sure How did I missed that, @shonsirsha can you add test steps please

[pecanoro]

Oh dammit, I remember noticing that when reviewing and forgot to post about it before merging.

[shonsirsha]

@mvtglobally @pecanoro @Santhosh-Sellavel sorry! I totally missed it as well. Added now!

[OSBotify]

🚀 Deployed to production by @Julesssss in version: 1.2.53-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅