[HOLD for payment 2023-04-10] [$1000] [Regression] Request Money - Amount field not auto focused if suddenly loose focus

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Issue found when executing PR https://github.com/Expensify/App/pull/16394

Action Performed:

1. Launch expensify app
2. Login with any account
3. Tap on the FAB button -> Send money
4. Enter numbers
5. Click outside of numpad
6. Enter numbers

Expected Result:

Autofocus Amount field if suddenly loose focus

Actual Result:

Amount field not auto focused if suddenly loose focus

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [x] MacOS / Desktop

Version Number: 1.2.89.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/227730584-334957a6-c2e6-4b3d-8e2e-2baa715af479.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01dbf046ada3294608
• Upwork Job ID: 1640296677002747904
• Last Price Increase: 2023-03-27

[MelvinBot]

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [x] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [x] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[luacmartins]

@mountiny @0xmiroslav @narefyev91 since you worked on https://github.com/Expensify/App/pull/16394. Tapping outside the input doesn't allow for further inputs

[mountiny]

Thanks for the bump here, I will take this one on with @narefyev91 again. Changes to this page open a can of worms sorry

[0xmiros]

@mountiny @0xmiroslav @narefyev91 since you worked on #16394. Tapping outside the input doesn't allow for further inputs

yup that's what I was thinking of but solution of just fixing mWeb side went ahead

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01dbf046ada3294608

[MelvinBot]

Current assignee @sonialiap is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

Current assignee @0xmiroslav is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

Current assignee @mountiny is eligible for the External assigner, not assigning anyone new.

[narefyev91]

I'm Nikolay from Callstack - and i will take a look on this one

[MelvinBot]

📣 @narefyev91 You have been assigned to this job by @mountiny! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[mountiny]

@0xmiroslav sorry it wasnt clear from you comment on the PR, your suggestion seemed to introduce a different regression.

Let's try to properly investigate this page and get the best cross platform solution, I truest in @0xmiroslav and @narefyev91 skills here to get to the best solution together 🤝 thanks 🙇

[0xmiros]

sorry it wasnt clear from you comment on the PR, your suggestion seemed to introduce a https://github.com/Expensify/App/pull/16394#issuecomment-1480735810 regression

yes I knew that so that's why I didn't persist to use my approach. We should find out way of fixing both issues on web/desktop:

• clicking on any area don't lose focus (alternative is to use similar approach what we did in mWeb, so listen keydown event and manually focus but not sure I follow that approach)
• input numbers should be selectable and cursor should be moveable as normal

[narefyev91]

@mountiny @0xmiroslav As a workaround seems we need to have custom event handler to detect onMouseDown event and only preventDefault() if we click on main wrapper, but not on input (to allow user to move cursor)

    /**
     * Listens for mouse down event and applies the action
     *
     * @param {Object} e
     */
    handleMouseDown(e) {
        if (e.target.nodeName !== 'DIV') { return; }
        e.preventDefault();
    }

https://user-images.githubusercontent.com/28352309/227934461-f73056bc-a039-4e24-bf23-9b716346565d.mov

[mountiny]

Currency selector will still work fine?

[narefyev91]

@mountiny yup

https://user-images.githubusercontent.com/28352309/227936140-6f158aa3-69ad-437a-8772-266243f091c7.mov

[mountiny]

Nice, curious for @0xmiroslav thoughts!

Thanks!

[0xmiros]

It seems work but as @narefyev91 said, it's a workaround. Any chance of finding better solutions?

[narefyev91]

@0xmiroslav sorry for workaround word LOL - possible solutions here which i see: 1) block loosing focus for input field 2) autofocus field if focus is lost

1. For blocking loose focus - we should prevent click inside main View (which you already suggested)
2. For autofocus on field we can also use the same idea we have fixes for all other platforms - (but for me personally it's flashing when setting this logic for text input).

https://user-images.githubusercontent.com/28352309/227953811-46556713-e0f5-4cdf-abe0-c8fd8c6be504.mov

Based on what i see we can choose between this 2 options

[0xmiros]

I also prefer 1 - block loosing focus for input field But was just looking for better implementation - to confirm preventDefault() is the only way

[0xmiros]

but for me personally it's flashing when setting this logic for text input

Do you think this is introduced by our solution. This already happens on main. Related issues: https://github.com/Expensify/App/issues/15957 https://github.com/Expensify/App/issues/16017 https://github.com/Expensify/App/issues/16082 caused by #15100

[narefyev91]

@0xmiroslav yup it seems coming from this line which I commented out

        input:focus-visible, input:focus[data-focusvisible-polyfill],
        select:focus-visible, select:focus[data-focusvisible-polyfill]  {
            box-shadow: none;
            /*animation: blink_input_opacity_to_prevent_scrolling_when_focus 0.01s;*/
        }

[0xmiros]

onMouseDown approach will cause accessibility issue in the future. i.e. focus Next button by pressing Tab several times and clicking anywhere doesn't unfocus button. Same happens to currency symbol or input selection. Not sure this should be considered as bug.

3rd approach is to focus on callback of onBlur event in input. What do you think?

[narefyev91]

@0xmiroslav yup this also works but manually force input to focus - show the same flickering issue. If we want to move out of MoustDown - we need to remove that animation flick

[0xmiros]

Hmm, not sure we should handle flickering issue here. But yes, I prefer preventing blur to focusing after blur. If we don't care about accessibility issue I mentioned above, I think we're good to go with mousedown approach (seems no better solutions) @mountiny what do you suggest?

[narefyev91]

@mountiny @0xmiroslav Based on our workaround we have: 1) MouseDown={this.focusInput} - will bring flickering issue 2) use onBlur={this.focusInput} - will bring flickering issue (also breaking tab click working - because we autofocusing field on onBlur) 3) MouseDown={this.handleMouseDown} - will bring accessibility issues like highlighting element, not possible to remove selection (as example)

