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
2.99k stars 2.5k forks source link

[HOLD for payment 2022-12-20] [$8000] iOS Safari - Blue text selection persists in the compose box even after message is sent @metehanozyurt #8592

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

In iOS Settings, Go to General > Keyboard and enable autocorrection

  1. Go to https://new.expensify.com on Safari iOS
  2. Log in with any account and select any chat
  3. Enter incorrectly spelled word in the compose box and let the autocorrection warn you with the blue text selection.
  4. Send the message

Expected Result:

Compose box should be cleared and empty blue selection shouldn't persist.

Actual Result:

Blue text selection still exist.

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.1.92.0 (Matt A retested 8/26/22)

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): n/a

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

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/162746866-ab9925a0-99c9-4f8f-b63b-2c0601647649.mp4

https://user-images.githubusercontent.com/93399543/162746890-ad84201d-c88b-4bba-9ae4-7d2210b4f1ef.mp4

Expensify/Expensify Issue URL:

Issue reported by: @metehanozyurt

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

View all open jobs on GitHub

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

@cdraeger11 Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 2 years ago

@cdraeger11 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

melvin-bot[bot] commented 2 years ago

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

MonilBhavsar commented 2 years ago

Can't seem to reproduce on Safari web, but can reproduce on iOS simulator

MonilBhavsar commented 2 years ago

Asked for confirmation on a PR which I think caused this.

parasharrajat commented 2 years ago

Looked at the issue. Tried a few things to see if those fix it but no luck. I will try more tomorrow.

melvin-bot[bot] commented 2 years ago

@MonilBhavsar Huh... This is 4 days overdue. Who can take care of this?

MonilBhavsar commented 2 years ago

Will get back to this

MonilBhavsar commented 2 years ago

@parasharrajat since this seems like a regression, do you want to give it a try to fix this?

parasharrajat commented 2 years ago

I would be happy to do that but I have been preoccupied with other stuff. I will try to debug this over the weekend and let you know by Monday.

parasharrajat commented 2 years ago

I tried many different approaches to solve the problem. But none of them worked. Basically, the selection length is already 0 so there is no reset. I tried to solve 7798 with a different approach than what I have on the PR but this issue persists.

I suggest exporting this if someone can fix this. My ultimate goal was to keep the spellcheck| autoCorrect behavior and only try to reset the blue highlight.

mallenexpensify commented 2 years ago

@MonilBhavsar , any reason this shouldn't be labeled External?
If it's confirmed to be a regression, let's add it to the RCA list even if we don't know what the offending/root issue or PR is yet.

MonilBhavsar commented 2 years ago

No reason, adding external label now. I'm not sure if we should add it to RCA list as it's not the breaking anything

melvin-bot[bot] commented 2 years ago

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

arielgreen commented 2 years ago

https://www.upwork.com/jobs/~01208db9c91b51fa69

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

Current assignee @MonilBhavsar is eligible for the Exported assigner, not assigning anyone new.

arielgreen commented 2 years ago

Price increased

songlang1994 commented 1 year ago

Proposal

I believe it's an issue of Safari that we can not fix it on the DOM side.

But if you do need to "fix" this issue, a workaround is disabling the autocorrect feature.

// File: src/components/Composer/index.js

<RNTextInput
  autoComplete="off"
+ autoCorrect={false}          // always disable auto correct
+ autoCorrect={isMobileSafari} // or more precisely
  placeholderTextColor={themeColors.placeholderText}
  ref={el => this.textInput = el}
parasharrajat commented 1 year ago

Thanks for the proposal. But as I said here https://github.com/Expensify/App/issues/8592#issuecomment-1116074045. The ultimate goal should be to fix the issue without disabling autoCorrect.

songlang1994 commented 1 year ago

@parasharrajat I've read your comment before I post the proposal. But I misunderstood that you mean the spellCheck prop. Spell checking and auto correcting are two different features.

Let's wait a more appropriate solution.

melvin-bot[bot] commented 1 year ago

@mananjadhav, @MonilBhavsar, @arielgreen Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

mananjadhav commented 1 year ago

Awaiting proposals

arielgreen commented 1 year ago

Price increased

parasharrajat commented 1 year ago

@mananjadhav Could you please update the RCA doc to add yourself as the leading person? You are C+ here.

allroundexperts commented 1 year ago

@parasharrajat How about doing a blur on click of send button? Something like:

this.textInput.blur();
setTimeout(() => this.textInput.focus(), 500);

We can also make this Safari iOS specific.

Result: https://user-images.githubusercontent.com/30054992/170832542-ddeacd00-455d-42d6-9b3a-d06b0a1d867c.mp4

parasharrajat commented 1 year ago

We never give consider setTimeout hacks as solutions. But this can be used as last resort.

What is the effect of this change on all platforms? What will be your plan for all the platforms?

allroundexperts commented 1 year ago

@parasharrajat This would be applicable to iOS Safari only. This seems to be a problem with Safari browser itself. We can't really create a fix PR against their repo since it's private.

allroundexperts commented 1 year ago

@parasharrajat Also, do we need to re-focus after blur? IMO, once the user clicks send button, the field should just blur. It should not remain in focus.

parasharrajat commented 1 year ago

Yeah, Focus should be remained on the composer after submit. That's what is the main motive.

arielgreen commented 1 year ago

Price increased

dhairyasenjaliya commented 1 year ago

Proposal

Solution

  1. For Safari (web app) it's an issue of Safari that we can not fix on the DOM side (blue background).

  2. To fix the issue in iOS (App) we need to comment out the line setNativeProps in submit() function as we are already clearing the text from the composer after submitting text so applying selection to empty text causes this issue

    this.textInput.setNativeProps({selection: {start: 0, end: 0}});

https://github.com/Expensify/App/blob/11a1f0debac2dfee461e6b71751c3b3bcfb9f2cf/src/pages/home/report/ReportActionCompose.js#L425-L430

Like this :

 this.props.onSubmit(trimmedComment);
 this.updateComment(''); // here we are already clearing composer textinput so applying 0 selection below this has no effect 
 this.setTextInputShouldClear(true);
 // Important to reset the selection on Submit action
 //this.textInput.setNativeProps({selection: {start: 0, end: 0}}); // comment this line solves problem in iOS (app)

Note: commenting on this line won't affect any other platform as we are already clearing the composer

Result (After commenting setNativeProps line) :

Device : iPhone 13(App)

https://user-images.githubusercontent.com/47522946/173188941-a898f924-0cd1-455b-97f5-35d35ee38f0f.mp4

Result (Without commenting setNativeProps line Current behaviour) :

https://user-images.githubusercontent.com/47522946/173188991-d0c0ecf2-dcbc-4ea5-b165-69431e46cc8f.mp4

arielgreen commented 1 year ago

@mananjadhav

MTN718 commented 1 year ago

Proposal

Current issue from Safari mobile web browser. Because of auto correct text not initializing on Virtual Keyboard when update TextInput.

Solution

Add blur on Textinput when submit Add focus on After submit done

image

https://user-images.githubusercontent.com/12084368/173642232-ffdd6586-aba2-4cd2-9d9e-acfaec81ffaa.mov

mananjadhav commented 1 year ago

@parasharrajat I don't have enough context on this and I can see you've been involved in this one. Would you mind taking up C+ on this one?

parasharrajat commented 1 year ago

Yeah, sure.

mananjadhav commented 1 year ago

@arielgreen Can you please assign @parasharrajat as C+ on this one?

MTN718 commented 1 year ago

Any feedback for my proposal?

parasharrajat commented 1 year ago

@MTN718 this a duplicate proposal.

MTN718 commented 1 year ago

So it's not solution for this issue?

melvin-bot[bot] commented 1 year ago

📣 @parasharrajat You have been assigned to this job by @MonilBhavsar! Please apply to this job in Upwork 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 📖

MonilBhavsar commented 1 year ago

Assigned Rajat, oh no this is wrong. Please ignore

MTN718 commented 1 year ago

This is not available to post proposal?

MonilBhavsar commented 1 year ago

No sorry, my bad. This is open for proposals. But as @parasharrajat mentioned, your proposal seems similar to the one we already received and we don't want to go with that.

MTN718 commented 1 year ago

Ok But I have tried lots of ways to resolve this issue. But that's the only way to resolve this issue I have tried. :)

dhairyasenjaliya commented 1 year ago

hello @parasharrajat @MonilBhavsar can you also review my above proposal thank you

parasharrajat commented 1 year ago

For Safari (web app) it's an issue of Safari that we can not fix on the DOM side (blue background).

@dhairyasenjaliya This issue is mainly focused on fixing the issue on iOS web.

dhairyasenjaliya commented 1 year ago

Got it @parasharrajat but have you checked issue in iOS app like mentioned in video i think that might be other bug then this one?