Fix mWeb Compose Focus bug #14230

bondydaa · 2023-01-12T00:15:07Z

Details

Buckle up this is a doozy.

Bug exists because React Native handles UX differently between web/mWeb/Desktop and native platform - for iOS and Android when a TextInput has focus tapping/clicking away from that input won't "blur" the input and focus will remain unless another TextInput is given focus. Web/mWeb/Desktop allows focus to blur how you would expect by clicking/tapping outside of an input.

With how can canFocusInputOnScreenFocus to detect between platform it is wrongly assuming b/c mWeb has touch events that the Native UX for focus will be there and that we don't need to trigger the code to manually give focus back to the compose component.

We are mainly focusing on fixing the bug here for now. We'll spin up a new issue to address the UX inconsistencies around the focusing of the compose component in a new issue.

Fixed Issues

$ #13443

Tests

Verify that no errors appear in the JS console

Offline tests

N/A same as QA steps since this is just some UX that doesn't involve specific offline behavior

QA Steps

Verify that no errors appear in the JS console

Click into any chat.
Focus (click/tap) into the compose input at the bottom
Click the + icon to reveal the add attachment or split/request money modal
click/tap away from the modal to dismiss it
confirm focus is returned to the compose input
Click the + icon to reveal the add attachment or split/request money modal
Click to send money, on the modal that appears click the x to dismiss it.
(Existing Behavior) On Web/mWeb/Desktop focus should be given back to the compose input. On Android / iOS focus should not be given back to the compose input. One could argue that these behaviors should be the same and they would be right but this functionality was previously coded this way. We are going to fix it in a future issue.

PR Author Checklist

Screenshots/Videos

Web

2023-01-13_15-22-23.mp4

Mobile Web - Chrome

android-chrome.mp4

Mobile Web - Safari

ios-safari.mp4

Desktop

desktop.mp4

iOS

ios-native.mp4

Android

android-native.mp4

…s back to the compose input component

…ocus-bug

…on compose button when modals close

…on for variable in composer component

melvin-bot · 2023-01-13T22:35:10Z

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

marcaaron

Looks pretty good to me just made some code clean up suggestions. Really great detective work on this one. Feel like we got to the root cause here and then presented a solution that addresses it well.

src/pages/home/report/ReportActionCompose.js

…ocus-bug

marcaaron

Nice changes thanks!

eVoloshchak · 2023-01-23T12:06:54Z

The code looks good!

I've found an issue when choosing "Add attachment" option: when you dismiss attachment modal, the input is correctly focused, but it's hidden by the keyboard because the screen didn't scroll up.

video_2023-01-23_14-01-05.mp4

I've tested this on Pixel 4 emulator and it doesn't have this problem, so this is most certainly depends on the flavor of Android.

bondydaa · 2023-01-23T16:09:49Z

😭 oh bother... I think I saw some other discussions or maybe code comments about this, @marcaaron do you remember anything about this?

I've definitely noticed that same thing happening on other apps too (not saying we shouldn't fix it) so that might be a somewhat common problem people end up needing to fix, going to see if I can see what the internet has to say about it.

marcaaron · 2023-01-24T00:54:25Z

I think I saw some other discussions or maybe code comments about this, @marcaaron do you remember anything about this?

I don't sorry 😞

bondydaa · 2023-01-26T00:13:35Z

okay sorry I haven't had a chance to dig back in on this one.

@eVoloshchak which emulator is your video from? I'm going to see about testing this on my phone (pixel 6) to see if it's reproducible there and will report back.

eVoloshchak · 2023-01-26T10:29:40Z

@bondydaa, this video is shot on a physical device, Samsung Note 10 running Android 12

While on Pixel 4 emulator running Android 13 the issue doesn't exist

bondydaa · 2023-01-26T16:11:54Z

ah awesome.

My pixel is on Android 13 so I'll test again today with it and see what happens.

bondydaa · 2023-01-27T21:40:03Z

okay after getting this branch built on my phone (pixel 6 android 13) I wasn't able to reproduce either.

Wonder if maybe it's an android 12 thing or maybe specific to Samsungs even?

