Fix/15086: Screen is flashing when changing focus between edit input #15201

tienifr · 2023-02-16T06:13:44Z

Details

When we move from 1 edit message input to another, the toggleReportActionComposeView is called, causing the ReportActionCompose to become visible, pushing the view upward and causing the glitch. We should only call toggleReportActionComposeView when the keyboard is completely hidden and this PR does that

Fixed Issues

$ #15086
$ #15086 (comment)

Tests

Login with any account
Try to select any report, send few messages, try to edit them but doesn’t press Save. Try to change the focus
Verify that the screen isn't flashing when switching focus between the edit fields.

Verify that no errors appear in the JS console

Offline tests

QA Steps

Login with any account
Try to select any report, send few messages, try to edit them but doesn’t press Save. Try to change the focus
Verify that the screen isn't flashing when switching focus between the edit fields.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-02-16.at.16.55.53.mp4

Mobile Web - Chrome

330453922_6101100966580019_8678114187605412338_n.1.mp4

Mobile Web - Safari

Screen.Recording.2023-02-16.at.16.56.51.mp4

Desktop

Screen.Recording.2023-02-16.at.17.00.53.mp4

iOS

Screen.Recording.2023-02-16.at.17.06.11.1.mp4

Android

Screen.Recording.2023-02-16.at.17.21.23.mp4

MelvinBot · 2023-02-16T08:04:43Z

@thienlnam @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]

…n only be triggered when close edit message

tienifr · 2023-02-17T08:34:24Z

Hi @mananjadhav @thienlnam, Please help to review this PR. Thanks

mananjadhav · 2023-02-17T08:44:32Z

On my list today. Focused on email getOptions task for the time being.

mananjadhav · 2023-02-17T19:14:51Z

src/pages/home/report/ReportActionItemMessageEdit.js