I will stick with first approach - flickering issue should be fixed globally

[0xmiros]

4. your original solution introduced in mWeb?

[narefyev91]

@0xmiroslav solution was to focus field when user start typing... not sure if it will be possible to add same logic here

[0xmiros]

@0xmiroslav solution was to focus field when user start typing... not sure if it will be possible to add same logic here

Not same logic. Needs to add another complex logic on page itself to listen keydown event, not on TextInput. So not recommended than others.

[mountiny]

I will stick with first approach - flickering issue should be fixed globally

Is the flickering issue with TextInput resolved by enabling Fabric

[narefyev91]

@mountiny hard to say - I can't find issue in github to check why those changes were implemented - but seems after that changes were merged - issue started to be tracked - and in case it's direct css manipulation - not sure if something will be changed after fabric will be enabled, but i can be wrong

[mountiny]

Quite a tough nut to crack this one. What if we take a step back further looking at how this page is implemented, is there something which gives us these limitations but we could maybe refactor for something better. We will go to hooks soon so we will refactor this nonetheless

[narefyev91]

Sure i will take a look at different angle

[ahmedGaber93]

replace View warpper with Pressable and use one of this props onResponderStart to prevent default click or onPressIn and focus input again

import { Pressable } from 'react-native';

<Pressable
      onResponderStart={e => e.preventDefault()}

//or focus input again when parent is press in

<Pressable
      onPressIn={() => this.textInput.focus()}

result of onResponderStart={e => e.preventDefault()}

https://user-images.githubusercontent.com/41129870/228066621-dddaf600-d4e6-4899-8a31-affa48564364.mov

Contributor details Your Expensify account email: a.gaber.dev@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~012883934e2c4dc4e7

[MelvinBot]

📣 @ahmedGaber93! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[narefyev91]

@mountiny @0xmiroslav The idea from ahmedGaber93 is also partially works - to switch to onResponderStart={e => e.preventDefault()} this one - but again it still the same workaround which we discussed yesterday. Based on my view - after checking deeper the logic around the screens: if we simplify what we currently have - Container -> Input -> Button (NumPad). In any case for web we need to control click outside based on event (onMouseDown, onResponderStart, onBlur) - all this workflows are correct - and the only issue we facing now in most cases - it's flickering issue. To avoid this issue we need to remove clicking event outside of input. And we coming again to the same proposals we have yesterday. I think we need to choose one of the approach and move forward

[mountiny]

Thank you!

@0xmiroslav @narefyev91 which one would you prefer

[0xmiros]

I prefer onBlur but focus only when blurred from clicking outside (black area). So it feels like entire black area is input view i.e. don't focus when blurred by tab

[narefyev91]

I prefer to focus on input field when we click outside - MouseDown={this.focusInput}

[mountiny]

I think both will bring in the flickering issue, but lets just go with what is more simple which (to me) feels like the click outside mouseDown approach from @narefyev91

Does that work @0xmiroslav or any strong argument why not to go that way

[0xmiros]

I don't like calling focus on onMouseDown.

• this will cause regression of currency symbol not pressed (visual effect)

https://user-images.githubusercontent.com/97473779/228274538-70ce6bf0-954b-4c5d-8d6f-83be2834da3c.mov

• this may depend on timeout or interaction manager because input will be blurred again after immediate focus this is call order: onMouseDown -> focus() -> onFocus -> onBlur

[mountiny]

Thanks, those are good arguments for not going that way.

@narefyev91 any arguments for not going the onBlur way @0xmiroslav proposed?

[narefyev91]

@mountiny Ok sure - we can try onBlur @0xmiroslav can you please explain what should we do - as i understand you - we need to add logic for TextInput onBlur - when onBlur fires we need to check what exactly triggered to loose textinput focus, and if that was done by clicking on dark area we need to focus again? am I right?

[0xmiros]

@narefyev91 I think we can use similar approach from edit composer: https://github.com/Expensify/App/blob/3fdae586112aa4e33f6f4bed5f889bbf0ac50255/src/pages/home/report/ReportActionItemMessageEdit.js#L275-L286

[narefyev91]

@0xmiroslav ok - after some work around i extract this to onBlur

                        onBlur={(event) => {
                            const relatedTargetId = lodashGet(event, 'nativeEvent.relatedTarget.id');
                            if (!relatedTargetId) {
                                this.focusTextInput();
                            }
                        }}

Also i add 2 nativeIDs for Button in the bottom, and for currency select as well Based on this - if user clicks anywhere on the dark - event will not have relatedTarget.id - which will be indication that user clicks somewhere but not on currency selector/textinput itself/button in the bottom and we will move back focus to input Let me know if that works for us

[0xmiros]

That works for me. Minor concern is that if already blurred and click black area, input still keeps unfocused. Is that fine? @mountiny

[mountiny]

Great lest go with this proposal, @narefyev91 can you make a PR please?

In regards to the concern, I think we are fine, chatted 1:1 with @narefyev91 and basically this should be same behaviour as we have right now so not a big deal

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[MelvinBot]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.2.93-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/16673

If no regressions arise, payment will be issued on 2023-04-10. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• [x] External issue reporter
• [x] Contributor that fixed the issue
• [x] Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [x] [@0xmiroslav / @mountiny] The PR that introduced the bug has been identified. Link to the PR:
• [x] [@0xmiroslav / @mountiny] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [x] [@0xmiroslav / @mountiny] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [x] [@sonialiap] Determine if we should create a regression test for this bug.
• [x] [@0xmiroslav] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [x] [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: