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.45k stars 2.81k forks source link

[$250] Chat - mWeb - Send button is activated only after entering a space #49486

Open IuliiaHerets opened 2 weeks ago

IuliiaHerets commented 2 weeks 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.38-0 Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Open a chat
  3. Enter a text
  4. Note send button not activated
  5. Enter a space
  6. Now note send button is activated

Expected Result:

Send activated after entering few characters.

Actual Result:

Send button is activated in mweb only after entering a space. Issue occurs when using the device's own keyboard.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/0b88bd41-2053-4f49-8b6d-5714b11b76f5

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836866672094965087
  • Upwork Job ID: 1836866672094965087
  • Last Price Increase: 2024-10-03
  • Automatic offers:
    • allgandalf | Contributor | 104090050
Issue OwnerCurrent Issue Owner: @allgandalf
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @zanyrenney (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.

IuliiaHerets commented 2 weeks ago

@zanyrenney FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

mattmoreira commented 2 weeks ago

Proposal

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

Fix send button being activated only after entering a space on Chrome for Android

What is the root cause of that problem?

When typing using Android's keyboard, only the input event is triggered, while pressing space or typing using an external keyboard activates the textInput event, which enables the send button.

That's likely caused by a difference on how onChange and onChangeText are handled by the TextInput component on web.

https://github.com/user-attachments/assets/fb813f1b-988d-4862-8919-aba2698f5358

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

Replace the usage of onChangeText by onChange on the line below: https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx#L773

melvin-bot[bot] commented 2 weeks ago

๐Ÿ“ฃ @mattmoreira! ๐Ÿ“ฃ Hey, it seems we donโ€™t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
mattmoreira commented 2 weeks ago

Contributor details

Your Expensify account email: matheus.moreira@pixelias.io Upwork Profile Link: https://www.upwork.com/freelancers/~011382b37ba6ccea27?mp_source=share

melvin-bot[bot] commented 2 weeks ago

โœ… Contributor details stored successfully. Thank you for contributing to Expensify!

paultsimura commented 2 weeks ago

I cannot reproduce on main ๐Ÿค”

bondydaa commented 2 weeks ago

@paultsimura can you checkout the staging branch and production tags and confirm its reproducible there?

paultsimura commented 2 weeks ago

@bondydaa still no luck โ€“ย works fine on both staging & prod. I will request in the C+ channel, maybe somebody will manage to reproduce & take over.

allgandalf commented 2 weeks ago

I am able to re-produce this issue, taking over (slack), @bondydaa @zanyrenney can someone please assign me here, thanks :)

https://github.com/user-attachments/assets/52be78a0-548d-41ea-8c4d-d6045e0cb3c6

mattmoreira commented 2 weeks ago

@bondydaa and @paultsimura, I had already reproduced the bug, please check my prior comment in thread where I issued a proposal on how to fix this.

allgandalf commented 2 weeks ago

@bondydaa and @paultsimura, I had already reproduced the bug, please check my prior comment in thread where I issued a proposal on how to fix this.

I am taking over as C+ @mattmoreira , not as a contributor ๐Ÿ˜„

allgandalf commented 2 weeks ago

@mattmoreira , can you please explain in your RCA why this is only reproducible on android web and not other platforms, please try to dig in deeper, also I think your solution is not correct, please dig in why onChangeText was used here at the first place

mattmoreira commented 2 weeks ago

@allgandalf I'll explain later why I proposed to use onChange instead of onChangeText.

Regarding why it happens only on Chrome for Android, I suspect it's related to the following issue that was reported on Chromium's bug tracker:

https://issues.chromium.org/issues/41403652

mattmoreira commented 2 weeks ago

Ok, I'll provide now more details:

Why the issue only happens on Chrome for Android?

Chrome on Android uses Blink as a rendering engine, while for iOS, Chrome works basically as a reskinned version of Safari: https://news.ycombinator.com/item?id=25850091

Also, according to this comment on Chromium's bug tracker, the textInput event is behaving as intended by not "committing" the new character until space is pressed, as the string might be changed by the autocorrector.

That doesn't happen on desktop because we use real keyboards for typing, while Gboard acts as an input method (IME), having complete control of when the string actually gets sent to the element.

Why onChangeText is being used?

The biggest difference between onChangeText and onChange is that the first only returns the string that was input, while the latter returns the synthetic event. That led me to believe that perhaps the onChange was being triggered by the input event, but after further research, I discovered that they're both triggered by the same event.

https://github.com/Expensify/react-native-live-markdown/blob/main/src/MarkdownTextInput.web.tsx#L335-L374

So, yes @allgandalf, you're correct, replacing onChangeText with onChange wouldn't solve the issue.

Also, it seems that the element is not actually an input, but rather a content editable div. This explains why the real change event wasn't used, as it doesn't work with divs.

Are there any other alternatives?

Another solution would be to use onKeyPress to identify when the element has changed, and then grab the new value using innerText on the target element.

https://stackoverflow.com/a/73238998

klajdipaja commented 2 weeks ago

Proposal

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

In the chat the send button is not enabled when the user starts typing on mobile browsers

What is the root cause of that problem?

