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.4k stars 2.79k forks source link

[HOLD for payment 2024-08-29] [$250] Voice recognition doesn't work on mobile web on Android in the message compose input #44394

Closed m-natarajan closed 2 weeks ago

m-natarajan commented 3 months 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!


Version Number: 9.0.1-17 Reproducible in staging?: Yes Reproducible in production?: Yes 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 Expensify/Expensify Issue URL: Issue reported by: @quinthar Slack conversation: https://expensify.slack.com/archives/C05LX9D6E07/p1719169189159419

Action Performed:

  1. Go to staging.new.expensify.com
  2. Initiate a DM to any user
  3. Tap mic icon on keyboard and say a message to be composed

Expected Result:

Message composed without any issues

Actual Result:

Voice recognition does not work if you begin it in an empty field, lose focus and it stops

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f063fe0495eff4ac
  • Upwork Job ID: 1805694285589168826
  • Last Price Increase: 2024-08-08
  • Automatic offers:
    • eh2077 | Reviewer | 103185927
Issue OwnerCurrent Issue Owner: @puneetlath
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @deetergp (AutoAssignerNewDotQuality)

deetergp commented 3 months ago

This one seems like it could be done by an External contributor.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

diegohosilver commented 3 months ago

I was able to reproduce it in dev and stage

https://github.com/Expensify/App/assets/20136219/8c8490dd-5f2d-461b-b345-f31c20799195

MitchExpensify commented 3 months ago

@diegohosilver It looks like its working in your video, am I missing something?

diegohosilver commented 3 months ago

@MitchExpensify the focus is interrupted when the input is empty. I had to press the mic button again to continue with the dictation.

I narrowed down the problem to the RNMarkdownTextInput component. The usage of MarkdownTextInput from '@expensify/react-native-live-markdown' is what introduced this behavior. I will continue my testing tomorrow

diegohosilver commented 3 months ago

Proposal

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

Android dictation cuts off in Chrome web

What is the root cause of that problem?

The package @expensify/react-native-live-markdown is causing this behavior. The code below is causing the dictation to cut off when setting targetElement.innerHTML = ''

 if (!rootSpan || rootSpan.innerHTML !== dom.innerHTML) {
      targetElement.innerHTML = '';
      targetElement.innerText = '';
      target.appendChild(dom);

      if (BrowserUtils.isChromium) {
        moveCursor(isFocused, alwaysMoveCursorToTheEnd, cursorPosition, target);
      }
 }

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

  1. Check if the current text is different from the parsed ranges to HTML nodes
  2. Clear the targetElement innerHTML by removing child nodes
  3. Append the HTML nodes to the targetElement by cloning them

What alternative solutions did you explore? (Optional)

N/A

eh2077 commented 3 months ago

@diegohosilver Thanks for your proposal! Can you elaborate on why this issue happens on mobile Chrome only?

diegohosilver commented 3 months ago

@eh2077 no problem!

I'm not 100% sure. I would say it has to do with Android's WebView implementation:

I'd like to provide an answer on why this is happening, but this is where the rabbit hole led me.

eh2077 commented 3 months ago

@diegohosilver Thanks for the comment. But the root cause is still not clear to me.

Looking for better proposals

melvin-bot[bot] commented 3 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

eh2077 commented 3 months ago

@MitchExpensify Should we consider increasing the bounty to get proposals to dig into the root cause of this issue?

MitchExpensify commented 3 months ago

Early days for a bump, lets give it another week

jacobkim9881 commented 2 months ago

It's not reproducible.

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 2 months ago

@deetergp @MitchExpensify @eh2077 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 2 months ago

@deetergp, @MitchExpensify, @eh2077 Eep! 4 days overdue now. Issues have feelings too...

eh2077 commented 2 months ago

Still waiting for proposals

mvtglobally commented 2 months ago

Issue not reproducible during KI retests. (First week)

muttmuure commented 2 months ago

Sounds like this might be fixed

muttmuure commented 2 months ago

Let's ask David to retest it in the thread

