Revert "Revert "replace onModalHide with onDismiss for the web""

[bondydaa]

Reverts #15495 which reverted #15298 since that was not the root cause of the performance regression #15495 (comment)

cc @cristipaval @ntdiary @mollfpr.

I'm just copying the original PR description below verbatim

Details

Fixed Issues

$ #14848
PROPOSAL: #14848 (comment)

Tests

1. Open a report that has historical messages.
2. Press the edit message icon.
3. Press the emoji icon in the edit composer to open the emoji modal.
4. Exit the emoji model by pressing outside.
5. The edit composer will highlight.

• Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

1. Open a report that has historical messages.
2. Press the edit message icon.
3. Press the emoji icon in the edit composer to open the emoji modal.
4. Exit the emoji model by pressing outside.
5. The edit composer will highlight.

• 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
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • 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.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• 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

android-chrome.mp4

Mobile Web - Safari

ios-safari.mp4

Desktop

desktop.mp4

iOS

ios-app.mp4

Android

android-app.mp4

[MelvinBot]

@cristipaval @mollfpr 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]

[Luke9389]

I approved the initial revert, so I'll just do this one too.

[Luke9389]

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.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

[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

Name	Duration
Open Search Page TTI	600.109 ms → 668.907 ms (+68.798 ms, +11.5%) 🔴

Show details

Name

Duration

Open Search Page TTI

Baseline Mean: 600.109 ms Stdev: 18.486 ms (3.1%) Runs: 564.3546959999949 567.7201739996672 575.0892340000719 578.2112630009651 579.046833999455 581.2911789994687 584.6187750007957 586.3546959999949 587.5615240000188 588.8333330005407 588.9936120007187 594.0775559991598 595.2496750000864 597.6497390009463 599.0179039984941 599.0775149986148 600.906534999609 600.9844979997724 602.5358889997005 603.7467450015247 604.2611090000719 606.9387610014528 607.2118730004877 607.9663500003517 610.6350920014083 618.4355880003422 621.8337399996817 621.9777020011097 627.9497480001301 628.0310880001634 634.6167810000479 638.3157560005784 Current Mean: 668.907 ms Stdev: 18.053 ms (2.7%) Runs: 638.6670329999179 640.5199790000916 641.1783450003713 643.8239339999855 645.8155519999564 652.4922690000385 654.145060999319 656.7302650008351 657.8067220002413 659.9246020000428 660.4343269988894 662.4549560006708 663.0885819997638 663.9174810014665 665.7833259999752 666.0568859986961 666.235027000308 667.3260910008103 669.4038500003517 674.4121500011533 676.6907139997929 677.0060629993677 679.3702810015529 679.5069989990443 683.7829190008342 686.6841640006751 688.0747480001301 688.1204429995269 691.0817059986293 692.9520260002464 693.5267750006169 718.0032549984753

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	684.264 ms → 710.204 ms (+25.940 ms, +3.8%)
App start runJsBundle	178.516 ms → 185.034 ms (+6.518 ms, +3.7%)
App start regularAppStart	0.013 ms → 0.014 ms (+0.000 ms, +3.6%)
App start nativeLaunch	21.097 ms → 21.067 ms (-0.030 ms, ±0.0%)

Show details

Name	Duration
App start TTI	Baseline Mean: 684.264 ms Stdev: 25.350 ms (3.7%) Runs: 628.5997819993645 644.6363229993731 650.832256000489 654.7931900005788 657.4283729996532 658.5502310004085 661.6295510008931 663.0639260001481 665.2113920003176 667.0957430005074 674.1705959998071 676.4753959998488 677.5735449995846 678.6334760002792 679.9323490001261 680.6071279998869 682.8152510002255 684.6215829998255 689.4452929999679 690.6663930006325 690.8901809994131 694.8910579998046 698.7621309999377 701.4378229994327 704.0315649993718 705.1634260006249 714.4187150001526 715.5534010007977 716.480732999742 726.0943119991571 726.5821080002934 735.3507170006633 Current Mean: 710.204 ms Stdev: 23.984 ms (3.4%) Runs: 664.8493850007653 671.9116200003773 679.55410999991 680.786271000281 681.0577450003475 686.1796170007437 687.7824939992279 693.9881260003895 694.1810079999268 697.1188329998404 698.1455770004541 701.4002539999783 702.4228639993817 704.7472980003804 704.8114720005542 712.4829600006342 715.1089319996536 715.8119109999388 716.9735829997808 717.4308470003307 717.5469390004873 721.3401939999312 722.0987810008228 722.9601620007306 727.2667789999396 732.0375309996307 733.1915649995208 747.9169459994882 748.7795570008457 751.7301460001618 764.7024140004069
App start runJsBundle	Baseline Mean: 178.516 ms Stdev: 13.614 ms (7.6%) Runs: 155 156 161 161 165 166 168 170 171 172 172 173 175 175 176 176 178 179 180 180 182 182 185 186 189 190 192 201 203 206 209 Current Mean: 185.034 ms Stdev: 10.301 ms (5.6%) Runs: 167 168 171 172 173 177 177 178 180 180 180 182 182 184 184 185 185 186 188 190 191 193 196 196 196 197 198 199 211
App start regularAppStart	Baseline Mean: 0.013 ms Stdev: 0.001 ms (5.5%) Runs: 0.01212499849498272 0.012246999889612198 0.012248000130057335 0.012409999966621399 0.012491999194025993 0.012654999271035194 0.012654999271035194 0.012816999107599258 0.01285799965262413 0.01285799965262413 0.012899000197649002 0.012938998639583588 0.01297999918460846 0.012980999425053596 0.013062000274658203 0.013346999883651733 0.013347001746296883 0.01342800073325634 0.013468001037836075 0.013508999720215797 0.01355000026524067 0.013630999252200127 0.013753000646829605 0.013793999329209328 0.013875000178813934 0.014079000800848007 0.014159999787807465 0.014160001650452614 0.014322999864816666 0.014364000409841537 0.014485999941825867 0.01521800085902214 Current Mean: 0.014 ms Stdev: 0.001 ms (6.1%) Runs: 0.012531999498605728 0.012654000893235207 0.012817000970244408 0.013020999729633331 0.013061000034213066 0.013183999806642532 0.013183999806642532 0.013224000111222267 0.013224000111222267 0.01326499879360199 0.013265000656247139 0.013265000656247139 0.013305999338626862 0.013468001037836075 0.013508999720215797 0.013509999960660934 0.013591000810265541 0.013670999556779861 0.013712998479604721 0.013753000646829605 0.013956999406218529 0.013956999406218529 0.014119001105427742 0.0143630001693964 0.014526000246405602 0.01476999931037426 0.014851000159978867 0.014893000945448875 0.015013998374342918 0.015257999300956726 0.015544001013040543 0.0157880000770092
App start nativeLaunch	Baseline Mean: 21.097 ms Stdev: 1.729 ms (8.2%) Runs: 18 18 19 19 19 19 19 20 20 20 20 21 21 21 21 21 21 21 21 22 22 22 22 23 23 23 23 23 24 24 24 Current Mean: 21.067 ms Stdev: 2.380 ms (11.3%) Runs: 18 18 18 19 19 19 19 19 19 20 20 20 20 21 21 21 21 21 21 21 21 22 23 23 23 23 23 24 27 28

[github-actions]

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

[cristipaval]

Sorry I'm messing around here with Github. I clicked Request changes by mistake and now I can't cancel that.

[cristipaval]

Oh wait, this was actually already merged 🤦‍♂️
Sorry guys

[Luke9389]

@cristipaval no worries! 😄

[OSBotify]

🚀 Deployed to staging by https://github.com/Luke9389 in version: 1.2.77-0 🚀

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

[OSBotify]

🚀 Deployed to staging by https://github.com/Luke9389 in version: 1.2.77-0 🚀

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

[mvtglobally]

@bondydaa since you reverted #15298 & #15495. Which one we should QA :)

[OSBotify]

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.77-4 🚀

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