Fix Invertedprop on the FlatListcomponent with a RN Patch

[mallenexpensify]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Expected Result:

• When copying text in e.cash that when you highlight lines of text, including across multiple messages, that the text is highlighted in order from top to bottom or bottom to top.
• When pasting text that has been copied in e.cash, that it shows up in the correct order.

Actual Result:

• Copying is sporadic and 'jumps around' when you highlight text over multiple messages. View video example at https://www.screencast.com/t/AzFBzTDs3ESm
  

• Pasting - When pasting copied text from multiple e.cash messages, it's out of order

Action Performed:

• Open Web or Desktop
• Create multiple messages in a test account (ie. 1 , click enter, 2, click enter, etc)
• Attempt to copy the multiple messages, observe you can't easily copy them in the correct order.
• Once you've been able to capture some of the multiple messages, paste in the compose box
• Observe the messages paste out of order.

Workaround:

The work around is to copy single messages at a time.

Platform:

Where is this issue occurring?

Web ✅
iOS
Android
Desktop App ✅
Mobile Web

Version Number: Version 1.0.62 (the issue has been happening since we launched)
Logs: N/A
Notes/Photos/Videos:
The deliverable is to fix the core issue (Copying and Pasting on Web and Desktop are weird and out of order). The PR should be submitted against our RN fork (Expensify/react-native).
A lot of discussion about this issue is here for additional context #1341

Expensify/Expensify Issue URL: From months ago https://github.com/Expensify/Expensify/issues/142340#

View all open jobs on Upwork

[MelvinBot]

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

[MelvinBot]

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

[mallenexpensify]

@thienlnam we plan to have a contributor work on this, can you review to ensure I didn't miss anything then add the External label? Thanks

[MelvinBot]

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

[thienlnam]

@mallenexpensify Sorry, was OOO but issue looks good and we can have a contributor start working on it. Nice detailed write-up!

[mallenexpensify]

Posted at $2000 https://www.upwork.com/jobs/~01953cb695d659063b
Raised to $4000 and created a new job https://www.upwork.com/jobs/~014c8ddadb545a60f4
Raised to $8000 and updated title to React Native - Fix Inverted prop on the FlatList component with a RN Patch #3381 Expensify. https://www.upwork.com/jobs/~01a4a59d0c8d74eae5

[FredericoGauz]

Hello,

I was checking this issue in the web version of the expensify.cash and noticed that the chat messages are deeply nested in divs. My first action towards fixing this issue would be trying to streamline this chat to make sure there are less areas of conflit between the messages.

However, if I also remember it right, it is an expensify policy not to have different code for web/android/ios etc. Is that still the case? Since I can imagine the reason for this complexity might be special needs of the mobile platforms.

[Viacheslav80]

Hi! it seems that the problem is in prop inverted of FlatList.
This prop used transform: scaleX: -1

[thienlnam]

@FredericoGauz

My first action towards fixing this issue would be trying to streamline this chat to make sure there are less areas of conflit between the messages.

Can you clarify what you mean by this? What are "areas of conflict" between the messages?

However, if I also remember it right, it is an expensify policy not to have different code for web/android/ios etc. Is that still the case?

We allow different code for platform specific changes, but try to dry it up so only the part that needs to be platform specific is separated out and the changes are in different platform specific file extensions. Feel free to read more information on it here

[thienlnam]

@Viacheslav80

Hi! it seems that the problem is in prop inverted of FlatList.
This prop used transform: scaleX: -1

I'm pretty sure that this is to display the chat messages in inverse order - can you verify that this doesn't change the functionality of the chat messages and doesn't break anything else?

[Viacheslav80]

@thienlnam I agree with you. this prop is the easiest way to do infinite scrolling with the addition of an element at the bottom

[mallenexpensify]

Raised to $4000 and created a new job https://www.upwork.com/jobs/~014c8ddadb545a60f4

[dklymenk]

@Viacheslav80 Is right, the prop inverted of Flatlist applies scaleY: -1 (specifically Y and not X), this causes a problem similar to that of flexbox's -reverse or order attribute known as html order vs css order. This is mainly a concern for website's accessibly - screen readers and keyboard navigation will follow the html order and not visual css one. However, it also breaks the mouse selection and copy-paste order (the actual issue in question). See my little fiddle for a simple demo. Try to select and copy-paste and you will the exact thing reported in the OP.

