[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 https://github.com/Expensify/App/issues/4822#issuecomment-907094752.

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.

[marcaaron]

We can add this into navigate behind a flag

If we want to start using a key logger to capture some keys before an input is focused then I'd still say we should not couple it with the navigate() method and only use the buffer when necessary. Otherwise, we are adding optional logic to something that 99% of the time isn't going to need it.

Still, something about this seems off to me. Mostly, I can't wrap my head around why would anyone would expect to be able to enter text into an input that is not yet on the screen. And moreover why it would be burdensome for them to wait for that input.

[parasharrajat]

Otherwise, we are adding optional logic to something that 99% of the time isn't going to need it.

Yup, it is always good to separate the responsibilities of functions. navigate should only navigate. that's why a wrapper like navigateToSearchPage and could be more generic is better.

[Luke9389]

Yea that's a good point about keeping it out of navigate. Separation of concerns.

Still, something about this seems off to me. Mostly, I can't wrap my head around why would anyone would expect to be able to enter text into an input that is not yet on the screen. And moreover why it would be burdensome for them to wait for that input.

I think it's a matter of users repeatedly going through the same flow and anticipating what to do next (and then jumping the gun). The burden seems to be accidentally hitting a key too soon, so then your search results are based on the wrong letter and you've gotta backspace it up and try again.

To me, it seems the problem is here. So far @iwiznia and @mallenexpensify have both independently reported this.

[mallenexpensify]

I was just able to command+k and type nik before ki showed up in the box. If someone is able to type at least 3 characters, I don't _think_ it's an edge case. Honestly.. I think I've unknowingly trained myself to pause before typing since I 'command+k 100(?) times a day.

Let's fix it if we reasonably can. If y'all aren't sold on it being important, let's drop a poll into #expensify-open-source to see if others experience the issue

[mallenexpensify]

We are placing all non-critical issues on hold while we're on a company offsite. The hold is expected to be lifted on 11/19 (but could go longer). For any PRs that are submitted before or during the hold, we will add a $250 bonus payment.

[parasharrajat]

I am not sure about the next steps here @Luke9389 what are we planning to do here?

[Luke9389]

Let's close for now and wait to see if users comment on this.

[parasharrajat]

Ok. Sad to see another waste of effort here.

[Luke9389]

@parasharrajat I think this one will end up coming back to us. Once we have lots of users, I think this will come up regularly for some folks.

[mallenexpensify]

Let's close for now and wait to see if users comment on this.

This happens EVERY DAY for me. IT's a 'known issue' so it'd be nice to fix it. @Luke9389 , what's the reason for closing? Cuz we don't think it's important or because the fix is difficult?

[Luke9389]

Oh, I misunderstood you @mallenexpensify. I thought by saying "I think I've unknowingly trained myself to pause" you were saying it's not a big deal. I'm fine with having this fix go through. I like the buffer solution that @parasharrajat presented.

[parasharrajat]

Please assign this issue to me....

[mallenexpensify]

Thanks Luke and Rajat. Rajat, can you apply here? https://www.upwork.com/jobs/~01a9fa1b364d799b4c

[laurenreidexpensify]

Sorry Rajat, missed this - you're hired in Upwork now

[Luke9389]

Ok, we decided not to proceed with @parasharrajat's PR above.

We want to refocus on understanding why autofocus breaks animation so you can't focus input until the animation is done. Any future proposals for this issue will need to address that rather than provide a workaround.

[laurenreidexpensify]

Paid and closed in Upwork 👍🏽

[parasharrajat]

@laurenreidexpensify I don't think we meant to close this yet. The focus of this issue is moved to a different context now. @Luke9389 Please correct me if I am wrong.

Why can't we focus on the input during animation? What causes the animations to break? and How can we fix it?

[laurenreidexpensify]

Ah sorry my bad! Read too quickly 😅

[laurenreidexpensify]

Realised we lost the help wanted label in the shuffle I'm gonna also make this $500 now

[parasharrajat]

I think I was left assigned to this so unassigning....

[parasharrajat]

OK. I was assigned early to this issue but we together followed an approach that ended up hacky.

Now, I would like to work over it again. If the plan is to either reduce the animation time or remove the animation from the Search page. Discussion is happening here

Proposal after the discussion is concluded.

[MelvinBot]

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[laurenreidexpensify]

@MitchExpensify i'm OOO quite a bit over next week moving house, so handing this one off while I"m away thanks!

[K4tsuki]

Proposal: In https://github.com/Expensify/App/blob/de5d2ba79fa86fa3510009ed618b88e0b1f4c5fa/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js#L22 I think setting cardStyle position to 'fixed' could fix the animation glitch on early TextInput focus. May I try to take this issue?

https://user-images.githubusercontent.com/1313124/145489775-b132ca11-0728-4569-b83b-a5010f61360a.mp4

I am slowing the animation to confirm that the glitch doesn't appear.

[MelvinBot]

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

[Luke9389]

@K4tsuki, thanks for your proposal. Before I can give it a proper review, I need to refamiliarize with the history of the issue and also do some research on how position 'fixed' will affect it. I'll be back once I know a bit more

[MitchExpensify]

@Luke9389 is researching before making a call on the proposal

[Luke9389]

We are going to open an issue in the reanimated repo where we believe the issue is occurring. Slack context here.

[K4tsuki]

@Luke9389 But I am trying to fix this issue here, meanwhile they are fixing so many issues. We don't know if this is really their issue, could you point out the exact issue?
I propose a fix which is currently available. This could be benefits for other screens too, like IOUmodal, the modal doesn't need spinning loader anymore. This is the first time I am submitting in Expensify, if you want more elaborate explanation I will give a proper explanation in my proposal. Have you tried my code and tested it? Have any other part of the App broken because of my code? I am sorry if I am too tense in this, but I am researching this issue and found the solution with a hardship. Thank you.

@marcaaron could you review my proposal?

[Luke9389]

Hey @K4tsuki, I understand that you've invested some time in this. I'll test your code and see if it resolves the issue. Thanks

[K4tsuki]

But what will you do? just input position: fixed? it needs another positional styling to properly show the modal.

[Luke9389]

Oh, I thought you were recommending that I set cardStyle to position: fixed. Do you have other code changes to test out with me?

[K4tsuki]

Here is the code for:

modalCardStyleInterpolator.js

``` import {Animated} from 'react-native'; import variables from '../../../styles/variables'; export default ( isSmallScreenWidth, isFullScreenModal, { current: {progress}, inverted, layouts: { screen, }, }, ) => { const translateX = Animated.multiply(progress.interpolate({ inputRange: [0, 1], outputRange: [isSmallScreenWidth ? screen.width : variables.sideBarWidth, 0], extrapolate: 'clamp', }), inverted); const cardStyle = { position: 'fixed', width: isSmallScreenWidth? screen.width : variables.sideBarWidth, right: 0, // height: '100%' }; const opacity = Animated.multiply(progress, inverted); //const cardStyle = {}; if (isFullScreenModal && !isSmallScreenWidth) { cardStyle.opacity = opacity; } else { cardStyle.transform = [{translateX}]; } return ({ containerStyle: { overflow: 'hidden', //position: 'absolute', //top: '0px', //left: '0px', width: '100%', height: '100%' }, cardStyle, overlayStyle: { opacity: progress.interpolate({ inputRange: [0, 1], outputRange: [0, 0.3], extrapolate: 'clamp', }), }, }); }; ```

And Change didScreenTransitionEnd to true just to try it. https://github.com/Expensify/App/blob/2b7d6d0a4171319a9328dca14bcd5023a5352a94/src/pages/SearchPage.js#L170-L171

I am using old file on modalCardStyleInterpolator.js. If you have an issue, just change this cardStyle:

https://github.com/Expensify/App/blob/de5d2ba79fa86fa3510009ed618b88e0b1f4c5fa/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js#L22

with:

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

and containerStyle in: https://github.com/Expensify/App/blob/de5d2ba79fa86fa3510009ed618b88e0b1f4c5fa/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js#L31 to:

containerStyle: {
    overflow: 'hidden',
    width: '100%',
    height: '100%'
} 

It is not final code. just to try it.

[K4tsuki]

We want to refocus on understanding why autofocus breaks animation so you can't focus input until the animation is done. Any future proposals for this issue will need to address that rather than provide a workaround.

Autofocus breaks the animation because of hidden part of text input while animating the search page. Browser will try to show whole text input when it is on focus by moving something to display the text input if it is using or child of absolute and relative element positioning. But if the text input is child of fixed position element, browser will display as it is, even is whole text input is not displayed in the screen. Browser will not try to scroll or display that text input.

Here is the video: Because search result is heavy element, to smooth out the animation we can just have to show the search bar first to catch the input value and show the search result after transition end.

Regular speed:

https://user-images.githubusercontent.com/1313124/147195294-412f0599-9e59-4e87-88a9-9c011f9ff08e.mp4

Slow speed:

https://user-images.githubusercontent.com/1313124/147195299-40393ec4-6da9-4754-91ec-3628e274f6df.mp4

[Luke9389]

Looping back around to this soon

[Luke9389]

I think this should be closed. There just hasn't been a consensus that this is a problem. Thoughts @MitchExpensify ?

[mallenexpensify]

It's 100% a problem, when I type cmd+k and start typing a name, it doesn't show what I've typed. It often takes a character or three. This is me typing james

And this is isabella

It's a bug and we should fix it. Is there a reason we haven't been doubling the price til someone proposes an acceptable solution? If not, @MitchExpensify should double pronto

[MitchExpensify]

This actually works reasonably well for me. Unless I really try to type super fast after CMD+K it works fine..

[mallenexpensify]

I'm not sure we want a product that only works reasonably well :trollface: I posted in #expensify-open-source to see what others think. I'm a keyboard shortcut fanatic so I might not be the best https://expensify.slack.com/archives/C01GTK53T8Q/p1645749626797729

[kidroca]

I'm not sure we want a product that only works reasonably well :trollface:

Agree - As a user I don't want to be training myself to type slower (after Cmd+K) so that I can use the product. I see this as one of these tiny imperfections that you wouldn't notice and appreciate for being fixed, but would cause frustration when they happen Left some notes in the slack conversation shared above

[mallenexpensify]

@K4tsuki also commented

I think the function of the shortcut is to increase productivity. But if user must wait until search page displayed to search for a user then it is counter productive/ hinder productivity.

Let's get this fixed, one way or another. @kidroca based on the vid you posted in thread, it looks like you have a solution, do you want to propose that here?

https://user-images.githubusercontent.com/22508304/155757400-312becdc-f391-4112-b393-f8983dcca34f.mp4

I created a new job and doubled price to $1000 to get 'er fixed https://www.upwork.com/jobs/~01ffc80a2016b3d9b4

[kidroca]

@mallenexpensify As you can see in the video above. The "fix" on the right has no drawer transition - RHN does not swipe in I failed to notice until it was brought up in slack (Closing search still has an animated transition)

It seems the current issue is stuck (for +6 months now) because we're looking for a fix that both preserves the animation and fixes the lost input problem

Right now animation works but (some) input is lost. I would prefer to have animation partially skipped but all input is captured, than to keep waiting for a fix, or apply a fix that is too complex for the problem

If you study other app with a search shortcut, pressing Cmd+F instantly snaps you to a search field: slack, whatsapp, browsers, everything.

My proposal is to render the search field before the transition ends so that input can be captured immediately We can still do one of the following:

• show loading below the search input until initial list is ready
• show a semi transparent loader like in the video above

Diff sample

```jsx diff --git a/src/pages/SearchPage.js b/src/pages/SearchPage.js --- a/src/pages/SearchPage.js (revision 1d3536ae49729b1d340986f0d894fc2ee1ba2d5a) +++ b/src/pages/SearchPage.js (date 1645812439373) @@ -174,18 +174,16 @@ /> - {didScreenTransitionEnd && ( - - )} + ```