The root cause of the issue is mobile keyboard behavior's when text prediction is enabled. I have tested this one the mobile chrome browser with the google gBoard and Samsung keyboard on S22FE and only in the samsung keyboard I've been able to reproduce the issue. The Google keyboard generates when typing events with eventType=input and inputType=insertText. The Samsung keyboard generates when typing events with eventType=input and inputType=insertCompositionText. Due to the inputType being wrong in the event it is not being picked up by react with the onInput event which is what the react-native-live-markdown library use to expose the onChangeText. We need to fix this in the live markdown library.

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

We can leverage the composition events to get the text by adding a listener to onCompositionUpdate in this line: https://github.com/Expensify/react-native-live-markdown/blob/a70e8570658a3654aa34d7817188474890c1fe15/src/MarkdownTextInput.web.tsx#L688

The event would use an updateComposition function to update the text by calling the onChangeText function:

const updateComposition = useCallback(
      (e) => {
        handleOnChangeText(e);
      },
      [handleOnChangeText],
    );

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 2 weeks ago

๐Ÿ“ฃ @allgandalf ๐ŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role ๐ŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review ๐Ÿง‘โ€๐Ÿ’ป Keep in mind: Code of Conduct | Contributing ๐Ÿ“–

bondydaa commented 2 weeks ago

dang it, that's what I was trying to avoid ๐Ÿคฆ guess i should have taken external off as well before try to re-assign?

bondydaa commented 2 weeks ago

anyways @allgandalf you're the c+ assigned now, we can get the job stuff figured out when Zany is back tomorrow b/c now I'm not sure what I need to do.

allgandalf commented 2 weeks ago

lesson for the next time ๐Ÿ˜† ,

anyways @allgandalf you're the c+ assigned now, we can get the job stuff figured out when Zany is back tomorrow b/c now I'm not sure what I need to do.

No hurry !! Still waiting for proper proposals

mattmoreira commented 2 weeks ago

No hurry !! Still waiting for proper proposals

@allgandalf I've tried to address here the points that were unclear, could you please provide me guidance on what other areas would you like to be clarified in my proposal?

bernhardoj commented 2 weeks ago

Proposal

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

Send button is only enabled when adding white space to the composer.

What is the root cause of that problem?

The send button will be enabled if the isCommentEmpty state is false. We update the state inside the updateComment function. https://github.com/Expensify/App/blob/accea60e318386b02971baab3a563160ce8259b5/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx#L432-L434

updateComment is called from the composer onChangeText. https://github.com/Expensify/App/blob/accea60e318386b02971baab3a563160ce8259b5/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx#L509-L511

However, the onChangeText isn't called. This is because, in the live markdown component, if compositionRef is true, then we return early which never reaches the onChangeText calls. https://github.com/Expensify/react-native-live-markdown/blob/a70e8570658a3654aa34d7817188474890c1fe15/src/MarkdownTextInput.web.tsx#L307-L316

compositionRef is true when we start typing, only if the auto-correct of the keyboard is enabled. It becomes false when we press space which ends the composition (it triggers onCompositionEnd event).

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

We need to trigger onChangeText even when compositionRef is true, the same we trigger selection change event (updateSelection). https://github.com/Expensify/react-native-live-markdown/blob/a70e8570658a3654aa34d7817188474890c1fe15/src/MarkdownTextInput.web.tsx#L307-L315

if (compositionRef.current) {
  updateTextColor(divRef.current, parsedText);
  updateSelection(e, {
    start: newCursorPosition,
    end: newCursorPosition
  });
  if (onChangeText) {
    onChangeText(parsedText);
  }
  ...
zanyrenney commented 1 week ago

@allgandalf are you able to review the proposal from @bernhardoj please?

zanyrenney commented 1 week ago

Just an FYI I'll be OOO until Tuesday 1st, please reapply the bug label if this needs urgent action in my absence. Otherwise I will get to it when I return. Thanks!

melvin-bot[bot] commented 1 week 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 1 week ago

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

allgandalf commented 1 week ago

Just an FYI I'll be OOO until Tuesday 1st, please reapply the bug label if this needs urgent action in my absence. Otherwise I will get to it when I return. Thanks!

Oops sorry I didn't realise there was a proposal, I will review it tomorrow / over the weekend, thanks :)

melvin-bot[bot] commented 1 week ago

@bondydaa, @zanyrenney, @allgandalf Whoops! This issue is 2 days overdue. Let's get this updated quick!

allgandalf commented 1 week ago

not overdue melvin!

melvin-bot[bot] commented 5 days 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 5 days ago

@bondydaa @zanyrenney @allgandalf 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!

allgandalf commented 4 days ago

Not overdue melvin

zanyrenney commented 4 days ago

added to the collect project as a key feature of admins needing to pay, is paying. Added to polish.

melvin-bot[bot] commented 1 day ago

@bondydaa, @zanyrenney, @allgandalf Whoops! This issue is 2 days overdue. Let's get this updated quick!

bondydaa commented 1 day ago

@allgandalf any updates?

allgandalf commented 13 hours ago

Can you add re-test label here? I think this has been fixed upstream