search

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.12k stars 2.62k forks source link

[HOLD for payment 2023-05-23] [$1000] Chat - The cursor absent in the chat input field #17187

Closed kbecciv closed 1 year ago

kbecciv commented 1 year 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 Performed:

  1. Open the App
  2. Login with any account
  3. Open any different chats one by one

Expected Result:

The cursor present in input field.

Actual Result:

The cursor is sometimes present, but in most cases absent in the chat input field.

Workaround:

Unknown

Platforms:

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

Version Number: 1.2.97.2

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

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/230912246-4a6bd626-ef64-4281-8b3f-b9668cdedf95.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010ec7a699a5894775
  • Upwork Job ID: 1645416218945282048
  • Last Price Increase: 2023-05-02
MelvinBot commented 1 year ago

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

0xmiroslav commented 1 year ago

@kbecciv @bfitzexpensify what is "chat input field"? I checked video but it's mainly focusing on IOU amount input.

kbecciv commented 1 year ago

@0xmiroslav This is composed box

0xmiroslav commented 1 year ago

@0xmiroslav This is composed box

@kbecciv you mean around 12s-14s in video?

kbecciv commented 1 year ago

@0xmiroslav Very very sorry, I attached a wrong video, just added a new one. The input field was missing on 7s -8s on video.

vladbogza123 commented 1 year ago

Proposal

What is the root cause of that problem?

The issue here is that the keyboard does not pop up on a chat input when clicking around to different chats. We get the functionality sometimes because IOS or Android can auto-highlight an input.

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

Add a piece of logic that highlights each input in each chat. That way the keyboard pops up each time.

MelvinBot commented 1 year ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

ericsonrd commented 1 year ago

Proposal

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

The cursor behavior is not consistent across native and mWeb. It shouldn’t show up on non-empty chats.

What is the root cause of that problem?

The <Composer> component logic pretty much ensures the input is auto-focused all the time. In native platforms, autofocus behave a differently and the provided logic doesn’t fully apply, thus no auto-focusing when chat is not empty on native.

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

A small modification to the autofocus logic should be added leaving out instances where the chat is not empty. This solution doesn’t involve reverting #14230. This modification should be done:

autoFocus={!this.props.modal.isVisible && (this.willBlurTextInputOnTapOutside && this.isEmptyChat() || this.isEmptyChat())}

What alternative solutions did you explore?

n/a

Results

MelvinBot commented 1 year ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

MelvinBot commented 1 year ago

📣 @vladbogza! 📣

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. 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.
  2. 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.
  3. 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>
vladbogza123 commented 1 year ago

Contributor details Your Expensify account email: vladimirbogza@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0117583cb41393c914

MelvinBot commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

0xmiroslav commented 1 year ago

This is expected behavior. This can be issue only when chat is empty but cursor absent. @kbecciv your video shows that most reports are not empty (enough message history) and it's expected that cursor is absent.

0xmiroslav commented 1 year ago

One more thing to confirm: I am heard that on mobile devices, cursor should always show but when chat is not empty, just hide keyboard, similar to whatsapp. cc: @bfitzexpensify @flodnv

kbecciv commented 1 year ago

@0xmiroslav Great, thank you for clarification.

flodnv commented 1 year ago

I agree, this is not a bug.

0xmiroslav commented 1 year ago

@flodnv I found opposite behavior on android mChrome. I opened report with enough chat history but still auto-focus input, cursor present and keyboard shows. This should be bug. The expected behavior is not to auto-focus.

https://user-images.githubusercontent.com/97473779/232041403-6f7a19ba-9041-49b1-bf8b-24ab06471163.mov

flodnv commented 1 year ago

To clarify, are you saying that this behavior in your screenshot, is a bug, because it is different on android mChrome than everywhere else?

0xmiroslav commented 1 year ago

To clarify, are you saying that this behavior in your screenshot, is a bug, because it is different on android mChrome than everywhere else?

yes correct. Also happens on mSafari. So the issue is inconsistent behavior between native and mWeb when report has enough chat history.

flodnv commented 1 year ago

Got it, then yes, we need to fix this inconsistent behavior on mWeb to make it match other platforms

dukenv0307 commented 1 year ago

Proposal

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

On mWeb, when opening report with enough chat history, the composer still auto-focus input, cursor present and keyboard shows.

What is the root cause of that problem?

Here https://github.com/Expensify/App/blob/e702c3a45934767da5a5da5f70f642e0da89bcb3/src/pages/home/report/ReportActionCompose.js#L838, we're auto focusing the input when the willBlurTextInputOnTapOutside is true, no matter whether the chat is empty or not. willBlurTextInputOnTapOutside will be true on mWeb, leading to this consistency issue.

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

