search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.78k forks source link

[HOLD for payment 2023-10-02] [$500] Request money - No autofocus blink cursor shown in Request money page #26994

Closed izarutskaya closed 12 months ago

izarutskaya commented 1 year ago

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

Action Performed:

  1. Go to fab button -> request money ->Manual
  2. Notice that there is no blinking cursor beside the currency symbol . ( however if the request money page was previously set to scan or distance pages, such behaviour doesn't appear. Thus make sure after clicking request money, manual section is opened automatically)
  3. Close the RHN and again go to fab button -> split bill
  4. Notice that there appears blinking cursor beside currency symbol

Expected Result:

Consistency in pages in split money and request money ( Manual)

Actual Result:

No autofocus blink cursor shown in Request money page

Workaround:

Unknown

Platforms:

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

Version Number: v1.3.65-6

Reproducible in staging?: Y

Reproducible in production?: Y

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://github.com/Expensify/App/assets/115492554/8a75e992-eda4-474b-876d-8af7b483a95e

https://github.com/Expensify/App/assets/115492554/deed1b2f-5f83-48c7-8a6e-99d8ee4573ae

Expensify/Expensify Issue URL:

Issue reported by: @ashimsharma10

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693742251468319

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d0c4dd10133dafa2
  • Upwork Job ID: 1701221973754032128
  • Last Price Increase: 2023-09-11
  • Automatic offers:
    • fedirjh | Reviewer | 26670391
    • Krishna2323 | Contributor | 26670393
    • ashimsharma10 | Reporter | 26670394
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

Krishna2323 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue:

Request money - No autofocus blink cursor shown in Request money page

What is the root cause of that problem?

Safari's WebKit rendering engine interacts differently with the JavaScript event queue compared to other browsers. This divergence is particularly evident when programmatically focusing an input element. While InteractionManager.runAfterInteractions() is designed to execute actions after any active animations, its interaction with Safari results in differing behavior. Specifically, Safari may require direct user actions for certain tasks, including focusing on an input, leading to timing inconsistencies in script execution and due to this onBlur event is being triggered immediately after onFocus, and the focus is shifting to the body element, it indicates that there is some external factor causing the TextInput to lose its focus.

What changes do you think we should make in order to solve the problem?

To resolve this, we should modify the focusTextInput method by introducing an internal setTimeout function with a delay of 0ms. This ensures the focus command is repositioned to the end of the event queue, effectively navigating around the idiosyncrasies of Safari.

Reference Code: Current Implementation

Updated Code:

const focusTextInput = () => {
    // Component may not be initialized due to navigation transitions
    // Wait until interactions are complete before trying to focus
    InteractionManager.runAfterInteractions(() => {
        // Focus text input
        if (!textInput.current) {
            return;
        }

        textInput.current.focus();

        setTimeout(() => {
            if (!textInput.current) {
                return;
            }

            textInput.current.focus();
        }, 0);
    });
};

Result

https://github.com/Expensify/App/assets/85894871/40485c0a-b2ce-42af-a31d-df1af0e16025

Explanation

Using InteractionManager.runAfterInteractions() inside React Native (or in environments where it's supported) ensures that any work wrapped inside it gets executed after all animations and interactions have finished.

However, "finished" doesn't always mean "rendered to the screen," especially on some browsers or environments. There might be slight differences in how the browser's rendering engine processes updates to the DOM and how it queues JavaScript events.

When we're using InteractionManager.runAfterInteractions(), the intended behavior is to delay JavaScript execution until animations have completed, but this does not necessarily guarantee that the browser has completed its rendering process. In Safari's case, the rendering and JavaScript event queues might interact a bit differently than in other browsers.

By adding the setTimeout with a delay of 0ms, we're essentially placing the focus event at the end of the current JavaScript event queue. Even though the delay is 0ms, it doesn't execute immediately; instead, it waits for the current queue to be emptied. This slight delay, even though minuscule, gives Safari the time it needs to complete the rendering process, ensuring that the focus event works correctly.

To summarize, while InteractionManager.runAfterInteractions() ensures that JS execution is deferred until animations are done, it doesn't account for potential idiosyncrasies in browser rendering. The nested setTimeout, even with a 0ms delay, provides that extra assurance that the focus will be executed after the rendering process has completed in the Safari browser.

melvin-bot[bot] commented 1 year ago

@NicMendonca Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 year ago

@NicMendonca Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh (External)

tienifr commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue:

Request money - No autofocus blink cursor shown in Request money page

What is the root cause of that problem?

In https://github.com/Expensify/App/blob/5a153a348b0dede15d9992873477d97bc2a01f24/src/pages/iou/MoneyRequestSelectorPage.js#L73

we're already using ScreenWrapper, and we can get this.props.navigation in ScreenWrapper.

In https://github.com/Expensify/App/blob/5a153a348b0dede15d9992873477d97bc2a01f24/src/pages/iou/steps/NewRequestAmountPage.js#L175

we're still using ScreenWrapper, but there's no such thing as the this.props.navigation, that why onEntryTransitionEnd function is never called

=> focusTextInput is not called when the transition is ended

What changes do you think we should make in order to solve the problem?

In MoneyRequestSelectorPage we create new state transitionEnded and set it to true in onEntryTransitionEnd function 

        <ScreenWrapper
       ...
            onEntryTransitionEnd={()=>{
                setTransitionEnded(true)
            }}
        >

and pass transitionEnded to NewRequestAmountPage

In NewRequestAmountPage we focus the input when transitionEnded is true

    useEffect(()=>{
        if(transitionEnded){
            focusTextInput()
        }
    },[transitionEnded])

Result

https://github.com/Expensify/App/assets/113963320/76779cee-03b4-4fa5-806f-455bb632e2a1

melvin-bot[bot] commented 1 year ago

@NicMendonca, @fedirjh Eep! 4 days overdue now. Issues have feelings too...

fedirjh commented 1 year ago

it indicates that there is some external factor causing the TextInput to lose its focus.

@Krishna2323 What's the external factor you referring to? why does the split bill page focus the input correctly while the request money page doesn’t?

@tienifr The root cause is not clear to me can you elaborate more?

=> focusTextInput is not called when the transition is ended

I don't think this is correct. The input appears to be initially focused correctly, but it loses focus instantly. Please check the video below. I added a console log inside the focus function, and it seems that the focus works as expected. However, it promptly loses focus, which is clearly noticeable.

https://github.com/Expensify/App/assets/36869046/30179a69-8110-4c26-b0ee-6fdd83c8e03b

tienifr commented 1 year ago

@fedirjh As we can see there're 2 places that call focusTextInput, focusTextInput is called when we open the page (useFocusEffect), but it is not called when the transition is ended (onEntryTransitionEnd)

tienifr commented 1 year ago

After the input is focused (in useFocusEffect), there're some transitions of navigation (this causes the input blur), that why we need to wait for them end before focusing on the input

fedirjh commented 1 year ago

why does the split bill page focus the input correctly? It has the same transition of navigation.

tienifr commented 1 year ago

Ah, because in Request money page, we use TopTab, and it requires some extra transitions.

tienifr commented 1 year ago

run focusTextInput in useFocusEffect can not make sure it runs after the transition is end.

Krishna2323 commented 1 year ago

Proposal (Updated)

Please re-state the problem that we are trying to solve in this issue.

Request money - No autofocus blink cursor shown in Request money page.

What is the root cause of that problem?

Upon investigation, the root cause appears to be the default behavior of the TopTab.Navigator. Without explicit configuration, the navigator might interfere with the keyboard's display settings, leading to its unintended dismissal or erratic behavior, especially when switching tabs or interacting with input fields.

What changes do you think we should make in order to solve the problem?

To counteract this issue, we should modify our TopTab.Navigator configuration by setting the keyboardDismissMode property to "none". This would ensure that the keyboard remains consistent in its behavior, regardless of interactions within the navigator.

By applying this change, we will override the default behavior and prevent the keyboard from dismissing due to tab interactions.

We just need to pass keyboardDismissMode={"none"} here.

https://github.com/Expensify/App/blob/d68263ad664d8a8a81ca859147696df8abfcd480/src/libs/Navigation/OnyxTabNavigator.js#L30-L47

Result

https://github.com/Expensify/App/assets/85894871/383547c2-88d7-4e19-af12-6cef4de2ff0f

Krishna2323 commented 1 year ago

@fedirjh, pls review updated proposal.

fedirjh commented 1 year ago

@Krishna2323 That makes sense to me. I believe it will provide us with greater flexibility in managing the keyboard.

This proposal from @Krishna2323 looks good to me.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @MariaHCD, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

MariaHCD commented 1 year ago

Can we recheck if this is still an issue? I wasn't able to see the issue on dev, staging or production:

https://github.com/Expensify/App/assets/12268372/ed478d65-30b0-46d3-9503-c9b8f23cbfe9

Krishna2323 commented 1 year ago

@MariaHCD, this only happens in safari.

MariaHCD commented 1 year ago

Ah, thanks, @Krishna2323. I totally missed that!

So will your proposal set the keyboardDismissMode TopTab.Navigator configuration across the whole app? Does this affect mobile in any way?

Krishna2323 commented 1 year ago

@MariaHCD, we are also facing this issue in android and I think this will also fix this issue.

MariaHCD commented 1 year ago

Okay, thanks for clarifying. Could you update your proposal with the exact code changes you're proposing?

Krishna2323 commented 1 year ago

@MariaHCD, sorry for the confusion, keyboardDismissMode will not have any effect on mobile but this will not fix other issue. Updated my proposal with code changes.

MariaHCD commented 1 year ago

Thanks, the proposal makes sense to me. We'll just have to ensure it does not cause any unexpected side effects.

melvin-bot[bot] commented 1 year ago

📣 @fedirjh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 year ago

📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer 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 📖

melvin-bot[bot] commented 1 year ago

📣 @ashimsharma10 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

Krishna2323 commented 1 year ago

@MariaHCD @fedirjh, just before making the PR I want to clarify the current bugs in native devices for autofocus so we don't take them as regression from the PR.

You can see here everything works same before and after applying changes, just on thing is changed in android, the input is not blurred after applying keyboardDismissMode but the cursor doesn't blink. So I think for now we should only do that for Safari until these issue are fixed.

https://github.com/Expensify/App/assets/85894871/78d2e426-3529-407b-a6ba-fb6e78cfcf10

https://github.com/Expensify/App/assets/85894871/682668b5-f653-43b3-a991-baa939a07c91

Krishna2323 commented 1 year ago

@fedirjh, PR is ready for review.

melvin-bot[bot] commented 1 year ago

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

On to the next one 🚀

Krishna2323 commented 1 year ago

@MariaHCD, considering we also addressed two additional issues and successfully resolved one of them, would this qualify for a bonus?

MariaHCD commented 1 year ago

@Krishna2323 I believe the additional issue identified had the same root cause so I am not sure this would be eligible for a bonus. We normally pay out bonuses if the issue/solution becomes more complicated/challenging and the contributor has to invest more time/effort into the issue.

However, I will defer to @NicMendonca

@Krishna2323 could you can explain why you feel your work on this issue deserves a bonus?

Krishna2323 commented 1 year ago

@MariaHCD , the PR was ready to be merged quite early. However, we tested this issue afterward, which added an additional time.

I'm inquiring about the urgency bonus and kindly asking if we're eligible.

MariaHCD commented 1 year ago

Hmm, so we tried to fix the same issue on iOS native but that led to a bit of a delay in getting the PR merged. I think it's all part of the review process and you did handle addressing the reviews with urgency so I think paying out the bonus in this case is fine. Regardless, I'll leave the final call to @NicMendonca

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

If no regressions arise, payment will be issued on 2023-10-02. :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.

For reference, here are some details about the assignees on this issue:

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

melvin-bot[bot] commented 1 year ago

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:

NicMendonca commented 12 months ago

@fedirjh bump on the BZ issue ^^

NicMendonca commented 12 months ago

Reporter: @ashimsharma10 - $50 Contributor: @Krishna2323 - $750 C+ @fedirjh - $750

NicMendonca commented 12 months ago

@Krishna2323 @ashimsharma10 - you've both been paid 🎉

@fedirjh can you please accept the offer in Upwork?

Krishna2323 commented 12 months ago

@NicMendonca Thank you very much for the bonus, I appreciate it.

NicMendonca commented 12 months ago

@fedirjh bump bump on BZ checklist

fedirjh commented 12 months ago

BugZero Checklist:

Regression Test Proposal

  1. Go to fab button -> Request money
  2. Switch to the manual tab

  3. Verify that the amount input is focused and the cursor is blinking.

    • Do we agree 👍 or 👎
NicMendonca commented 12 months ago

@fedirjh thanks! You've been paid via Upwork🎉

All set here ✅