Disable the focus trap for report page context menu #15375

allroundexperts · 2023-02-22T21:53:14Z

Details

This PR fixes the issue where focus was being restored back to the message once the context menu closed. It disables this focus trap by blurring the element immediately after right click.

Fixed Issues

$ #14329
PROPOSAL: #14329 (comment)

Tests

Go to any chat

Send any long message (10000+ characters)
Send few message after long message
Right click on long message
Press copy to clipboard or touch outside to close the context menu

Verify that no errors appear in the JS console

Offline tests

QA Steps

Go to any chat
Send any long message (10000+ characters)
Send few message after long message
Right click on long message
Press copy to clipboard or touch outside to close the context menu

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-02-23.at.2.48.13.AM.mov

Desktop

Screen.Recording.2023-02-23.at.2.44.59.AM.mov

iOS

Screen.Recording.2023-02-23.at.10.42.22.PM.mov

Android

Screen.Recording.2023-02-23.at.10.49.39.PM.mov

iOS Safari

Screen.Recording.2023-02-24.at.3.09.43.AM.mov

Android Chrome

Screen.Recording.2023-02-24.at.3.08.34.AM.mov

MelvinBot · 2023-02-22T22:02:12Z

@chiragsalian @eVoloshchak 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]

src/components/PressableWithSecondaryInteraction/index.js

allroundexperts · 2023-02-23T14:46:15Z

@eVoloshchak I've gone through this again and I think that removing PressableWithoutFocus is not a good idea here. PressableWithoutFocus does not have any secondary interaction defined in it. If you do a right click on a PressableWithoutFocus, you'll see that a native context menu opens up. However, with PressableWithSecondaryInteraction, if you do a right click on it, the native context menu does not open. Instead, the handler provided for longPress gets triggered. We can add a property to disable this for PressableWithSecondaryInteraction but I think that will kill the whole purpose of this component. Please let me know what you think about this and I can edit the PR after that.

eVoloshchak · 2023-02-23T14:55:34Z

@allroundexperts, good catch!
I can observe this behavior too. In that case you can edit the PR, I'll start going through the checklist shortly

allroundexperts · 2023-02-23T15:07:29Z

@allroundexperts, good catch! I can observe this behavior too. In that case you can edit the PR, I'll start going through the checklist shortly

Awesome. I've updated the PR.

eVoloshchak · 2023-02-23T15:16:16Z

@allroundexperts, looks like you need to merge with main, there's something weird with the commits

allroundexperts · 2023-02-23T15:24:11Z

@allroundexperts, looks like you need to merge with main, there's something weird with the commits

@eVoloshchak Fixed.

src/components/PressableWithSecondaryInteraction/index.js

…econdaryIntegration component

eVoloshchak · 2023-02-23T16:47:35Z

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

Screen.Recording.2023-02-23.at.18.57.33.mov

Mobile Web - Chrome

Screen_Recording_20230223-190101_Chrome.mp4

Mobile Web - Safari

Screen.Recording.2023-02-23.at.18.56.41.mov

Desktop

Screen.Recording.2023-02-23.at.18.58.45.mov

iOS

Screen.Recording.2023-02-23.at.18.51.51.mov

Android

Screen_Recording_20230223-190014_New.Expensify.mp4

eVoloshchak · 2023-02-23T16:49:57Z

@allroundexperts, you need to check off every checkbox in the checklist.
Also, there should be a video for each platform, even the ones that aren't affected by this change (just to verify everything works as expected)

allroundexperts · 2023-02-23T16:52:01Z

@allroundexperts, you need to check off every checkbox in the checklist. Also, there should be a video for each platform, even the ones that aren't affected by this change (just to verify everything works as expected)

@eVoloshchak What screen should I show on mobile? Since there is no right click there, what should I show there?

eVoloshchak · 2023-02-23T16:54:30Z

@eVoloshchak What screen should I show on mobile? Since there is no right click there, what should I show there?

You can long-press on a message to trigger the context menu. I've uploaded an iOS video here

allroundexperts · 2023-02-23T16:55:14Z

@eVoloshchak What screen should I show on mobile? Since there is no right click there, what should I show there?

You can long-press on a message to trigger the context menu. I've uploaded an iOS video here

Got it. Thank you. I'll upload these 👍

allroundexperts · 2023-02-23T22:18:24Z

@eVoloshchak I've added the missing recordings.

eVoloshchak

LGTM
cc: @chiragsalian

chiragsalian

Code LGTM too.
The GH linked is incorrect. You linked #14148
but i think you meant to link #14329. @Luke9389 is supposed to review but i totally don't mind reviewing too 🙂

Please correct it so that melvin can track the issue status properly 🙂

github-actions · 2023-02-24T00:52:59Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	705.252 ms → 725.625 ms (+20.373 ms, +2.9%)
App start runJsBundle	188.719 ms → 199.656 ms (+10.938 ms, +5.8%)
App start regularAppStart	0.013 ms → 0.014 ms (+0.001 ms, +7.2%)
App start nativeLaunch	20.129 ms → 20.097 ms (-0.032 ms, ±0.0%)
Open Search Page TTI	605.175 ms → 600.323 ms (-4.852 ms, -0.8%)

Show details

Name	Duration
App start TTI	Baseline Mean: 705.252 ms Stdev: 32.043 ms (4.5%) Runs: 656.7403159998357 657.1650129999034 657.6229329998605 658.5840039998293 666.2301900000311 667.3704229998402 676.4139439999126 676.4394769999199 677.107822000049 681.5935630002059 682.5857239998877 687.9977750000544 689.3819499998353 692.152557999827 702.7912240000442 712.7078940002248 717.3833550000563 717.6698389998637 717.8453399999999 717.9589450000785 721.354619000107 721.7916930001229 722.7528030001558 725.1078789997846 729.0729040000588 730.9200410000049 736.872181000188 737.2294990001246 744.251718999818 750.28448199993 759.7817609999329 774.8945019999519 Current Mean: 725.625 ms Stdev: 26.807 ms (3.7%) Runs: 670.5485060000792 684.9547600001097 686.7896170001477 695.833149000071 697.4832729999907 700.684297000058 702.4100569998845 710.6013949997723 710.6729759997688 710.685513000004 712.8711040001363 714.2182300002314 715.4176210002042 717.0192450000905 718.7411299999803 719.5670440001413 721.9738920000382 723.039197999984 730.8015310000628 731.8001819998026 734.316234999802 737.1466000000946 740.3472799998708 743.4288499997929 744.3552899998613 745.6797569999471 753.5112919998355 754.5922940000892 754.7620970001444 765.7149820001796 781.609267000109 788.4282689997926
App start runJsBundle	Baseline Mean: 188.719 ms Stdev: 17.692 ms (9.4%) Runs: 156 161 162 167 169 170 172 175 175 177 180 181 182 182 183 189 190 190 192 193 194 196 198 202 206 208 209 214 214 215 218 219 Current Mean: 199.656 ms Stdev: 17.296 ms (8.7%) Runs: 168 174 177 178 179 179 182 183 187 188 188 190 194 196 200 202 203 203 203 208 208 210 210 210 211 213 214 217 220 222 231 241
App start regularAppStart	Baseline Mean: 0.013 ms Stdev: 0.001 ms (6.0%) Runs: 0.0118410000577569 0.012004000134766102 0.012409999966621399 0.01245200028643012 0.012613999657332897 0.012614000122994184 0.01269499957561493 0.01293900003656745 0.012980000115931034 0.013020999962463975 0.013142999727278948 0.013143000192940235 0.013427999801933765 0.013549999799579382 0.013549999799579382 0.01355000026524067 0.013631000183522701 0.013672000262886286 0.013753000181168318 0.013793999794870615 0.01383400009945035 0.013875000178813934 0.013915999792516232 0.014119000174105167 0.014200999867171049 0.01432300009764731 0.014364000409841537 0.014566999860107899 0.014934000093489885 0.01525900000706315 Current Mean: 0.014 ms Stdev: 0.001 ms (5.3%) Runs: 0.013061999808996916 0.013224000111222267 0.013428000267595053 0.013630999717861414 0.013631000183522701 0.01371300034224987 0.013793999794870615 0.013875999953597784 0.013956999871879816 0.013956999871879816 0.0139979999512434 0.014119999948889017 0.014241999946534634 0.014282000251114368 0.01436399994418025 0.014404000248759985 0.01448499970138073 0.014526000246405602 0.014648000244051218 0.014769999776035547 0.014769999776035547 0.014810999855399132 0.015054999850690365 0.015055999625474215 0.015177000313997269 0.015299000311642885 0.015300000086426735 0.015828000381588936 0.015949999913573265 0.016032000072300434
App start nativeLaunch	Baseline Mean: 20.129 ms Stdev: 2.211 ms (11.0%) Runs: 17 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 21 21 21 21 23 23 23 23 26 26 Current Mean: 20.097 ms Stdev: 2.146 ms (10.7%) Runs: 17 17 17 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 22 22 22 22 23 23 24 26
Open Search Page TTI	Baseline Mean: 605.175 ms Stdev: 17.270 ms (2.9%) Runs: 569.3623449997976 578.8050540001132 580.1968999998644 581.7034100000747 584.7371420003474 585.3002929999493 588.2017010003328 592.1441249996424 592.4130869996734 594.5898029999807 598.3425300000235 598.4251299998723 603.1194670000114 603.1233729999512 604.746460000053 608.6879070000723 610.6042490000837 612.3327639997005 612.8538819998503 613.8345950003713 615.229411999695 619.6076659997925 620.238362999633 620.5504560000263 624.3396000000648 624.5426019998267 624.8949390002526 624.90677900007 633.5151780000888 633.9039710001089 Current Mean: 600.323 ms Stdev: 15.976 ms (2.7%) Runs: 576.9980060001835 578.9287109998986 580.8439539996907 581.1759449997917 581.3503010002896 582.4112149998546 583.2737219999544 583.8689779997803 583.9273279998451 588.1091720000841 589.0050459997728 590.2371829999611 595.1565350000747 595.3644619998522 595.9261480001733 600.0214029997587 601.3459479999729 601.6167399999686 602.6269530002028 602.9576829997823 604.5247810003348 606.2018639999442 608.568847999908 608.7139900000766 608.8649499998428 612.0704759997316 612.8096920000389 616.1691080001183 616.6960049998015 627.2088219998404 627.4858400002122 630.4606119999662 635.7317300001159

