Composer: Control selection using setSelection by s77rt · Pull Request #17687 · Expensify/App

s77rt · 2023-04-19T21:11:35Z

Details

Instead of controlling the selection the traditional way (passing selection prop) which is always render dependent and is so buggy on RN especially for Android. We are going to control the selection directly using an independent and render free method which is setSelection for Native and setSelectionRange for Web. We we will be using our setSelection library for that.

Fixed Issues

$ #12854
PROPOSAL: #12854 (comment)

Tests

Open App
Go to any chat
Type message Hi there
Move the cursor to be between Hi and there
Type :wave:
Verify the 👋 emoji got inserted correctly between Hi and there
Verify the cursor position is after the emoji
Verify the cursor did not jump around

Verify that no errors appear in the JS console

Offline tests

n/a

QA Steps

Open App
Go to any chat
Type message Hi there
Move the cursor to be between Hi and there
Type :wave:
Verify the 👋 emoji got inserted correctly between Hi and there
Verify the cursor position is after the emoji
Verify the cursor did not jump around

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mp4

Mobile Web - Chrome

mweb-chrome.mp4

Mobile Web - Safari

mweb-safari.mp4

Desktop

desktop.mp4

iOS

ios.mp4

Android

android.mp4

MelvinBot · 2023-04-19T21:11:43Z

@MariaHCD @mollfpr One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

src/components/Composer/index.android.js

hannojg · 2023-04-21T09:43:50Z

I would highly recommend to run the tests from this PR, as it should cover most edge cases.

(Edit: I can also take some time to run these tests if needed, as I am by now quite experiences with the weird bugs that can happen when using SetSelection)

Here is a copy of it:

Regression tests:

Test plan

Helper list to confirm you ran every test

All platforms, make sure for web to run on every browser (Safari, Chrome, Firefox, Edge):

Selection is not reset after selecting an emoji

Add multiple message on the composer.
selected multiple lines.
Open the emoji picker and choose an emoji.
selection remained.

screen-2022-10-12_01.10.50.mp4

Broken emojis are rendered when picked from the picker in a series

Add a bunch of text lines in the composer.
Move the cursor to some middle position in the para.
Now pick an emoji from the emoji picker.
Try to pick the same emoji a few more times from the picker.

screen-2022-10-12_01.35.38.mp4

Cursor position is broken

Steps

Add a bunch of lines to the composer on web.
Place your cursor somewhere in the middle.
Go to any other page and copy some strings. (e.g. Copy step 2 in clipboard)
Paste in the composer.
Cursor is not at the end of pasted string.

screen-2022-10-12_01.46.21.mp4

Steps

Steps.

Open the app on the web.
Copy long text from some where (only text no markup).
Paste it on the composer.
Cursor is at the start of the pasted text.

Expected: it should be at the end of the pasted text.

Adding emoji via EmojiPicker in replacement for multiple lines creates a delay before the composer height is calculated.

Add multi-line message in composer.
Selects the multi-line text.
Add an emoji from the picker.
Observe that there is a delay before the composer height is adjusted correctly.

screen-2022-10-19_01.19.07.mp4

Selected Emoji is placed in the wrong position.

open the app.
Add three lines of message in composer.
Select some text from the middle line.
Open Emoji Picker and select an emoji.
Notice emoji is placed correctly and the selected text is gone.
Now reopen the EmojiPicker.
Select another emoji.
Notice that new emojis are added to the end of the line (not the previous cursor position).
Repeat from 4 to 6 to observe the same pattern.

[Web]

Pasting image does not send the attached text message.

Type some message in the composer.
Go to image and copy the image.
Back to the app and focus after the typed text in the composer.
Press CTRL+V to paste the image.
Click send on the opened attachment modal.
Observe the image is sent but the text remained in the composer and Send button is disabled.

Expected: Pressing send on the attachment modal should first send the text and then the image. (See staging for correct behaviour).

There is a delay before Composer expands the height on the web.

Open the app on the web.
type text quickly.
Use Shift+Enter to enter a couple of lines.

screen-2022-11-16_19.39.59.mp4

Cursor position is not focused after picking a emoji in Composer.

Paste the following content in composer.

ASDSADSA
DA😆 🤫 😙 
SD
ASDSA
D
ASDAS
DSA
DSA
DAS
D
AS
D


😋 🤫 😋 😋 😋 
SADSA
D
ASD
SA
D
SADS
AD
ASDAS
DAS

Now place the cursor after the first three emojies.
Scroll the composer content below so that cursor is no more visible but composer remains focused.
open emoji Picker.
select an emoji.
Notice that cursor position is not revealed.

screen-2023-01-05_00.03.18.mp4

Content flashing while pasting

Paste the following content in composer.

ASDSADSA
DA😆 🤫 😙 
SD
ASDSA
D
ASDAS
DSA
DSA
DAS
D
AS
D


😋 🤫 😋 😋 😋 
SADSA
D
ASD
SA
D
SADS
AD
ASDAS
DAS

Now use Cmd + A to select the whole content in the composer.
Paste the same copied content to replace.
Notice that there is a flashing of content. (Not present on staging).

screen-2023-01-05_00.11.41.mp4

Edit Message: Copy-pasting is slow.

Edit an old send message.
Copy following to clipboard.

ASDSADSA
DA😆 🤫 😙 
SD
ASDSA
D
ASDAS
DSA
DSA
DAS
D
AS
D


😋 🤫 😋 😋 😋 
SADSA
D
ASD
SA
D
SADS
AD
ASDAS
DASASDSADSA
DA😆 🤫 😙 
SD
ASDSA
D
ASDAS
DSA
DSA
DAS
D
AS
D


😋 🤫 😋 😋 😋 
SADSA
D
ASD
SA
D
SADS
AD
ASDAS
DAS

Replace the content on the edit message input with the copied content.
Select everything on the edit composer.
Paste via shortcuts.
Notice that it takes a few secs to replace.

screen-2023-01-09_16.19.55.mp4

Cursor jumping or moving positions after picking emoji.

Edit a message or draft a long message on the composer. (Use the content from the previous report).
Place the cursor either before the center or after the center line.
Now pick an emoji.
Notice that the cursor does not remain in the same position even though the previous cursor position was in the view.

screen-2023-01-09_16.26.09.mp4

Compose text is not cleared after sending the message

bug1-web.mov

s77rt · 2023-04-21T15:56:26Z

@hannojg That's a LIST. By the time we get to finish testing RN would have fixed the bug and Safari would be awesome 😂.

s77rt · 2023-04-21T16:00:21Z

@hannojg

I am by now quite experiences with the weird bugs that can happen when using SetSelection

Can you please highlight some of those? or you are referring to those in the list?

mountiny

Thanks for the changes @s77rt and @mollfpr would you be able to go through all those tests?

The job price is already at $2000, if you think we should increase the reward for this work, let me know

mountiny · 2023-04-21T23:11:31Z

Also there are some merge conflicts @s77rt

mollfpr · 2023-04-21T23:15:36Z

Thanks @mountiny! I’ll start testing after the conflict resolved.

parasharrajat · 2023-04-22T00:42:55Z

We should test all the bugs from the list over this PR. I tried to capture as much as I could.

s77rt · 2023-04-22T08:57:31Z

Resolved conflicts. Should be ready for test/review.

hannojg · 2023-04-25T13:51:49Z

I am just loosely going through a few items of the list, @parasharrajat will probably do a full audit.
What's following is just a few bugs I have noticed

iOS: Enter multiline text, set the cursor to the top position, scroll down, insert emoji. Bug: text input doesn't scroll up

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-04-25.at.15.49.37.mp4

hannojg · 2023-04-25T14:01:36Z

iOS: Inserting an emoji by using the : moves the text input's content:

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-04-25.at.15.59.28.mp4

hannojg · 2023-04-25T14:04:13Z

Android: Selection is not reset after inserting an emoji

Screen.Recording.2023-04-25.at.16.02.46.mov