bondydaa · 2023-01-27T21:45:25Z

I have a samsung tablet with android 13 that I'm going to see if I can build the app on 🙏

bondydaa · 2023-01-27T21:51:16Z

Yep I can't reproduce on my samsung tablet with android 13 so I think it's probably something with android 12 then.

I'm not sure what Android versions we fully support? The hope is we end up cleaning up the behavior on a follow up issue with focus going back and forth and hiding/showing the keyboard so I think we could probably merge this and clean up anything else on a follow up.

eVoloshchak · 2023-01-31T13:17: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.
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-31.at.15.24.18.mov

Mobile Web - Chrome

Screen_Recording_20230131-153539_Chrome.mp4

Mobile Web - Safari

Screen.Recording.2023-01-31.at.15.32.35.mov

Desktop

Screen.Recording.2023-01-31.at.15.26.14.mov

iOS

Screen.Recording.2023-01-31.at.15.30.50.mov

Android

Screen_Recording_20230131-153713_New.Expensify.mp4

eVoloshchak

LGTM!

There is a visual bug when using this branch, so tested with changes applied to main

OSBotify · 2023-01-31T16:52:58Z

✋ 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 · 2023-01-31T17:20:19Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	671.028 ms → 684.026 ms (+12.998 ms, +1.9%)
App start runJsBundle	185.161 ms → 192.438 ms (+7.276 ms, +3.9%)
Open Search Page TTI	629.001 ms → 631.665 ms (+2.664 ms, ±0.0%)
App start nativeLaunch	20.065 ms → 20.500 ms (+0.435 ms, +2.2%)
App start regularAppStart	0.014 ms → 0.014 ms (+0.001 ms, +4.1%)

Show details