Proposal

To solve the issue I propose to avoid usage of Flatlist's inverted prop and pass the array of messages to Flatlist in their actual order (older first, newest last). I checked how other chat apps are doing it and they are not inverting the order of messages either. See for yourself, inspect the last message in slack or mattermost web app, and you'll see that it's the last block in it's parent div.

As a quick demo I removed the inverted prop in BaseInvertedFlatList.js as well as reversed the data array: data={this.props.data.reverse()}, also I had to remove the scaleY(-1) here and I got this result:

3381-demo.mp4

Now there is obviously more to it:

1. Make sure that chat is scrolled all the way to the bottom by default.
2. Remove reverse mouse wheel scrolling
3. Try to benchmark the impact of this.props.data.reverse(). If it's too much I'll need to find another way to do it.
4. Make sure the messages are bundled together in a correct way (when one person sends multiple messages, only the first message should show the avatar and date).
5. Think about android and iOS, maybe it's worth it to ditch the usage of inverted list there too. Should help with accessibility if that's a concern.

There might be more todos. These are just the ones that are on my mind right now.

Anyway, I believe that since it is a common practice to not invert the message list visually in chat apps, I shouldn't run into any pitfalls. If anything, I will always be able to take a peek at mattermost source code (an open-source self-hosted slack alternative).

[thienlnam]

Thank you both for the very detailed proposals!

@dklymenk I believe we ran into performance issues when reversing the props like that before but now that pagination exists with chats that might be okay but you may need to deal with some changes related to that

It looks like @ghorbani-m's proposal does something similar with changing it back to a regular FlatList and inverting the chats virtually before they are rendered.

Before we accept any proposal here, I'm finding that these proposals would change a fundamental way the chats work right now and want to loop in @kidroca / @roryabraham / @marcaaron who have been looking into bi-directional scrolling and get their thoughts on this

[marcaaron]

Based on the issue description we are not trying to do any workarounds (past solutions all look like workaround and none have mentioned a RN fork) please see:

The deliverable is to fix the core issue (Copying and Pasting on Web and Desktop are weird and out of order). The PR should be submitted against our RN fork (Expensify/react-native).

[dklymenk]

The deliverable is to fix the core issue (Copying and Pasting on Web and Desktop are weird and out of order). The PR should be submitted against our RN fork (Expensify/react-native).

Actually, I have completely missed that part. Sorry for that. However, even though I wouldn't call it a bug in react-native, what we can do is change how the inverted prop of Flatlist is implemented in your fork, if that's what you would prefer.

To accomplish that we remove all the scaleY stuff, and just reverse the array order in react-native Flatlist implementation. Ultimately I don't think it matters where we reverse it (in your fork or in Expensify.cash), there will, however, be some cleanup required in Expensify.cash anyway, as I have mentioned in my proposal.

I actually tried the code suggested by @ghorbani-m and it doesn't seem to work at all. I get a bunch of errors in the console and a blank white screen. Not sure what's up with that.

Also, I have to mention that messages stored in LocalStorage are already sorted by sequence number in ascending order, and then a reverse is applied to them in ReportActionsView.js. Additionally, I tried to apply my fix (removing inverted prop and scaleY) without doing an additional reverse and instead I have removed the one in ReportActionsView.js and the fix still worked.

So regardless of your priorities I can do the job, but personally I wouldn't touch the react-native code and just fix the code you have in FE repo.

[marcaaron]

Thanks, our priority and the deliverable for this issue is to fix the react native code in our fork. If anyone is interested in doing that job (and not a workaround) then please provide a clear proposal for what changes you would make.

[azimgd]

@thienlnam could you create a new branch at expensify/react-native-web which has version 0.15.7 from this commit Expensify/react-native-web@9d6d39f please ?

[thienlnam]

Looks like our fork already has version 0.15.7 https://github.com/Expensify/react-native-web/blob/master/package.json

[thienlnam]

@azimgd Any update on those fixes?

[azimgd]

@thienlnam https://github.com/Expensify/react-native-web/blob/master/package.json#L3

[azimgd]

To sumup:

• I was able to get correct behaviour on desktop FF, Chrome, Opera, Safari
• Fixed issue with blank screen on mobile safari
• Fixed list with small amount of messages sticks to up (instead of down)