s77rt · 2023-04-25T18:31:54Z

@hannojg Those bugs does not occur with traditional selection prop right? If so then we should probably fix the controlled selection bug instead

mountiny · 2023-04-25T18:46:44Z

@mollfpr will you have time to go through the testing?

Whats the plan @s77rt regarding what Hanno pointed out?

s77rt · 2023-04-25T19:56:13Z

@mountiny I was hoping that setSelection will magically work regression-free 😅. I have no idea how to fix the reported bugs on E/App, it's probably something that must escalates to RN. And if it's the case we should just fix the buggy selection prop. That bug is about 2-3 years so it may not be that straightforward.

@hannojg Do you have some comments on those bugs? Or what would you suggest in this case?

mountiny · 2023-04-25T20:00:30Z

I see, I guess if we can clearly define some problem/ solution for that buggy selection issue, we could get some bigger job running 8k+ and someone will have to fix this in RN eventually :D

parasharrajat · 2023-04-26T01:57:52Z

For now, we can close this PR and hold the issue on the main issue. We have been working on the main issue for quite sometime.

mollfpr · 2023-04-26T04:38:38Z

Selection is not reset after selecting an emoji
❌ Android
✅ iOS
✅ mWeb/Chrome
✅ mWeb/Safari
✅ Safari
✅ Chrome
✅ Firefox
✅ Desktop

Broken emojis are rendered when picked from the picker in a series
✅ Android
✅ iOS
✅ mWeb/Chrome
✅ mWeb/Safari
✅ Safari
✅ Chrome
✅ Firefox
✅ Desktop

Cursor position is broken
✅ Android
✅ iOS
✅ mWeb/Chrome
✅ mWeb/Safari
✅ Safari
✅ Chrome
✅ Firefox
✅ Desktop

Adding emoji via EmojiPicker in replacement for multiple lines creates a delay before the composer height is calculated.
✅ Android
✅ iOS
✅ mWeb/Chrome
✅ mWeb/Safari
✅ Safari
✅ Chrome
✅ Firefox
✅ Desktop

Selected Emoji is placed in the wrong position.
✅ Android
❌ iOS
✅ mWeb/Safari
✅ mWeb/Chrome
✅ Safari
✅ Chrome
✅ Firefox
✅ Desktop

Pasting image does not send the attached text message.
✅ mWeb/Safari
✅ mWeb/Chrome
✅ Safari
✅ Chrome
✅ Firefox
✅ Desktop

There is a delay before Composer expands the height on the web.
✅ mWeb/Safari
✅ mWeb/Chrome
✅ Safari
✅ Chrome
✅ Firefox
✅ Desktop

Cursor position is not focused after picking a emoji in Composer.
✅ mWeb/Safar
✅ mWeb/Chrome
✅ Safari
✅ Chrome
✅ Firefox
✅ Desktop

Content flashing while pasting.
✅ mWeb/Safari
✅ mWeb/Chrome
✅ Safari
✅ Chrome
✅ Firefox
✅ Desktop

Edit Message: Copy-pasting is slow.
✅ mWeb/Safari
✅ mWeb/Chrome
❌ Safari - is not slower than the video step but slower than Chrome and Firefox
✅ Chrome
✅ Firefox
❌ Desktop - the same speed as Safari

Cursor jumping or moving positions after picking emoji.
✅ mWeb/Safari
✅ mWeb/Chrome
✅ Safari
✅ Chrome
✅ Firefox
✅ Desktop

Compose text is not cleared after sending the message
✅ mWeb/Safari
✅ mWeb/Chrome
✅ Safari
✅ Chrome
✅ Firefox
✅ Desktop

mollfpr · 2023-04-26T04:39:35Z

Issue when executing Adding emoji via EmojiPicker in replacement for multiple lines creates a delay before the composer height is calculated.

Firefox - Adding emoji from the picker adding a bottom space inside the input.

Focus on the input and open the emoji picker
Select any emoji
Observed the input bottom

Video

Screen.Recording.2023-04-26.at.11.35.06.mov