@@ -244,12 +247,18 @@ class ReportActionItemMessageEdit extends React.Component {
                            toggleReportActionComposeView(false, this.props.isSmallScreenWidth);
                        }}
                        onBlur={(event) => {
+                            const relatedTargetId = lodashGet(event, 'nativeEvent.relatedTarget.id');


nab, but I don't see why we had to change this.

mananjadhav · 2023-02-17T19:21:54Z

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

web-edit-message-screen-flicker.mov

Mobile Web - Chrome

mweb-chrome-edit-message-screen-flicker.mov

Mobile Web - Safari

mweb-safari-edit-message-screen-flicker.mov

Desktop

desktop-edit-message-screen-flicker.mov

iOS

ios-message-screen-flicker.mov

Android

android-edit-message-screen-flicker.mov

mananjadhav · 2023-02-17T19:22:53Z

@thienlnam All yours.

@tienifr Quick suggestion, I saw last few commits had the same commit message. I would recommend writing meaningful messages that explains the context of the commit. Thanks for the quick PR on this one. Appreciate it.

github-actions · 2023-02-17T20:25:07Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	683.880 ms → 723.541 ms (+39.661 ms, +5.8%)
App start runJsBundle	180.516 ms → 189.759 ms (+9.242 ms, +5.1%)
App start nativeLaunch	20.862 ms → 20.867 ms (+0.005 ms, ±0.0%)
App start regularAppStart	0.016 ms → 0.015 ms (-0.001 ms, -6.8%)
Open Search Page TTI	616.057 ms → 612.361 ms (-3.695 ms, -0.6%)

Show details

Name	Duration
App start TTI	Baseline Mean: 683.880 ms Stdev: 20.127 ms (2.9%) Runs: 640.5332760000601 643.583104999736 645.0769959995523 657.3026329996064 660.3962009996176 661.6043670000508 663.7550189998001 673.3396269995719 676.1821779999882 677.9270949997008 678.5462899999693 682.0754399998114 682.7335500000045 683.0728930002078 683.5170139996335 683.7560259997845 687.8628970002756 689.2201850004494 689.3250569999218 693.2150370003656 694.1703049996868 695.3870190000162 701.3097689999267 701.921156000346 703.8645770000294 704.1202189996839 704.3447049995884 704.7865209998563 710.3596310000867 712.4009790001437 714.5826000003144 Current Mean: 723.541 ms Stdev: 21.772 ms (3.0%) Runs: 684.2921749996021 688.7699039997533 694.9382539996877 695.2978959996253 696.9777939999476 702.0004209997132 704.3307600002736 708.4575070003048 712.474721999839 715.4316349998116 716.1941080000252 718.2228309996426 718.2903880001977 718.3193220002577 718.5429929997772 719.9785209996626 721.3027729997411 722.7120289998129 725.6719340002164 728.7184969997033 732.3698110003024 732.8827309999615 737.1819430002943 737.4865950001404 739.5725379996002 739.7116790004075 748.9945729998872 752.91579800006 757.1831670003012 763.5856140004471 776.9522670004517
App start runJsBundle	Baseline Mean: 180.516 ms Stdev: 11.728 ms (6.5%) Runs: 160 162 164 165 166 169 172 173 174 174 175 176 177 177 177 178 178 179 182 184 188 188 188 190 193 193 194 195 200 201 204 Current Mean: 189.759 ms Stdev: 10.605 ms (5.6%) Runs: 176 177 177 179 179 179 179 180 180 183 183 184 186 187 189 189 189 194 195 195 196 198 199 199 199 201 206 206 219
App start nativeLaunch	Baseline Mean: 20.862 ms Stdev: 1.676 ms (8.0%) Runs: 17 18 19 19 19 19 20 20 20 20 20 20 20 21 21 21 21 22 22 22 22 22 22 22 22 23 23 24 24 Current Mean: 20.867 ms Stdev: 1.765 ms (8.5%) Runs: 17 18 19 19 19 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 22 22 22 23 23 24 24 24 24
App start regularAppStart	Baseline Mean: 0.016 ms Stdev: 0.001 ms (6.5%) Runs: 0.01428299956023693 0.01448499970138073 0.014852000400424004 0.014973999932408333 0.015054999850690365 0.015137000009417534 0.015176999382674694 0.015217999927699566 0.015259000472724438 0.015298999845981598 0.015421999618411064 0.015544000081717968 0.015544000081717968 0.0157880000770092 0.016032000072300434 0.016112999990582466 0.016276000067591667 0.016316000372171402 0.016601999290287495 0.01664199959486723 0.016764000058174133 0.016886000521481037 0.017048999667167664 0.017049000598490238 0.017090000212192535 0.017130999825894833 0.017456000670790672 0.017822000198066235 0.018635999411344528 Current Mean: 0.015 ms Stdev: 0.001 ms (4.0%) Runs: 0.013793999329209328 0.013916000723838806 0.014078999869525433 0.01416000071913004 0.014281999319791794 0.014282000251114368 0.0143630001693964 0.01444500032812357 0.014526999555528164 0.014566999860107899 0.014688999392092228 0.014973999932408333 0.015054999850690365 0.015056000091135502 0.015095999464392662 0.015096000395715237 0.015177000313997269 0.015178000554442406 0.015259000472724438 0.01534000039100647 0.01534000039100647 0.015381000004708767 0.015421999618411064 0.01550199929624796 0.015502999536693096 0.015544000081717968 0.015625 0.015665999613702297 0.015951000154018402 0.016153999604284763
Open Search Page TTI	Baseline Mean: 616.057 ms Stdev: 24.369 ms (4.0%) Runs: 569.0596930002794 578.6653650002554 580.2514239996672 585.4323729993775 589.2961430000141 591.2554119993001 595.7294110003859 600.3218999998644 603.7272140001878 604.0758869992569 604.5852049998939 606.2316900007427 607.0135909998789 607.1797289997339 608.1683359993622 612.090657999739 614.8725180001929 617.8495279997587 618.9895029999316 620.7725430000573 622.250163000077 623.0248619997874 628.5310469996184 629.7152509996668 633.8287360006943 638.271809999831 640.1977949999273 643.8157959999517 651.8096519997343 656.6109619997442 659.5093589993194 670.6827809996903 Current Mean: 612.361 ms Stdev: 22.475 ms (3.7%) Runs: 572.1167000001296 575.171590000391 577.3401699997485 582.5426030000672 586.3274339996278 588.0142829995602 591.6241459995508 592.3232829999179 595.1125499997288 600.8301189998165 602.2818599995226 603.6640220005065 604.9175619995221 605.0448399996385 606.838217000477 609.2598470002413 612.8201090004295 612.8960779998451 614.3682860005647 616.4528410006315 620.348348999396 620.891072999686 622.4200440002605 631.7154959999025 632.2601319998503 632.5320640001446 632.5526940003037 633.4675709996372 635.1398120000958 639.119384999387 644.7867850000039 645.378703000024 667.3633220000193

OSBotify · 2023-02-17T21:23:33Z

🚀 Deployed to staging by https://github.com/thienlnam in version: 1.2.74-0 🚀

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

OSBotify · 2023-02-21T20:34:16Z

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.2.74-0 🚀

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

s77rt · 2023-03-28T16:09:46Z

src/pages/home/report/ReportActionItemMessageEdit.js

+                            if (this.messageEditInput === relatedTargetId) {
+                                return;
+                            }
+                            openReportActionComposeViewWhenClosingMessageEdit(this.props.isSmallScreenWidth);


Why do we still need to delay opening the main composer? Isn't the relatedTargetId check enough?

@mananjadhav @tienifr

I also think the same.

I see that relatedTargetId only exists for web. But we are delaying opening the main composer for every single blur event (including those that are not related to the edit input).

s77rt · 2023-09-21T14:53:18Z

src/libs/openReportActionComposeViewWhenClosingMessageEdit/index.native.js

+    const keyboardDidHideListener = Keyboard.addListener('keyboardDidHide', () => {
+        toggleReportActionComposeView(true, isSmallScreenWidth);
+        keyboardDidHideListener.remove();
+    });


This caused a regression (case 1). If we focus on message A then switch to message B then message A will be blurred and we register the keyboardDidHide callback. Now if we press the back button to hide the keyboard (while message B is still focused) the main composer will show up.

What we had to do is to remove that callback as soon as a new message is focused.

tienifr added 3 commits February 16, 2023 13:10

fix: toggle composer input only when the keyboard hide completely

84058e0

fix: toggle composer input only when the keyboard hide completely

f7b3c5c

fix: toggle composer input only when the keyboard hide completely

d41fad7

tienifr marked this pull request as ready for review February 16, 2023 08:04

tienifr requested a review from a team as a code owner February 16, 2023 08:04

melvin-bot bot requested review from mananjadhav and thienlnam and removed request for a team February 16, 2023 08:04

tienifr added 2 commits February 16, 2023 15:18

fix: toggle composer input only when the keyboard hide completely

ff2c8b2

fix: create toggleReportActionComposeViewWhenCloseEditMessage functio…

e50dbb0

…n only be triggered when close edit message

tienifr marked this pull request as draft February 16, 2023 08:34

fix: create toggleReportActionComposeViewWhenCloseEditMessage functio…

9dc3b00

…n only be triggered when close edit message

tienifr marked this pull request as ready for review February 16, 2023 09:13

tienifr mentioned this pull request Feb 16, 2023

[HOLD for payment 2023-02-28] [$1000] Screen is flashing when changing focus between edit input #15086

Closed

1 task

tienifr added 3 commits February 16, 2023 16:17

fix: issue

ffda9fc

fix: delete redundant

50bb6bb

fix: mweb bug

aafc79f

mananjadhav reviewed Feb 17, 2023

View reviewed changes

mananjadhav approved these changes Feb 17, 2023

View reviewed changes

thienlnam approved these changes Feb 17, 2023

View reviewed changes

thienlnam merged commit fefb521 into Expensify:main Feb 17, 2023

OSBotify mentioned this pull request Feb 17, 2023

Deploy Checklist: New Expensify 2023-02-17 #15260

Closed

17 tasks

s77rt mentioned this pull request Mar 28, 2023

[$8000] Compose box disappears when user edit message and press "New message" button. #15216

Closed

1 task

s77rt reviewed Mar 28, 2023

View reviewed changes

s77rt reviewed Sep 21, 2023

View reviewed changes

s77rt mentioned this pull request Sep 21, 2023

[HOLD for payment 2023-09-27] [$2000] Chat - Main composer is displayed when editing two messages #23908

Closed

6 tasks

s-alves10 mentioned this pull request Oct 30, 2023

[$500] Chat - Input field briefly disappears after saving edited message #29211

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/15086: Screen is flashing when changing focus between edit input #15201

Fix/15086: Screen is flashing when changing focus between edit input #15201

tienifr commented Feb 16, 2023 •

edited

Loading

MelvinBot commented Feb 16, 2023

tienifr commented Feb 17, 2023

mananjadhav commented Feb 17, 2023

mananjadhav Feb 17, 2023

mananjadhav commented Feb 17, 2023

mananjadhav commented Feb 17, 2023

github-actions bot commented Feb 17, 2023

OSBotify commented Feb 17, 2023

OSBotify commented Feb 21, 2023

s77rt Mar 28, 2023

s77rt Mar 28, 2023

parasharrajat Mar 28, 2023

s77rt Mar 29, 2023

s77rt Sep 21, 2023

Fix/15086: Screen is flashing when changing focus between edit input #15201

Fix/15086: Screen is flashing when changing focus between edit input #15201

Conversation

tienifr commented Feb 16, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Feb 16, 2023

tienifr commented Feb 17, 2023

mananjadhav commented Feb 17, 2023

mananjadhav Feb 17, 2023

Choose a reason for hiding this comment

mananjadhav commented Feb 17, 2023

Reviewer Checklist

Screenshots/Videos

mananjadhav commented Feb 17, 2023

github-actions bot commented Feb 17, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Feb 17, 2023

OSBotify commented Feb 21, 2023

s77rt Mar 28, 2023

Choose a reason for hiding this comment

s77rt Mar 28, 2023

Choose a reason for hiding this comment

parasharrajat Mar 28, 2023

Choose a reason for hiding this comment

s77rt Mar 29, 2023

Choose a reason for hiding this comment

s77rt Sep 21, 2023

Choose a reason for hiding this comment

tienifr commented Feb 16, 2023 •

edited

Loading