OSBotify · 2023-02-27T22:56:41Z

🚀 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 · 2023-02-28T00:00:33Z

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

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

OSBotify · 2023-03-02T22:34:29Z

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

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

fedirjh · 2023-05-23T11:32:52Z

src/components/PressableWithSecondaryInteraction/index.js

+        if (this.props.withoutFocusOnSecondaryInteraction && this.pressableRef) {
+            this.pressableRef.blur();
+        }


Hey everyone , this change caused a regression in #17868. In Safari, when copying part of a multiline message, the entire message is copied. The issue was resolved by changing the order from blurring -> onSecondaryInteraction to onSecondaryInteraction -> blurring.

allroundexperts marked this pull request as ready for review February 22, 2023 22:02

allroundexperts requested a review from a team as a code owner February 22, 2023 22:02

melvin-bot bot requested review from chiragsalian and eVoloshchak and removed request for a team February 22, 2023 22:02

eVoloshchak suggested changes Feb 22, 2023

View reviewed changes

src/components/PressableWithSecondaryInteraction/index.js Outdated Show resolved Hide resolved

allroundexperts changed the title ~~Fix #14329~~ Disable the focus trap for report page context menu Feb 23, 2023

allroundexperts force-pushed the fix-14329 branch from 2ce85d8 to 55680ab Compare February 23, 2023 15:07

allroundexperts force-pushed the fix-14329 branch from 55680ab to 58ac3e5 Compare February 23, 2023 15:09

allroundexperts force-pushed the fix-14329 branch from 3b2597d to 1d5209e Compare February 23, 2023 15:23

eVoloshchak suggested changes Feb 23, 2023

View reviewed changes

src/components/PressableWithSecondaryInteraction/index.js Outdated Show resolved Hide resolved

feat: added withoutFocusOnSecondaryInteraction prop on PressableWithS…

cd58bc8

…econdaryIntegration component

allroundexperts force-pushed the fix-14329 branch from 1d5209e to cd58bc8 Compare February 23, 2023 15:31

eVoloshchak approved these changes Feb 23, 2023

View reviewed changes

chiragsalian approved these changes Feb 23, 2023

View reviewed changes

chiragsalian assigned allroundexperts Feb 23, 2023

Luke9389 approved these changes Feb 24, 2023

View reviewed changes

Luke9389 merged commit 6c8ac04 into Expensify:main Feb 24, 2023

OSBotify mentioned this pull request Feb 27, 2023

Deploy Checklist: New Expensify 2023-02-27 #15521

Closed

29 tasks

This was referenced Mar 2, 2023

[$1000] Baseline of the recipient name and message preview do not line up in LHN #14148

Closed

[HOLD for payment 2023-03-09] [$1000] Right-clicking a long message and closing the context menu causes scrolling in chat to the clicked message #14329

Closed

tienifr mentioned this pull request Apr 24, 2023

[Paid: Final Steps] [$1000] Web - Chat - When copying part of a multiline message, the entire message is copied #17868

Closed

6 tasks

fedirjh reviewed May 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable the focus trap for report page context menu #15375

Disable the focus trap for report page context menu #15375

allroundexperts commented Feb 22, 2023 •

edited

Loading

MelvinBot commented Feb 22, 2023

allroundexperts commented Feb 23, 2023

eVoloshchak commented Feb 23, 2023

allroundexperts commented Feb 23, 2023

eVoloshchak commented Feb 23, 2023

allroundexperts commented Feb 23, 2023

eVoloshchak commented Feb 23, 2023 •

edited

Loading

eVoloshchak commented Feb 23, 2023

allroundexperts commented Feb 23, 2023 •

edited

Loading

eVoloshchak commented Feb 23, 2023

allroundexperts commented Feb 23, 2023

allroundexperts commented Feb 23, 2023

eVoloshchak left a comment

chiragsalian left a comment

github-actions bot commented Feb 24, 2023

OSBotify commented Feb 27, 2023

OSBotify commented Feb 28, 2023

OSBotify commented Mar 2, 2023

fedirjh May 23, 2023

Disable the focus trap for report page context menu #15375

Disable the focus trap for report page context menu #15375

Conversation

allroundexperts commented Feb 22, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Feb 22, 2023

allroundexperts commented Feb 23, 2023

eVoloshchak commented Feb 23, 2023

allroundexperts commented Feb 23, 2023

eVoloshchak commented Feb 23, 2023

allroundexperts commented Feb 23, 2023

eVoloshchak commented Feb 23, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

eVoloshchak commented Feb 23, 2023

allroundexperts commented Feb 23, 2023 • edited Loading

eVoloshchak commented Feb 23, 2023

allroundexperts commented Feb 23, 2023

allroundexperts commented Feb 23, 2023

eVoloshchak left a comment

Choose a reason for hiding this comment

chiragsalian left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 24, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Feb 27, 2023

OSBotify commented Feb 28, 2023

OSBotify commented Mar 2, 2023

fedirjh May 23, 2023

Choose a reason for hiding this comment

allroundexperts commented Feb 22, 2023 •

edited

Loading

eVoloshchak commented Feb 23, 2023 •

edited

Loading

allroundexperts commented Feb 23, 2023 •

edited

Loading