There is a new bug tho: when a very long inverted-long list (consiting of one liners) is scrolled up until end is reached, list will re-render itself from top to bottom (only happens on ios mobile).

Some buggy behaviour with long lists on react-native-web:

• https://codesandbox.io/s/quizzical-silence-qmgvp?file=/src/App.js (simple scroll is on large list is broken)

I will push update version for the future reference, but I'm not sure this will pass the QA.

[azimgd]

@thienlnam

• Fix flatlist on Safari iOS 15 react-native-web#3
• Fix inverted list by applying forked version of react-native-web #6738

[thienlnam]

Added some comments to the react-native-web PR

[azimgd]

@thienlnam replied.

[alex-mechler]

Reverting the App pr because it created this deploy blocker #6792

[azimgd]

Fix: #6798

[azimgd]

Hey guys, all 3 PR's are deployed to production on Monday. Can we update the status of this ticket please.
@thienlnam @mallenexpensify

[mallenexpensify]

@azimgd Jack is out of of the office until Jan 3rd. I've pinged other engineers to see if they had time to take a look, I think many of them might also be out. I'll follow up and try to get more 👀/help on this issue next week.

Can you confirm the 3 PRs? I think one is below.
#6738

Standard payment timing is 7 days after PR is deployed to production, assuming there are no regressions.

[thienlnam]

Current status of this ticket:

After the first regression, Azimgd submitted a couple PRs which fixed the problems

• Fix flatlist on Safari iOS 15 react-native-web#3
• Fix inverted list by applying forked version of react-native-web #6738

We ran into another regression #6792 which Azimgd quickly fixed with #6798

As @mallenexpensify mentioned, our process is

Standard payment timing is 7 days after PR is deployed to production, assuming there are no regressions.

Since #6798 was the last PR to address the issue (deployed to production Dec 21) here the payment will be applied around Dec 28th assuming no other regressions occur

[thienlnam]

Update on the pricing of this ticket @mallenexpensify

Original ticket price: $16000
Company holds (N6, Offsite): $500
Bonus for scope increase (original investigation, IOS 15 updates): $4000

New ticket price: $20500

[quinthar]

Woohoo! And they said it couldn't be done!

[mallenexpensify]

Thanks for the thorough breakdown @thienlnam . Thanks for all the help @azimgd !
I added the payment to my calendar for the 28th (assuming no regressions). I'll update here once payment is issued.

[quinthar]

Hi there, just wanted to note a regression to copy/paste: https://expensify.slack.com/archives/C01GTK53T8Q/p1640337980101200

…

On Fri, Dec 24, 2021 at 12:35 AM Matt Allen ***@***.***> wrote: Thanks for the thorough breakdown @thienlnam <https://github.com/thienlnam> . Thanks for all the help @azimgd <https://github.com/azimgd> ! I added the payment to my calendar for the 28th (assuming no regressions). I'll update here once payment is issued. — Reply to this email directly, view it on GitHub <#3381 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEMNUSUJJJYQHFIJXEIIZ3USO537ANCNFSM46DJUEOQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[azimgd]

Hey David,
I was indeed able to reproduce your issue, but I wouldn't count that as a regression since this exact behavior existed in the older/pre-merge version of the app. Tested from this commit e230e7e

Can we actually create a new task for this one as it's related to formatting ?

[mallenexpensify]

Agree with @azimgd I think these are separate issues. Per Slack (link) We have this two existing issues:
#5142 Copying messages with markdown via CTRL + C does not keep formatting (moving along)
#5125 Markdown - Copied text (with markdown) does not show the formatting when pasted (waiting for comment on up-to-date plan, I think @parasharrajat is looking into that)

When/where needed, let's keep creating singular issue til we get all the copy/issues resolved

[mallenexpensify]

@azimgd paid $20,500 per Jack's breakdown above.

Thanks for seeing this one through @azimgd , it's been the largest external issue we've ever had, feels realllly good to close this.

[marcaaron]

cc @kidroca as we are discussing a possible regression here https://expensify.slack.com/archives/C01GTK53T8Q/p1641320742022500?thread_ts=1641315371.490700&cid=C01GTK53T8Q

[kidroca]

Added some more comments on slack to explain what's happening and why