mollfpr · 2023-04-26T07:10:34Z

Issue when executing Selected Emoji is placed in the wrong position.

Android - The selection selects the previous letter and will select the previous text

Type multiline text e.g.:

abcdefg abcdefg
abcdefg abcdefg

Select the letter cde from the second line
Open emoji picker and select an emoji
Notice that the selection highlights the letter b, and the emoji added
Do step 3 again
Notice that the letter b is removed, and now the selection highlights the previous a
Do step 3 again
Notice that the letter a is removed, and the selection highlights space in the first line
Do step 3 again
Notice that the selection randomly highlights the letter of the second word from the first line with the emoji added

Video

Screen_Recording_20230426_150138_New.Expensify.mp4

Note: It's only reproduce when select 3 or more letter

s77rt · 2023-04-26T11:58:03Z

@parasharrajat

For now, we can close this PR and hold the issue on the main issue. We have been working on the main issue for quite sometime.

I think the other issue is also following the setSelection approach.

@mollfpr Thanks for testing! The results are not that bad 😅 but Android selection issue is so concerning.

Can you please add a video for falling iOS - Selected Emoji is placed in the wrong position. That issue should be fixed, it would be strange if it's still reproducible.

mollfpr · 2023-04-26T13:42:47Z

@s77rt

Simulator.Screen.Recording.-.iPhone.14.-.2023-04-26.at.20.42.18.mp4

parasharrajat · 2023-04-26T15:12:35Z

I think the other issue is also following the setSelection approach.

Yes. Based on your last comment, I thought we don't have a clear solution for moving forward. I suggested this as we have done extensive research and testing on the other issue. We are even exploring an upstream solution. Unless we have a different approach, we should not replicate the effort.

s77rt · 2023-04-26T15:38:02Z

@mollfpr Ah this requires a selection range. Unfortunately both Android and iOS are buggy using selection range.

s77rt · 2023-04-26T15:40:09Z

@parasharrajat Yes sadly we don't have a clear solution. I believe the other issue is only for Android. I think we should do what @mountiny proposed, raise a new clear P/S statement in an attempt to fix the buggy selection prop on RN for both Android and iOS.

parasharrajat · 2023-04-26T15:53:05Z

The issue title is misleading. But all the issues are tested and perfected on each platform before we merge PRs. So I tested that PR on all platforms and make sure that it works. This pushed us to get a common solution for all platforms.

raise a new clear P/S statement in an attempt to fix the buggy selection prop on RN for both Android and iOS.

Sounds good.

mountiny · 2023-04-26T21:20:05Z

There is not that many fails, but lets go with fixing this upstream. Anyone is willing to take this on? I will close this PR after

s77rt · 2023-04-27T08:22:31Z

@mountiny I will but can I get a clarification on the plan, is it:

Create 2 E/App issues (one for Android and other for iOS)
Wait for proposals
Fix the bugs

Or we will be taking this to RN first?

hannojg · 2023-04-27T14:14:14Z

Hey, Hanno here from Margelo 👋 I am here to help find a solid fix for this issue, also in hindsight of my other PR #11684.

I am ready to make a proposal in some appropriate place, that will handle all the issues discussed.

I just want to note that the proposal likely won't be a upstream fix, as I figured that some of the problems are simply not fixable upstream (there exists no solution that is working generically enough, to suite all use cases with inputs in react native). My proposal would very likely include creating an own native component for TextInput on android (which will extend/inherit the existing react native's text input).
I just want to point out that if you set as a hard requirement that it should be fixed upstream, you are very likely to deadlock these issues, as -again - its not really possible to fix it upstream to serve all our use cases.

mountiny · 2023-04-27T19:07:30Z

Alright, I think we could go deep haha but what I suggest then @hannojg is to create a P/S in the #expensify-open-source channel to explain what problems we are solving (this should be quite straightforward we have real bugs right here haha) and then in the threads also please dont forget to mention why its not fixable upstream ideally with backing of some exisitng tries from other people. I am sure some people will come and ask "are you sure" so adding osmething like this will help with it.

