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.

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

[HOLD on #3381] Selecting, copying, and pasting text on web and desktop are weird and in inverted order #1341

Closed
puneetlath opened this issue Jan 27, 2021 · 24 comments
Assignees
Labels
Planning Changes still in the thought process Weekly KSv2

Comments

@puneetlath
Copy link
Contributor

puneetlath commented Jan 27, 2021

If you haven’t already, check out our contributing guidelines for onboarding!


Upwork job: https://www.upwork.com/jobs/~0195a298f26a002c08

NOTE: We are only looking for proposals on fixing this directly in https://github.com/facebook/react-native. Example issue addressing this accessibility issue facebook/react-native#30373
We are not looking for a work-around in our repo

Platform - version:
Web and Desktop.

Action Performed (reproducible steps):
Attempt to select text across multiple chat inputs. You will find that it is difficult to select text across multiple chat inputs. Then, once you have managed to select multiple lines of text, try pasting that text. You will find that the order of the text has been inverted.

Expected Result:
That it would be easy to select text across chat inputs and that when copying and pasting the text it would retain itss order.

Actual Result:
The text is difficult to select and pastes in inverted order.

Notes/Photos/Videos:
image

image

We use Flatlist for rendering the list of chat messages. We use the inverted prop which reverses the scroll direction on mobile. We then use react-native-web for web and desktop. The ideal solution would fix this upstream, either in React Native or react-native-web. You can see an example of the problem here: https://codesandbox.io/s/cold-wave-grzpj?file=/src/App.js

@parasharrajat
Copy link
Member

So I did some digging. The issue is that inverted Flatlist uses transform:scale(-1) for inversion but the Browser selection works based on the order of dom elements. Even if we change the Flatlist code to use flex-direction:column-reverse the problem will remain same as the order of dom element is still reversed.

@kadiealexander kadiealexander added this to Awaiting Proposal in Issue Management Feb 9, 2021
@puneetlath
Copy link
Contributor Author

@parasharrajat can you think of a way FlatList could implement this functionality that wouldn't require inverting the order of the dom elements?

@puneetlath
Copy link
Contributor Author

@anthony-hull pointed out that this also creates an accessibility issue: facebook/react-native#30373

@parasharrajat
Copy link
Member

@puneetlath This issue is not with the FlatList. It is just a wrapper but more of the VirtualizedList and ScrollView underneath. And this is not really an accessibility issue. FlatList is made for the purpose of mobile devices so inversion logic is correct. Why would they create two-dimensional renderings if they do not need to? So they are not fixing this issue. It's been there for years now.

Yeah. I was suggesting using a non-inverted FlatList. I have a plan but first I need to give it a go. I think it is possible to show the chats with the non-inverted list. I have found some supporting evidence for my plan. I will soon let you in few hours.

@marcaaron
Copy link
Contributor

Gonna add this context here since @joaniew brought to our attention another bug related to this. I have a strong suspicion it is related to the FlatList CSS issues (if not the same exact issue) so whenever we fix this we should make sure to check whether this is resolved as well.

When copying text from Expensify.cash and pasting into gmail we see this:

screen_shot_2021-02-24_at_4 42 30_pm

@mallenexpensify
Copy link
Contributor

cc @tgolen so you can follow along

@mallenexpensify mallenexpensify self-assigned this Mar 26, 2021
@henrymoulton
Copy link

henrymoulton commented Mar 30, 2021

Howdy, I'm a React Native contractor/freelancer currently at Amazon, looking to do a few small fixed price things in my spare time. This might end up being too deep of a rabbit hole for me to work on, so I apologise in advance if that's the case.

I think this format of open source core + contract work to solve issues is super cool.

I looked through the issues and source code of the react-native-web FlatList/VirtualizedList.

My current conclusion is that the approach to inversion inside React Native is pretty broken, affecting scroll + text highlight + A11y.

Like @parasharrajat says:

I was suggesting using a non-inverted FlatList.

I have a screenshot showing that below, but appreciate the outcome you probably want is a React Native fork which would change the inverted implementation?

image

@mallenexpensify
Copy link
Contributor

Thanks for taking a look @henrymoulton

the outcome you probably want is a React Native fork which would change the inverted implementation?

Yes, we're hoping for a React Native fork so that we can fix the issue for ourselves as well as everyone else!
Do you think this is something you'd be able to invest the time in to fix?
If not, we have many other smaller jobs on Upwork - link.

@iwiznia
Copy link
Contributor

iwiznia commented Mar 30, 2021

My current conclusion is that the approach to inversion inside React Native is pretty broken, affecting scroll + text highlight + A11y.

heh, that were my impressions too when I looked at this 😄

To clarify, ideally the fix would be merged into the react native code base. So it would be good to also check with the maintainers that the proposed solution is good for them (once we review it here).

@ahmedjamshed001
Copy link

This issue can be resolved by using modified FlatList component here

https://codesandbox.io/s/musing-rubin-tti3o

@mallenexpensify
Copy link
Contributor

Hi @ahmedjamshed001 We're not looking to override FlatList functionality, we want flatlist itself changed. Can you propose a solution for that?

@nbgspl-github
Copy link

Hi

I looked through the issues and source code of the react-native-web FlatList/VirtualizedList.

My current conclusion is that inversion inside React Native is pretty broken, affecting scroll + text highlight + A11y.

As few people suggested above I agreed suggesting using a non-inverted FlatList.

Thanks
Sumit NBGSPL

@mallenexpensify
Copy link
Contributor

@puneetlath Nirav in Upwork proposed

I have reverse order of data instead of element
I had to do change on the other three screens as I have changed the data order in the element
that change is
removed inverted from component and changed y position

Screen.Recording.2021-04-08.at.11.14.24.AM.mov

Can you provide some feedback to share with Nirav?

@puneetlath
Copy link
Contributor Author

Hm, their solution doesn't look like it solves the underlying FlatList issue, which is the approach we'd like. It looks like they are just suggesting a workaround.

@marcaaron
Copy link
Contributor

Based on the discussions in Slack, how do we feel about explicitly removing this requirement:

The ideal solution would fix this upstream, either in React Native or react-native-web

Yes, the ideal solution should theoretically be something we could merge into the main react-native codebase (not react-native-web as the problem is not with this package). However, we should be clear that that the solution doesn't need to be merged into react-native in order to be considered "done". It just needs to be:

  • A modified version of FlatList (in it's current state) where the inverted prop works correctly and continues to work when it's not inverted
  • Ideally this version would be offered as a self-contained module so that we can use it alongside react-native and not maintain a fork of the entire react-native package

Thoughts?

@tgolen
Copy link
Contributor

tgolen commented Apr 15, 2021 via email

@tgolen
Copy link
Contributor

tgolen commented Apr 15, 2021

Ah, sorry... I read your comment wrong. I see that you said that we don't want to maintain any forks 👍

@tgolen
Copy link
Contributor

tgolen commented Apr 15, 2021

I guess that sounds like an alternative that we might reach at some point, but I feel like we haven't quite exhausted all the effort to get this into RN either... @puneetlath Maybe you should take your issue (facebook/react-native#30383) and start contacting their twitter feed or their discord channel about it? It would at least be good to hear from a maintainer what the feasibility of getting this fixed is.

@iwiznia
Copy link
Contributor

iwiznia commented Apr 15, 2021

Yeah, let's try that. We should also clarify that we are willing to sponsor with money and/or engineering time the implementation of the solution.

@tgolen
Copy link
Contributor

tgolen commented Apr 15, 2021

Yeah, perhaps we can work on this messaging a little bit too so that it looks a little more legit (cc @mallenexpensify ):

image

Something like:

Hey @teod, thanks for the comments above. That makes sense about the scroll direction. I'd really like to get this fixed and I was curious if you'd be interested in working on this for me? Of course, I would reimburse you for your time and effort. If you're interested, I've posted a job to UpWork that you can grab.

@mallenexpensify
Copy link
Contributor

Yeah, my message is pretty blunt. I can def drop in another post of someone else can. I'm going to let this marinate til tomorrow before doing anything

@mallenexpensify mallenexpensify added the Weekly KSv2 label May 2, 2021
@mallenexpensify
Copy link
Contributor

mallenexpensify commented May 2, 2021

Still need to work on this, added Weekly so I don't forget. Comment linking and this GH made me think of it - https://github.com/Expensify/Expensify/issues/147480#issuecomment-827044976

@mallenexpensify mallenexpensify changed the title Selecting, copying, and pasting text on web and desktop are weird and in inverted order [HOLD on #3381] Selecting, copying, and pasting text on web and desktop are weird and in inverted order Jun 4, 2021
@mallenexpensify mallenexpensify added Planning Changes still in the thought process and removed Exported labels Jun 4, 2021
@mallenexpensify
Copy link
Contributor

Created a new, clean issue for this with the deliverable being

The deliverable is to fix the core issue. The PR should be submitted against our RN fork (Expensify/react-native).

View it here - #3381
I'm putting this issue on hold. Once 3381 is finished the plan is to

  • Implement it to fix our issue
  • Try to get RN to merge the fork

@mallenexpensify
Copy link
Contributor

Closing this, once we're able to fix #3381 we can create a new issue with/for next steps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Planning Changes still in the thought process Weekly KSv2
Projects
No open projects
Issue Management
Awaiting Proposal
Development

No branches or pull requests

9 participants