Name	Duration
App start TTI	Baseline Mean: 671.028 ms Stdev: 40.392 ms (6.0%) Runs: 616.5758210001513 616.7320720003918 617.4101820001379 623.6614250000566 628.8030190002173 629.5431030001491 640.494028000161 641.6922909999266 644.1668239999563 646.0486080003902 646.6913040000945 648.1070940000936 650.5409479998052 652.5798369999975 655.542011000216 660.0166389998049 666.3264800002798 673.3585249995813 680.6151339998469 688.1040599998087 690.1060319999233 691.6169349998236 695.5707780001685 697.9167090002447 707.4084339998662 713.4981490001082 715.491109999828 715.624662999995 724.1311550000682 732.0067309997976 791.4814680004492 Current Mean: 684.026 ms Stdev: 30.906 ms (4.5%) Runs: 629.6756379995495 636.2693029996008 649.1408559996635 653.2232919996604 658.5323430001736 661.0332730002701 662.0452190004289 662.7427780004218 664.0252179997042 664.4101619999856 665.5790440002456 666.1074409997091 667.6457270001993 669.4644879996777 676.520596999675 679.3724389998242 680.4018550002947 680.8251590002328 681.7152319997549 682.205156000331 684.9259829996154 694.4615249997005 697.1495279995725 698.7217020001262 707.0314790001139 708.6139089995995 710.6525079999119 724.8939140001312 726.4570110002533 730.6185039998963 744.4178700000048 769.9581390004605
App start runJsBundle	Baseline Mean: 185.161 ms Stdev: 24.825 ms (13.4%) Runs: 160 161 161 162 162 164 165 166 168 168 168 168 169 171 171 172 177 179 187 189 190 196 197 198 211 213 214 218 219 244 252 Current Mean: 192.438 ms Stdev: 25.570 ms (13.3%) Runs: 158 160 160 160 162 165 169 172 175 177 178 180 180 183 186 189 191 191 194 195 195 197 199 209 210 215 219 220 233 240 246 250
Open Search Page TTI	Baseline Mean: 629.001 ms Stdev: 26.371 ms (4.2%) Runs: 585.4354650005698 587.716878999956 602.6089690001681 603.6357430005446 604.5129810003564 606.1318359998986 606.5026449998841 607.9683019993827 609.7104090005159 609.8369140001014 612.6914059994742 613.5890709999949 617.2115070000291 619.7681479994208 622.4165049996227 623.1671959999949 623.2275799997151 626.5145670007914 628.1506350003183 630.2061769999564 630.5311689991504 632.572061999701 633.183633999899 638.9839679999277 640.7200929997489 641.1187340002507 647.2248540008441 655.6860760003328 664.7565509993583 678.24768100027 683.6659349994734 684.2098799999803 685.1139730000868 Current Mean: 631.665 ms Stdev: 36.037 ms (5.7%) Runs: 588.3165690004826 591.2798260003328 593.3416750002652 596.67602599971 600.8917240006849 601.7498789997771 602.081664999947 605.0856119999662 607.2323409998789 609.2507729995996 612.0866289995611 612.578451000154 613.8933520000428 614.8304039994255 616.300578000024 616.7417000001296 617.0933839995414 621.3214529994875 623.6826579999179 628.6320399995893 628.8175869993865 629.3457040004432 631.4822599999607 635.400349999778 652.569132999517 657.0693359998986 659.1306560002267 666.0907800002024 669.562663000077 693.7355140000582 710.6684980001301 712.0010580001399 725.9925939999521
App start nativeLaunch	Baseline Mean: 20.065 ms Stdev: 2.124 ms (10.6%) Runs: 17 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 21 21 21 22 22 22 24 24 25 25 Current Mean: 20.500 ms Stdev: 2.773 ms (13.5%) Runs: 18 18 18 18 18 18 18 18 18 18 19 19 19 19 19 20 20 20 20 20 20 21 22 22 22 22 23 23 25 26 27 28
App start regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (5.9%) Runs: 0.011962000280618668 0.012370000593364239 0.012736000120639801 0.012817000038921833 0.01285799965262413 0.012898999266326427 0.012898999266326427 0.013061000034213066 0.013101999647915363 0.0131029998883605 0.013224000111222267 0.013264999724924564 0.013427999801933765 0.013468999415636063 0.013508999720215797 0.01355000026524067 0.013590999878942966 0.013956999406218529 0.014038000255823135 0.014078999869525433 0.014120000414550304 0.014161000028252602 0.014240999706089497 0.014282000251114368 0.01428299956023693 0.014526000246405602 0.014526000246405602 0.014852000400424004 0.015829000622034073 Current Mean: 0.014 ms Stdev: 0.001 ms (5.1%) Runs: 0.012736000120639801 0.013020999729633331 0.013020999729633331 0.013264999724924564 0.013427999801933765 0.013469000346958637 0.01371300034224987 0.013794000260531902 0.0138349998742342 0.013915999792516232 0.013915999792516232 0.013917000032961369 0.013957000337541103 0.01407800056040287 0.01407800056040287 0.01416000071913004 0.014241999946534634 0.014364000409841537 0.014444999396800995 0.014527000486850739 0.014607999473810196 0.014728999696671963 0.0147299999371171 0.014770999550819397 0.014851999469101429 0.015096000395715237 0.015176999382674694 0.015258999541401863 0.01590999960899353

OSBotify · 2023-01-31T20:32:32Z

🚀 Deployed to staging by https://github.com/bondydaa in version: 1.2.63-0 🚀

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

OSBotify · 2023-02-01T23:59:10Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.63-0 🚀

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

0xmiroslav · 2023-05-25T09:13:31Z

src/pages/home/report/ReportActionCompose.js

                                    >
                                        <Composer
-                                            autoFocus={!this.props.modal.isVisible && (this.shouldFocusInputOnScreenFocus || this.isEmptyChat())}
+                                            autoFocus={!this.props.modal.isVisible && (this.willBlurTextInputOnTapOutside || this.isEmptyChat())}


Coming from #17187:
This was overlooked while replacing all shouldFocusInputOnScreenFocus occurrences. Actually no changes needed here. Native and mWeb should be consistent in autofocus behavior.

bondydaa added 3 commits January 10, 2023 11:07

add log line to help confirm if this code is ever run

9360d83

hot fix a bug so that mWeb beahaves like Web in regard to giving focu…

4ea07c9

…s back to the compose input component

remove unneeded log line, add comment

64b83e0

bondydaa self-assigned this Jan 12, 2023

bondydaa added 7 commits January 12, 2023 15:56

Merge branch 'main' of github.com:Expensify/App into bondy-fix-mweb-f…

6bb07d6

…ocus-bug

re-add this logic so we dont cause regression on sign in pages

4884bdc

set variable based on platform type so we know if we need to refocus …

9f33b93

…on compose button when modals close

remove unused import now

638a35c

remove unused import

80ce3c3

use separate native files to determine booleans, fix naming convencti…

57528b4

…on for variable in composer component

remove unused import

0e1d944

bondydaa requested a review from marcaaron January 13, 2023 22:34

bondydaa marked this pull request as ready for review January 13, 2023 22:34

bondydaa requested a review from a team as a code owner January 13, 2023 22:34

melvin-bot bot requested review from ctkochan22 and eVoloshchak and removed request for a team January 13, 2023 22:35

marcaaron requested changes Jan 17, 2023

View reviewed changes

src/pages/home/report/ReportActionCompose.js Outdated Show resolved Hide resolved

src/pages/home/report/ReportActionCompose.js Outdated Show resolved Hide resolved

bondydaa added 2 commits January 19, 2023 10:03

Merge branch 'main' of github.com:Expensify/App into bondy-fix-mweb-f…

f479419

…ocus-bug

rename variable and files, thanks chatgpt

89722bf

bondydaa requested a review from marcaaron January 19, 2023 17:26

marcaaron approved these changes Jan 20, 2023

View reviewed changes

bondydaa mentioned this pull request Jan 23, 2023

[$1000] [HOLD for payment 2023-02-08] mWeb/Chrome - Focus is lost and the keyboard does not reopen after pressing "+" button #13443

Closed

ctkochan22 approved these changes Jan 23, 2023

View reviewed changes

trjExpensify mentioned this pull request Jan 27, 2023

[HOLD for payment 2023-09-04] [$4000] Keyboard shows again briefly when clicking "Add attachment" while the composer is focused #13922

Closed

2 tasks

eVoloshchak approved these changes Jan 31, 2023

View reviewed changes

bondydaa merged commit 9385408 into main Jan 31, 2023

bondydaa deleted the bondy-fix-mweb-focus-bug branch January 31, 2023 16:47

OSBotify mentioned this pull request Jan 31, 2023

Deploy Checklist: New Expensify 2023-01-31 #14709

Closed

14 tasks

bondydaa mentioned this pull request Mar 15, 2023

[Tracking] Fix Composer Component Focus Issues #15992

Closed

20 tasks

0xmiroslav mentioned this pull request Apr 18, 2023

[HOLD for payment 2023-05-23] [$1000] Chat - The cursor absent in the chat input field #17187

Closed

6 tasks

flodnv mentioned this pull request May 11, 2023

Fix: Auto focus on mWeb when open report with enough chat history #18699

Merged

57 tasks

0xmiroslav reviewed May 25, 2023

View reviewed changes

Fix mWeb Compose Focus bug #14230

Fix mWeb Compose Focus bug #14230

Uh oh!

Conversation

bondydaa commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Jan 13, 2023

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

eVoloshchak commented Jan 23, 2023

Uh oh!

bondydaa commented Jan 23, 2023

Uh oh!

marcaaron commented Jan 24, 2023

Uh oh!

bondydaa commented Jan 26, 2023

Uh oh!

eVoloshchak commented Jan 26, 2023

Uh oh!

bondydaa commented Jan 26, 2023

Uh oh!

bondydaa commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bondydaa commented Jan 27, 2023

Uh oh!

bondydaa commented Jan 27, 2023

Uh oh!

eVoloshchak commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

eVoloshchak left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Jan 31, 2023

Uh oh!

github-actions bot commented Jan 31, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

Uh oh!

OSBotify commented Jan 31, 2023

Uh oh!

OSBotify commented Feb 1, 2023

Uh oh!

0xmiroslav May 25, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bondydaa commented Jan 12, 2023 •

edited

Loading

bondydaa commented Jan 27, 2023 •

edited

Loading

eVoloshchak commented Jan 31, 2023 •

edited

Loading