muttmuure commented 2 months ago

Asking here: https://expensify.slack.com/archives/C05LX9D6E07/p1719169189159419

muttmuure commented 2 months ago

David is still experiencing this

eh2077 commented 2 months ago

Still waiting for proposals

muttmuure commented 2 months ago

As Scott is working on another critical in LHN unreads, I will reapply the autoassigner

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @puneetlath (AutoAssignerNewDotQuality)

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

MitchExpensify commented 2 months ago

Asking CT for help with this here https://expensify.slack.com/archives/C03UK30EA1Z/p1721150619478589

melvin-bot[bot] commented 2 months ago

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

puneetlath commented 2 months ago

CT said they'll assign someone tomorrow.

mvtglobally commented 2 months ago

Issue not reproducible during KI retests. (Second week)

JKobrynski commented 2 months ago

Hi, I'm Julian from Callstack - expert agency - and I would like to work on this issue.

eh2077 commented 2 months ago

Not overdue, Callstack team just picked up this issue

melvin-bot[bot] commented 2 months ago

📣 @eh2077 🎉 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

MitchExpensify commented 2 months ago

I don't think we need another Bug Zero team assignee here as I head out on paternity seeing as @puneetlath is on the team. As such unassigning myself without tracking down a leave buddy

melvin-bot[bot] commented 2 months ago

@puneetlath, @JKobrynski, @eh2077 Whoops! This issue is 2 days overdue. Let's get this updated quick!

eh2077 commented 2 months ago

@JKobrynski Do you have any updates on this?

melvin-bot[bot] commented 2 months ago

@puneetlath @JKobrynski @eh2077 this issue is now 4 weeks old, please consider:

Thanks!

muttmuure commented 2 months ago

Pinged @JKobrynski in Slack

JKobrynski commented 2 months ago

I've spent some time investigating this issue today and I've narrowed it down to @react-native-live-markdown. I forked the repo and was able to reproduce this exact bug in the example web app. I'm going to dig deeper into the package itself to try to understand why that's happening. If necessary, I'm going to create an issue in the package's repo and also contact SWM devs who were working on it to speed things up.

JKobrynski commented 2 months ago

Another update! I was able to pinpoint the exact function that is causing this bug, will try to fix it

JKobrynski commented 2 months ago

Today's investigation led me to a conclusion that the exact thing that causes this bug is this line

ref={setRef}

in MarkdownTextInput.web.tsx. If removed, everything works fine.

I'm going to continue my work on it, and try to make this ref work.

JKobrynski commented 2 months ago

Today's further investigation led me from setRef function to parseText method of parserUtils.ts, more specifically the following three lines:

  targetElement.innerHTML = '';
  targetElement.innerText = '';
  target.appendChild(dom);

that seem to be causing the issue. The dom value comes from parseRangesToHTMLNodes method from the same file, and I suspect that this where the things get mixed up. I will continue my investigation there.

diegohosilver commented 2 months ago

@JKobrynski check my proposal from a month ago. I discovered that too and proposed a fix. It might help you a bit

JKobrynski commented 2 months ago

Today I reached out to @BartoszGrajdek who coauthored the library, you can check the results of our joint investigation/discussion here

BartoszGrajdek commented 2 months ago

Hi! Tried debugging this issue a little bit yesterday/today to help out, but didn't manage to find anything yet due to the lack of time.

Unfortunately, I'm OOO till 11.08, but once I'm back I'll try helping you here if this doesn't get fixed.

Something I also told Julian is that if I remember correctly we used to have the parseText method without targetElement.innerHTML = ''; (something like @diegohosilver proposal) but the problem was that in some cases it added additional line breaks, or broke cursor positioning, keep that in mind if you go that route and make sure it doesn't introduce any similar regressions.

melvin-bot[bot] commented 1 month ago

@puneetlath, @JKobrynski, @eh2077 Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 1 month ago

@puneetlath, @JKobrynski, @eh2077 6 days overdue. This is scarier than being forced to listen to Vogon poetry!