Would @s77rt and @hannojg be interested in working on this a bit together? Not sure how you like to work but there could be help with debugging nad testing for sure :) That do you think?

s77rt · 2023-04-27T21:46:50Z

@hannojg I wouldn't be too surprised if the upstream fix is a lot of work but this was actually working on RN, so maybe we can just drop the "features" that broke it?

Also please note that we are looking to fix the buggy selection on iOS too (which would also require upstream changes)

I'm interested on working on this as @mountiny suggested, let me know what do you think

mountiny · 2023-04-27T21:56:59Z

Can we move this discussion to Slack? I think that would be easier (the problem solution statement can be nailed later, I will close this Pr for now)

Make Composer no longer selection controlled

c221cdf

s77rt requested a review from a team as a code owner April 19, 2023 21:11

melvin-bot bot requested review from MariaHCD and mollfpr and removed request for a team April 19, 2023 21:11

mountiny assigned s77rt Apr 19, 2023

mountiny self-requested a review April 19, 2023 21:32

parasharrajat reviewed Apr 20, 2023

View reviewed changes

src/components/Composer/index.android.js Show resolved Hide resolved

Composer: merged index.ios.js and index.android.js into index.native.js

137e8fc

mountiny reviewed Apr 21, 2023

View reviewed changes

Merge branch 'main' into composer-set-selection

9c5978c

eVoloshchak mentioned this pull request Apr 23, 2023

[$1000] IOS - IOUAmount - When entering/deleting numbers quickly, the cursor doubles #15856

Closed

6 tasks

s77rt mentioned this pull request Apr 24, 2023

[HOLD for payment 2023-07-14] [$4000] Text selection options disappears abruptly #16696

Closed

6 tasks

Merge branch 'main' into composer-set-selection

908b7ad

mountiny closed this Apr 27, 2023

Conversation

s77rt commented Apr 19, 2023

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

MelvinBot commented Apr 19, 2023

Uh oh!

Uh oh!

hannojg commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Selection is not reset after selecting an emoji

Broken emojis are rendered when picked from the picker in a series

Cursor position is broken

Adding emoji via EmojiPicker in replacement for multiple lines creates a delay before the composer height is calculated.

Selected Emoji is placed in the wrong position.

Pasting image does not send the attached text message.

There is a delay before Composer expands the height on the web.

Cursor position is not focused after picking a emoji in Composer.

Content flashing while pasting

Edit Message: Copy-pasting is slow.

Cursor jumping or moving positions after picking emoji.

Compose text is not cleared after sending the message

Uh oh!

s77rt commented Apr 21, 2023

Uh oh!

s77rt commented Apr 21, 2023

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

mountiny commented Apr 21, 2023

Uh oh!

mollfpr commented Apr 21, 2023

Uh oh!

parasharrajat commented Apr 22, 2023

Uh oh!

s77rt commented Apr 22, 2023

Uh oh!

hannojg commented Apr 25, 2023

Uh oh!

hannojg commented Apr 25, 2023

Uh oh!

hannojg commented Apr 25, 2023

Uh oh!

s77rt commented Apr 25, 2023

Uh oh!

mountiny commented Apr 25, 2023

Uh oh!

s77rt commented Apr 25, 2023

Uh oh!

mountiny commented Apr 25, 2023

Uh oh!

parasharrajat commented Apr 26, 2023

Uh oh!

mollfpr commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mollfpr commented Apr 26, 2023

Uh oh!

mollfpr commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s77rt commented Apr 26, 2023

Uh oh!

mollfpr commented Apr 26, 2023

Uh oh!

parasharrajat commented Apr 26, 2023

Uh oh!

s77rt commented Apr 26, 2023

Uh oh!

s77rt commented Apr 26, 2023

Uh oh!

parasharrajat commented Apr 26, 2023

Uh oh!

hannojg commented Apr 21, 2023 •

edited

Loading

mollfpr commented Apr 26, 2023 •

edited

Loading

mollfpr commented Apr 26, 2023 •

edited

Loading