We need to replace willBlurTextInputOnTapOutside by canFocusInputOnScreenFocus() in https://github.com/Expensify/App/blob/e702c3a45934767da5a5da5f70f642e0da89bcb3/src/pages/home/report/ReportActionCompose.js#L838 so that for mWeb it will not auto focus if the chat is not empty, similar to on native. It will still auto focus for desktop and desktop web, I assume we want that.

If we want the input to not show the green focus highlight around the composer to be consistent with the focused behavior of the Composer, we should replace willBlurTextInputOnTapOutside by canFocusInputOnScreenFocus() on this line and add this.isEmptyChat() as well

What alternative solutions did you explore? (Optional)

NA

0xmiroslav commented 1 year ago

@dukenv0307 your solution simply reverts #14230 which fixes #13443 so 👎

MelvinBot commented 1 year ago

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

hoangzinh commented 1 year ago

Proposal

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

Inconsistent behavior between native and mWeb. On mWeb, when opening report with enough chat history, the composer still auto-focus input

What is the root cause of that problem?

The composer is auto-focus on mWeb because this line https://github.com/Expensify/App/blob/f5991465754ede68c429989c21239d29cbb0f077/src/pages/home/report/ReportActionCompose.js#L838 willBlurTextInputOnTapOutside currently is defined as true on mWeb but it's false in native devices

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

If we want to keep behavior consistency between mWeb and native devices, we can replace willBlurTextInputOnTapOutside by

!DeviceCapabilities.canUseTouchScreen()

in this line.

That means in touchable device, it will not autofocus.

ericsonrd commented 1 year ago

Proposal updated taking into account the updated issue.

Proposal

Updated

bfitzexpensify commented 1 year ago

Couple of proposals for you to review @0xmiroslav

MelvinBot commented 1 year ago

@flodnv @bfitzexpensify @0xmiroslav 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!

MelvinBot commented 1 year ago

@flodnv, @bfitzexpensify, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

bfitzexpensify commented 1 year ago

Awaiting proposal review

0xmiroslav commented 1 year ago

@ericsonrd your root cause is not correct

0xmiroslav commented 1 year ago

@dukenv0307 your root cause is correct but solution is not 100%. input will not be auto-focused on touchable laptops.

0xmiroslav commented 1 year ago

@hoangzinh your proposal is same as @dukenv0307.

ericsonrd commented 1 year ago

@ericsonrd your root cause is not correct

I don't know if I stated it well, but I'm basically saying the same as other proposals. What I mean is the logic focuses all the time for mWeb because willBlurTextInputOnTapOutside is forced true on load and it doesn't discern if chat is empty or not

0xmiroslav commented 1 year ago

@ericsonrd also, I didn't get your solution. what condition should be added or removed exactly?

ericsonrd commented 1 year ago

@ericsonrd also, I didn't get your solution. what condition should be added or removed exactly?

Here it is.

Proposal

Updated

hoangzinh commented 1 year ago

Proposal

Updated https://github.com/Expensify/App/issues/17187#issuecomment-1513523839 cc @0xmiroslav

0xmiroslav commented 1 year ago

@hoangzinh I don't think your updated solution will work on iPad, tablet. So the idea is to autofocus only on devices which don't have virtual keyboard which makes screen push.

0xmiroslav commented 1 year ago

@ericsonrd I am still not following your updated solution.

MelvinBot commented 1 year ago

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

ericsonrd commented 1 year ago

@ericsonrd I am still not following your updated solution.

My solution is to add && this.isEmptyChat() to the this.willBlurTextInputOnTapOutside condition,

from: autoFocus={!this.props.modal.isVisible && (this.willBlurTextInputOnTapOutside || this.isEmptyChat())}

to autoFocus={!this.props.modal.isVisible && (this.willBlurTextInputOnTapOutside && this.isEmptyChat() || this.isEmptyChat())}

It results in disabling autofocus on mWeb when chat is not empty.

0xmiroslav commented 1 year ago

@ericsonrd your solution always prevents autoFocus even on web if it's not empty chat. This is wrong.

hoangzinh commented 1 year ago

@0xmiroslav I have updated my proposal again to support ipad case => https://github.com/Expensify/App/issues/17187#issuecomment-1513523839

Please help to review it again. TIA

flodnv commented 1 year ago

@0xmiroslav what are your thoughts about that latest update from @hoangzinh ?

MelvinBot commented 1 year ago

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

0xmiroslav commented 1 year ago

@hoangzinh where are you applying this condition? !(this.props.isKeyboardShown || isSmallScreenWithTouchableDevice())