[HOLD for payment 2022-05-20][$1000] Input is lost upon pressing ⌘K to open the Search View

[Luke9389]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! https://www.upwork.com/jobs/~01a9fa1b364d799b4c

---

If you start typing right after pressing ⌘K, some of your inputs might be lost as a result of the transition time for bringing up the search view. In a recent PR, @Santhosh-Sellavel attempted to shorten the animation time, but the problem persisted.

Action Performed:

1. Sign into New Dot with any account
2. Press ⌘K to open the search view and immediately start typing

Expected Result:

All of the text you type should be captured.

Actual Result:

Only the text that you type after the Search View has rendered gets captured.

Workaround:

Don't type so fast 😄

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number:
Reproducible in staging?:
Reproducible in production?:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @laurenreidexpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[parasharrajat]

I have posted a technique here #4822 (comment).

Let me know how does that sound?

[laurenreidexpensify]

https://www.upwork.com/jobs/~01a9fa1b364d799b4c

[Luke9389]

@parasharrajat would you be willing to make a formal proposal here for posterity?

[parasharrajat]

@Luke9389
I checked the suggested implementation, it works but it blocks the entering animation due to input focus while animating.

I am experimenting with another approach.

Update

OK, I have a fascinating idea of how can we do this without losing the animation. and It works.
I call it buffering the input.

1. We create an internal value buffer that stores all keypresses. It would be a string and we will append it to it.
2. When the search page is visible we consume that buffer. This will prevent input loss.

This how it can be done

// captures keys on keypress event.
function buffer(captureOnInputs = false) {
    if (keyBufferCallback) {
        return;
    }
    keyBufferCallback = (evt) => {
        if (!captureOnInputs
            && evt.target.nodeName === 'INPUT'
            && evt.target.nodeName === 'TEXTAREA'
            && evt.target.contentEditable === 'true') {
            return;
        }
        let chrCode = 0;
        if (evt.charCode != null) {
            chrCode = evt.charCode;
        } else if (evt.which != null) {
            chrCode = evt.which;
        } else if (evt.keyCode != null) {
            chrCode = evt.keyCode;
        }

        if (chrCode !== 0) {
            keyBuffer += String.fromCharCode(chrCode);
        }
    };

    document.addEventListener('keypress', keyBufferCallback, {capture: true});
}

// returns the value stored in bufffer and unsubscribe from keypress.
function consumeBuffer() {
    const value = keyBuffer;
    document.removeEventListener('keypress', keyBufferCallback, {capture: true});
    keyBuffer = '';
    keyBufferCallback = null;
    return value;
}

Now when we navigate to searchPage. we start the buffer via

navigateToSearchPage() {
        Navigation.navigate(ROUTES.SEARCH);
        KeyboardShortcut.buffer(false);
    }

and consume it in SearchPage

  <ScreenWrapper
                onTransitionEnd={() => {
                    const searchValue = KeyboardShortcut.consumeBuffer();
                    this.setState({didScreenTransitionEnd: true, searchValue}, this.updateOptions);
                }}
            >

[Luke9389]

This is a really cool idea. I'd be interested to see how many places in our code could use this type of behavior.

We call Navigation.navigate(ROUTES.SEARCH); from two places right now (and maybe we'll add more). Maybe it'd be better to start the buffer inside the navigate method. We could have a list of routes that require an input capture buffer, and then check for them inside of navigate.

Does this direction make sense for you? Essentially I'm wanting to implement this behavior in a manner that can be applied elsewhere. The assumption for me is "more generalizable" = "better". I'm going to loop in @marcaaron here to see what his thoughts are.

[marcaaron]

Don't type so fast 😄

I think I like this workaround better. The code feels a bit complex to me for solving an edge case. I've never had a problem with typing into this search box.

[parasharrajat]

Yup, this is an edge case but Matt is really finding this problematic. and that's why I invested time to find a solution.

---

I would say yes. We can add this into navigate behind a flag. but maybe it won't be pleasing to other devs on the team. It's your call we can do that for sure.

[rushatgabhane]

Proposal

Autofocus on TextInput breaks the animation because the browser scrolls to the TextInput!
Making the card's position: fixed solves this problem. However, mobile doesn't support position of type fixed. So we'll create a native file which uses the old implementation.

Modify this card style.

App/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js

Line 22 in 226cb00

const cardStyle = {};

const cardStyle = {
    position: 'fixed',
    width: isSmallScreenWidth? screen.width : variables.sideBarWidth,
    right: 0,
    height: '100%',
};

We'll conditionally render the search results based on didScreenTransitionEnd for New chat, and New group.

---

Verification here - https://jsfiddle.net/e01tjwvq/1/

This has 2 inputs (one is fixed, one is absolute)

Click run. Result --> Input with position: absolute is scrolled to.
But the input with position: fixed isn't scrolled to.

(Increase height of the viewport to verify that both inputs exist)

[mallenexpensify]

@Luke9389 can you review Rushat's proposal above please?

[Luke9389]

Thanks for the bump @mallenexpensify

Ok @parasharrajat, lets do this thing! Go ahead an make a PR.

[rushatgabhane]

omg, not again 😂

[rushatgabhane]

ETA for PR: 1 day.

[mallenexpensify]

It wasn't me this time @rushatgabhane !!!

[mallenexpensify]

@rushatgabhane is working on the PR

[rushatgabhane]

Actually, PR is awaiting a review

[mallenexpensify]

Looks like @Luke9389 needs to review the PR #8535 (comment)

[rushatgabhane]

PR is on staging

[Luke9389]

Go away melvin 😄

[mallenexpensify]

Been on staging 16 days which seems like a lot ¯_(ツ)_/¯h
#8535 (comment)

[mallenexpensify]

PR is on production, will pay 5/20 if no regressions, updated title

[mallenexpensify]

@rushatgabhane , you're due $1000 for this, right? If so, can you accept the offer I just sent?
https://www.upwork.com/jobs/~01d596bdb404440f06

[rushatgabhane]

@mallenexpensify yes, accepted!

[mallenexpensify]

Paid @rushatgabhane $1000 for fixing the issue.

[melvin-bot]

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.