Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keyboard doesn't work for edit comment - reported by @rushatgabhane #7482

Closed
mvtglobally opened this issue Jan 31, 2022 · 16 comments
Closed
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@mvtglobally
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Tap and hold to edit any comment
  2. Scroll down
  3. Tap on edit box

Expected Result:

Edit box scrolls down and keyboard works

Actual Result:

Focus is lost to the edit box and typing on the keyboard doesn't do anything

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • Android

Version Number: 1.1.32-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

screen-20220125-083408_2.mp4

Expensify/Expensify Issue URL:
Issue reported by: @rushatgabhane
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1643089355394200

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Jan 31, 2022
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added Overdue and removed AutoAssignerTriage Auto assign issues for triage to an available triage team member labels Jan 31, 2022
@MelvinBot
Copy link

Triggered auto assignment to @flodnv (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@muttmuure muttmuure removed their assignment Feb 2, 2022
@MelvinBot MelvinBot removed the Overdue label Feb 2, 2022
@flodnv flodnv removed their assignment Feb 3, 2022
@flodnv flodnv added the External Added to denote the issue can be worked on by a contributor label Feb 3, 2022
@MelvinBot
Copy link

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

@NicMendonca
Copy link
Contributor

@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 3, 2022
@MelvinBot
Copy link

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

@eVoloshchak
Copy link
Contributor

I was able to reproduce this issue

22-02-04-11-01-53.mp4

I'm guessing the root cause is that we are calling focus() for TextInput right after it first mounted.
There is a couple of issues describing this problem, this comment expains it:

RN 0.65 (or 0.64?) changed something in the native dispatching order that ends up delaying certain changes. For example, changing a prop and immediately running ref.someMethod (on the setState callback), on Android, will result in the prop not being ready / updated yet, requiring an extra setTimeout call. The same cannot be reproduced on iOS.
For the input focus issue, the behaviour is similar. Do a state change that renders the input (e.g., conditionally rendering it), and immediately call ref.focus(). This will result in the focus not firing because the input is technically not ready on the setState callback, which causes the issue (and can be fixed with a timeout).

We are using newer version of RN, but i was not able to find any newer issues about this in RN repository.

Proposal
We can fix this by changing this line to the following:
Platform.OS === 'android' ? setTimeout(() => this.textInput.focus(), 100) : this.textInput.focus();
Result would look like this:

22-02-04-11-52-26.mp4

This, of course, isn't a fix, but a workaround.
react-navigation has something similar in it's useKeyboardManager.tsx

// Too fast input refocus will result only in keyboard flashing on screen and hiding right away.
// During first ~100ms keyboard will be dismissed no matter what,
// so we have to make sure it won't interrupt input refocus logic.
// That's why when the interaction is shorter than 100ms we add delay so it won't hide once again.
// Subtracting timestamps makes us sure the delay is executed only when needed.

@rushatgabhane
Copy link
Member

Thanks for the detailed investigation @eVoloshchak.

Changing a prop and immediately running ref.someMethod (on the setState callback), on Android, will result in the prop not being ready / updated yet
Do a state change that renders the input (e.g., conditionally rendering it), and immediately call ref.focus(). This will result in the focus not firing because the input is technically not ready on the setState callback, which causes the issue

That seems true and should help.

But take a look at this - I've added the 100ms workaround and there are no state/props being changed.
I think the animation is causing the trouble here.

screen-20220204-212159.mp4

@eVoloshchak
Copy link
Contributor

But take a look at this - I've added the 100ms workaround and there are no state/props being changed. I think the animation is causing the trouble here.

Yup, that clearly doesn't work.
But i cannot reproduce it on my phone

  1. It is unclear what happens on video, am I correct in thinking that on the third secong keyboad disappears by itself? Or did you press/scroll on some element on the screen?

I think the animation is causing the trouble here.

Yes, that's probably it, considering that TextInput has fixes for it throughout the project

  1. Could you please check, if this makes any difference
    InteractionManager.runAfterInteractions(() => {
        if (!this.textInput) {
            return;
        }
        Platform.OS === 'android' ? setTimeout(() => this.textInput.focus(), CONST.ANIMATED_TRANSITION) : this.textInput.focus();
    });

insead of this line

@rushatgabhane
Copy link
Member

@eVoloshchak there are 3 cases in the video

  1. Edit report scrolls down 100%, keyboard opens up, and edit report is focused.
  2. Edit report scrolls down 50%, keyboard opens up, but edit report loses focus so typing on keyboard does nothing.
  3. Edit report scrolls down 50%, keyboard flashes and edit report focus is lost.

I hope this makes sense.

InteractionManager.runAfterInteractions(())

This doesn't help.

@eVoloshchak
Copy link
Contributor

@eVoloshchak there are 3 cases in the video

  1. Edit report scrolls down 100%, keyboard opens up, and edit report is focused.
  2. Edit report scrolls down 50%, keyboard opens up, but edit report loses focus so typing on keyboard does nothing.
  3. Edit report scrolls down 50%, keyboard flashes and edit report focus is lost.

I hope this makes sense.

InteractionManager.runAfterInteractions(())

This doesn't help.

Ok, thanks for clarification
I'll try to investigate this a bit more on monday

@marcaaron
Copy link
Contributor

@rushatgabhane What's the status with this one?

@MelvinBot MelvinBot removed the Overdue label Feb 14, 2022
@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 15, 2022

@marcaaron I'm waiting for proposals. No proposal so far fixes the issue.
If I'm not wrong, we just need to wait for the edit box to finish it's animation before opening the keyboard.

With that said, @nickmurray47 we could double this issue to get more 👀️

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 15, 2022

I'll try to investigate this a bit more on monday

@eVoloshchak let us know if you found anything that could be useful, thanks!

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 17, 2022

@marcaaron I came across this same issue which was created in Aug #4345

The consensus was to close the issue. Because

Keyboard is purposely disabled here and I am pretty sure we would want to keep that behavior.
Imagine, you press edit on a message and the then message will scroll to the bottom and in the meantime, the whole layout will shift up due to the keyboard. How bizarre will that look?

Yes, I was also under the impression that mobile giants (Apple and Google) greatly discouraged giving things auto-focus because it creates a worse UX in most cases.

Let's close this issue if it's okay by you?

@marcaaron
Copy link
Contributor

Sounds good. Let's close it based on the comments here. Agree it is the same issue and clear based on that what the default behavior should be.

@marcaaron marcaaron removed their assignment Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants