Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Invertedprop on the FlatListcomponent with a RN Patch #3381

Closed
mallenexpensify opened this issue Jun 4, 2021 · 138 comments
Closed

Fix Invertedprop on the FlatListcomponent with a RN Patch #3381

mallenexpensify opened this issue Jun 4, 2021 · 138 comments
Assignees
@azimgd @mallenexpensify @thienlnam
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Jun 4, 2021 •
edited
Loading

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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
    image

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

image

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
@mallenexpensify mallenexpensify added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 4, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 4, 2021
@MelvinBot
Copy link

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

@mallenexpensify mallenexpensify added the Daily KSv2 label Jun 4, 2021
@mallenexpensify
Copy link
Contributor Author

@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

@mallenexpensify mallenexpensify mentioned this issue Jun 4, 2021
Closed
@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label Jun 7, 2021
@MelvinBot
Copy link

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

@thienlnam
Copy link
Contributor

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

@mallenexpensify
Copy link
Contributor Author

mallenexpensify commented Jun 10, 2021 •
edited
Loading

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
Copy link
Contributor

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
Copy link
Contributor

Viacheslav80 commented Jun 21, 2021 •
edited
Loading

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

@thienlnam
Copy link
Contributor

@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
Copy link
Contributor

thienlnam commented Jun 21, 2021 •
edited
Loading

@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
Copy link
Contributor

@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
Copy link
Contributor Author

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

@dklymenk
Copy link
Contributor

@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).

@ghorbani-m

This comment has been minimized.

Sign in to view
@thienlnam
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

azimgd commented Nov 24, 2021

@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
Copy link
Contributor

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

@thienlnam
Copy link
Contributor

@azimgd Any update on those fixes?

@MelvinBot MelvinBot removed the Overdue label Dec 7, 2021
@azimgd
Copy link
Contributor

azimgd commented Dec 8, 2021

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

@azimgd
Copy link
Contributor

azimgd commented Dec 8, 2021 •
edited
Loading

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:

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

@azimgd azimgd mentioned this issue Dec 13, 2021
Merged
5 tasks
@azimgd
Copy link
Contributor

azimgd commented Dec 13, 2021 •
edited
Loading

@thienlnam

@thienlnam
Copy link
Contributor

Added some comments to the react-native-web PR

@azimgd
Copy link
Contributor

azimgd commented Dec 14, 2021

@thienlnam replied.

@alex-mechler
Copy link
Contributor

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

@azimgd
Copy link
Contributor

azimgd commented Dec 16, 2021

Fix: #6798

@azimgd
Copy link
Contributor

azimgd commented Dec 22, 2021

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

@mallenexpensify
Copy link
Contributor Author

@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
Copy link
Contributor

thienlnam commented Dec 24, 2021 •
edited
Loading

Current status of this ticket:

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

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
Copy link
Contributor

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
Copy link
Contributor

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

@mallenexpensify
Copy link
Contributor Author

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
Copy link
Contributor

quinthar commented Dec 24, 2021 via email

@azimgd
Copy link
Contributor

azimgd commented Dec 24, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@kidroca
Copy link
Contributor

kidroca commented Jan 5, 2022 •
edited
Loading

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

@marcaaron marcaaron mentioned this issue Jan 6, 2022
Closed
@adelekennedy adelekennedy mentioned this issue Mar 3, 2022
Closed
@fabOnReact fabOnReact mentioned this issue Apr 27, 2023
Closed
@burczu burczu mentioned this issue May 11, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@azimgd azimgd

@mallenexpensify mallenexpensify

@thienlnam thienlnam

Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests