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.03k stars 2.54k forks source link

[$4000] Android - Edit message - keyboard is dismissed after selecting an emoji @Tushu17 #9252

Closed kbecciv closed 7 months ago

kbecciv commented 2 years 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 steps:

  1. Send a message in any chat in App
  2. Long press the message and tap on Edit message
  3. Tap on the emoji picker
  4. Select an emoji and observe the keyboard is dismissed

Expected results:

The alphabetical keyboard should not be dismissed after selecting an emoji

Actual Result:

Keyboard doesn't open up after selecting emoji.

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.1.69.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

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

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/171174709-69b79478-f54a-4c55-b84b-114f7ec51087.mp4

Upwork URL: https://www.upwork.com/jobs/~01f30fad98ca9b6c69

Issue reported by: @Tushu17

Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1650039236885699

View all open jobs on GitHub

Upwork Automation - Do Not Edit

aneequeahmad commented 1 year ago

@parasharrajat Actually the next time i test again code base has changed like previously pressing the emojiPicker wasn't doing any action and now in latest build it is opening the emojiPicker but setTimeout was to be added as this is a known problem discussed here.

I'll take care next time keeping in mind what you said. apologies and thanks for your valuable feedback.

melvin-bot[bot] commented 1 year ago

@Julesssss, @trjExpensify, @parasharrajat Eep! 4 days overdue now. Issues have feelings too...

parasharrajat commented 1 year ago

No update.

Julesssss commented 1 year ago

@trjExpensify could we please double, given that we're still seeking proposals.

trjExpensify commented 1 year ago

👋 back from OoO, doubled here.

mollfpr commented 1 year ago

Proposal

There are couples of root cause explained by other's and the issue only happen on Android. Also, we encountered the same issue on ReportActionCompose and fix with adding a timeout before focusing the input and that's not acceptable in this issue.

So I try replacing InteractionManager.runAfterInteractions when onModalHide is called with requestAnimationFrame. Turns out this requestAnimationFrame is called after InteractionManager.runAfterInteractions and makes the keyboard appear when we hide the modal. I think it's because requestAnimationFrame will fire after all the frames have been flushed.

I also try the changes on ReportActionCompose and works as expected. So probably we can eliminate the setTimeout implementation on focusing input.

https://github.com/Expensify/App/blob/22e39f9de1c1fbace6e7a0e4c06f364310129895/src/pages/home/report/ReportActionItemMessageEdit.js#L223

-  onModalHide={() => InteractionManager.runAfterInteractions(() => this.textInput.focus())}
+  onModalHide={() => requestAnimationFrame(() => this.textInput.focus())}

Result

Android

https://user-images.githubusercontent.com/25520267/185793028-3e3b5ec8-73d2-47c4-8e99-8eb4054209d2.mp4

iOS

https://user-images.githubusercontent.com/25520267/185793127-4feb627d-0555-4df0-b5de-f2b1d420d3c3.mov

mWeb

https://user-images.githubusercontent.com/25520267/185793319-4bacce53-07b8-4861-b7bf-badb9231ceef.mp4

parasharrajat commented 1 year ago

Thank you all for your proposals. As I can see everyone's solution is somehow based on SetTimeout. I do not consider requestAnimationFrame very different from setTimeout. InteractionManager should be enough to detect the running animations that put a heavy load on the app. requestAnimationFrame will just wait for the next frame and its timing is not deterministic. It could be delayed if the app is going through a heavy load(FPS drops). Similarly setTimeout can also be delayed but almost guaranteed to be fired at the provided time. Overall requestAnimationFrame should be faster than setTimeout,100.

Now the main part...

The only reason to keep this issue open is to find the real issue, which is Why even after calling textinputRef.focus() does not open the soft keyboard? textinputRef.focus() is tightly coupled with the Native layer but still it fails to open the soft keyboard.

I have seen no research so far in the proposal that tries to solve the mystery.

trjExpensify commented 1 year ago

Not overdue, awaiting proposals.

melvin-bot[bot] commented 1 year ago

@Julesssss, @trjExpensify, @parasharrajat Whoops! This issue is 2 days overdue. Let's get this updated quick!

Julesssss commented 1 year ago

Not overdue, we're still awaiting proposals here.

Julesssss commented 1 year ago

Still awaiting proposals here.

melvin-bot[bot] commented 1 year ago

⚠️ 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.

melvin-bot[bot] commented 1 year ago

⚠️ 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.

melvin-bot[bot] commented 1 year ago

@Julesssss, @trjExpensify, @parasharrajat Whoops! This issue is 2 days overdue. Let's get this updated quick!

trjExpensify commented 1 year ago

Still awaiting proposals here?

Julesssss commented 1 year ago

Yep

Julesssss commented 1 year ago

@trjExpensify please hold on any further pay increases for the moment though.

mallenexpensify commented 1 year ago

Per this internal conversation, putting this keyboard-related issue on hold pending https://github.com/Expensify/App/issues/10273

I updated title and removed Help Wanted

melvin-bot[bot] commented 1 year ago

@Julesssss, @trjExpensify, @parasharrajat Whoops! This issue is 2 days overdue. Let's get this updated quick!

Julesssss commented 1 year ago

No further proposals received.

mallenexpensify commented 1 year ago

It's on HOLD for now @Julesssss, pending https://github.com/Expensify/App/issues/10273 Not sure why this was a Daily, bumped to Weekly for now

trjExpensify commented 1 year ago

Can we put this on Monthly then and mark it as Internal if we're scoping a solution internally?

melvin-bot[bot] commented 1 year ago

⚠️ 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.

melvin-bot[bot] commented 1 year ago

⚠️ 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.

Julesssss commented 1 year ago

Still on hold, see this message.

Julesssss commented 1 year ago

No change from the last update.

trjExpensify commented 1 year ago

👋 Isn't #10273 largely complete now, why is this still held? Could it be even fixed on Android, perhaps?

CC: @JmillsExpensify, I don't think this one is on your radar as the BZ on the KeyboardAvoidingView initiative.

Julesssss commented 1 year ago

Just confirmed this is still occurring for me on Android 13.3.

Julesssss commented 1 year ago

Given that the Keyboard changes now seem unlikely to resolve this, let's open it up for solutions again

trjExpensify commented 1 year ago

Yeah, I agree with that. 👍

JmillsExpensify commented 1 year ago

Thanks @trjExpensify Feel free to keep it, though I'll subscribe and participate.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~018e351e7d34632075

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

trjExpensify commented 1 year ago

Cool, thanks for confirming. Added the External label afresh!

Tushu17 commented 1 year ago

proposal

We can create a function to use setTimeOut in ReportActionItemMessageEdit file.

focus(shouldelay = false) {
        InteractionManager.runAfterInteractions(() => {
            if (!this.textInput) {
                return;
            }

            if (!shouldelay) {
                this.textInput.focus();
            } else {
                setTimeout(() => this.textInput.focus(), 100);
            }
        });
    }

we can pass that function to onModalHide prop of EmojiPickerButton as true. https://github.com/Expensify/App/blob/98354545cc89b75a53f07cb1d694f7ab78ffbb40/src/pages/home/report/ReportActionItemMessageEdit.js#L252 onModalHide={() => this.focus(true)}.

parasharrajat commented 1 year ago

@Julesssss Are we expecting a setTimeout-based solution as a fix to this issue? Just to clarify because we reopened this one.

If not, https://github.com/Expensify/App/issues/9252#issuecomment-1223036462.

@Tushu17 being an experienced contributor, we expect our process to be followed. And the process says that a proposal should not match existing ones and contributors should go over the whole issue discussion before proposing a solution. As the proposal matches existing ones, it is rejected.

Julesssss commented 1 year ago

Are we expecting a setTimeout-based solution as a fix to this issue?

Only if it can't be avoided.

trjExpensify commented 1 year ago

Thread we're discussing next steps on this one is here for the breadcrumbs!

JmillsExpensify commented 1 year ago

Floated with Margelo so removed the External and Help Wanted labels for now.

fedirjh commented 1 year ago

@parasharrajat this is a known issue from RN from a long time https://github.com/facebook/react-native/issues/19366 , it was mentioned in their docs as well Known issues

This may explain the mystery :

We can see then that a ShadowNode exists with a view, and a parent shadow node, but the view doesn't have a parent. This means for the we try to focus in React has had a corresponding native UIElement created, but the element hasn't been connected to the app's visual tree.

So we cannot focus the component, because the UIElement hasn't been attached to the window's view tree yet.

This thread https://github.com/microsoft/react-native-windows/issues/9292 can answer the mystery behind this issue , and it provide a solution https://github.com/microsoft/react-native-windows/issues/9292#issuecomment-1038218769

I think the queue-based solution is a good one. If a focus request comes in, on something not yet connected, we buffer subsequent UIManager focus/blur requests until the next is available. That would preserve app-side focus state correctly, as well.

@Julesssss it Looks like setTimeout-based solution can't be avoided

hannojg commented 1 year ago

I can imagine that my PR https://github.com/Expensify/App/pull/11684 fixes this issue. In my testing the keyboard always reopens after selecting an emoji. Maybe someone can take the time to try to reproduce the issue on the branch? 😊

Julesssss commented 1 year ago

Cool, thanks all. We'll hold this one briefly until the linked PR is merged. We also have the option of the setTimeout solution as a workaround to revisit if that doesn't work.

trjExpensify commented 1 year ago

Well, that would be marvellous! Fortuitously @parasharrajat, you're the C+ on it as well! 🎉

@Julesssss I'm updating to include the hold reason in the title, let's always remember to do that for ease of following.

parasharrajat commented 1 year ago

https://github.com/Expensify/App/issues/9252#issuecomment-1344145606 Exactly, I said on Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1670538621954439?thread_ts=1670271699.383879&cid=C01GTK53T8Q.

trjExpensify commented 1 year ago

Still on hold for that PR. @parasharrajat are you back on that to test this week?

JmillsExpensify commented 1 year ago

@parasharrajat Are you still blocked from testing?

parasharrajat commented 1 year ago

No completely so I will retest the latest changes.

trjExpensify commented 1 year ago

Latest from the linked PR this is held on is that it will be tested today.