Fix `Inverted`prop on the `FlatList`component with a RN Patch

mallenexpensify commented 3 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!

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 https://github.com/Expensify/Expensify.cash/issues/1341

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

View all open jobs on Upwork

MelvinBot commented 3 years ago

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

MelvinBot commented 3 years ago

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

mallenexpensify commented 3 years ago

@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 commented 3 years ago

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

thienlnam commented 3 years ago

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

mallenexpensify commented 3 years ago

~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 commented 3 years ago

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 commented 3 years ago

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

thienlnam commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

dklymenk commented 3 years ago

@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:

https://user-images.githubusercontent.com/64093836/123218109-fe072080-d4d3-11eb-92e2-325393480133.mp4

Now there is obviously more to it:

Make sure that chat is scrolled all the way to the bottom by default.
Remove reverse mouse wheel scrolling
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.
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).
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 commented 3 years ago

Proposal

As we know the issue is the inverted prop in the FlatList component, so my solution is to implementing the inverted FlatList virtually in the BaseInvertedFlatList.js file.

I have changed the BaseInvertedFlatList component so that the inverted feature is implemented and we don't need to change the other part of the source code used the BaseInvertedFlatList component.

New BaseInvertedFlatList component

the renderItem method is reimplemented.

/* eslint-disable react/jsx-props-no-multi-spaces */
import _ from 'underscore';
import React, {forwardRef, Component} from 'react';
import PropTypes from 'prop-types';
import {FlatList, View} from 'react-native';
import {lastItem} from '../../libs/CollectionUtils';

const propTypes = {
    /** Same as FlatList can be any array of anything */
    data: PropTypes.arrayOf(PropTypes.any),

    /** Same as FlatList although we wrap it in a measuring helper before passing to the actual FlatList component */
    renderItem: PropTypes.func.isRequired,

    /** This must be set to the minimum size of one of the renderItem rows. Web experiences issues when inaccurate. */
    initialRowHeight: PropTypes.number.isRequired,

    /** Passed via forwardRef so we can access the FlatList ref */
    innerRef: PropTypes.oneOfType([
        PropTypes.func,
        PropTypes.shape({current: PropTypes.instanceOf(FlatList)}),
    ]).isRequired,

    /** Should we measure these items and call getItemLayout? */
    shouldMeasureItems: PropTypes.bool,

    /** Should we remove the clipped sub views? */
    shouldRemoveClippedSubviews: PropTypes.bool,
};

const defaultProps = {
    data: [],
    shouldMeasureItems: false,
    shouldRemoveClippedSubviews: false,
};

class BaseInvertedFlatList extends Component {
    constructor(props) {
        super(props);

        this.renderItem = this.renderItem.bind(this);
        this.getItemLayout = this.getItemLayout.bind(this);

        // Stores each item's computed height after it renders
        // once and is then referenced for the life of this component.
        // This is essential to getting FlatList inverted to work on web
        // and also enables more predictable scrolling on native platforms.
        this.sizeMap = {};
    }

    /**
     * Return default or previously cached height for
     * a renderItem row
     *
     * @param {*} data
     * @param {Number} index
     *
     * @return {Object}
     */
    getItemLayout(data, index) {
        const size = this.sizeMap[index];

        if (size) {
            return {
                length: size.length,
                offset: size.offset,
                index,
            };
        }

        // If we don't have a size yet means we haven't measured this
        // item yet. However, we can still calculate the offset by looking
        // at the last size we have recorded (if any)
        const lastMeasuredItem = lastItem(this.sizeMap);

        return {
            // We haven't measured this so we must return the minimum row height
            length: this.props.initialRowHeight,

            // Offset will either be based on the lastMeasuredItem or the index +
            // initialRowHeight since we can only assume that all previous items
            // have not yet been measured
            offset: _.isUndefined(lastMeasuredItem)
                ? this.props.initialRowHeight * index
                : lastMeasuredItem.offset + this.props.initialRowHeight,
            index,
        };
    }

    /**
     * Measure item and cache the returned length (a.k.a. height)
     *
     * @param {React.NativeSyntheticEvent} nativeEvent
     * @param {Number} index
     */
    measureItemLayout(nativeEvent, index) {
        const computedHeight = nativeEvent.layout.height;

        // We've already measured this item so we don't need to
        // measure it again.
        if (this.sizeMap[index]) {
            return;
        }

        const previousItem = this.sizeMap[index - 1] || {};

        // If there is no previousItem this can mean we haven't yet measured
        // the previous item or that we are at index 0 and there is no previousItem
        const previousLength = previousItem.length || 0;
        const previousOffset = previousItem.offset || 0;
        this.sizeMap[index] = {
            length: computedHeight,
            offset: previousLength + previousOffset,
        };
    }

    /**
     * Render item method wraps the prop renderItem to render in a
     * View component so we can attach an onLayout handler and
     * measure it when it renders.
     *
     * @param {Object} params
     * @param {Object} params.item
     * @param {Number} params.index
     *
     * @return {React.Component}
     */
    renderItem({item, index}) {
        const {data=[]}=this.props;
        const invertedIndex=data.length-index-1;
        const invertedItem=data[invertedIndex];
        if (this.props.shouldMeasureItems) {
            return (
                <View onLayout={({nativeEvent}) => this.measureItemLayout(nativeEvent, invertedIndex)}>
                    {this.props.renderItem({invertedItem, invertedIndex})}
                </View>
            );
        }
        return this.props.renderItem({invertedItem, invertedIndex});
    }

    render() {
        const {style}=this.props;
        return (
            <FlatList
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...this.props}
                style={[style,{justifyContent:'flex-end'}]}
                ref={this.props.innerRef}
                //inverted
                renderItem={this.renderItem}
                // Native platforms do not need to measure items and work fine without this.
                // Web requires that items be measured or else crazy things happen when scrolling.
                getItemLayout={this.props.shouldMeasureItems ? this.getItemLayout : undefined}
                bounces={false}

                // We keep this property very low so that chat switching remains fast
                maxToRenderPerBatch={1}
                windowSize={15}
                removeClippedSubviews={this.props.shouldRemoveClippedSubviews}
            />
        );
    }
}

BaseInvertedFlatList.propTypes = propTypes;
BaseInvertedFlatList.defaultProps = defaultProps;

export default forwardRef((props, ref) => (
    // eslint-disable-next-line react/jsx-props-no-spreading
    <BaseInvertedFlatList {...props} innerRef={ref} />
));

thienlnam commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

mallenexpensify commented 3 years ago

Pinged a few contributors in Upwork who submitted proposals there, asked them to post here.

MelvinBot commented 3 years ago

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

mallenexpensify commented 3 years ago

@jliexpensify added you as a buddy to keep eyes on the next week while I'm OOO, thanks

garudamon commented 3 years ago

Proposal

The problem caused by when we use inverted without reversing the style. And this proposal I change the style so the selection is not jumping, and the result of the copy was expected and not weird. You can check on the video I have attached.

Video

https://user-images.githubusercontent.com/42667275/124374633-40481300-dcc7-11eb-8b33-35a63aeced9c.mov

Solution

// styles.js
// below just code should be changes
const styles = {
  ...existingStyles,
  chatContentScrollView: {
    // we need to reverse the direction, to solve jumping selection
    flexDirection: 'column-reverse',
    paddingVertical: 16,
  }
}

// ReportActionsView.js
// below just code should be changes
class ReportActionsView extends React.Component {

  updateSortedReportActions(reportActions) {
    this.sortedReportActions = _.chain(reportActions)
      .sortBy('sequenceNumber')
      .filter((action) => {
        // Only show non-empty ADDCOMMENT actions or IOU actions
        // Empty ADDCOMMENT actions typically mean they have been deleted and should not be shown
        const message = _.first(lodashGet(action, 'message', null));
        const html = lodashGet(message, 'html', '');
        return (
          action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU ||
          (action.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT &&
            html !== '')
        );
      })
      .map((item, index) => ({action: item, index}))
      .value();

    // we need to remove .reverse, because I change to styles logic
    // and we got an advantage performance by removing .reverse()
  }

  isConsecutiveActionMadeByPreviousActor(actionIndex) {
    // previously const previousAction was use a "+", 
    // and we remove our logic .reverse on updateSortedReportActions()
    // so we need to chage to "-"
    const previousAction = this.sortedReportActions[actionIndex - 1];
    const currentAction = this.sortedReportActions[actionIndex];

    // It's OK for there to be no previous action, and in that case, false will be returned
    // so that the comment isn't grouped
    if (!currentAction || !previousAction) {
      return false;
    }

    // Comments are only grouped if they happen within 5 minutes of each other
    if (
      currentAction.action.timestamp - previousAction.action.timestamp >
      300
    ) {
      return false;
    }

    return currentAction.action.actorEmail === previousAction.action.actorEmail;
  }

}

parasharrajat commented 3 years ago

@garudamon

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

The fix is needed to be patched in RN.

garudamon commented 3 years ago

maybe this just my opinion, but patching the RN rather than fixing the logic will potentially trigger another issue without proper and consecutive testing in the future

mallenexpensify commented 3 years ago

@thienlnam can you comment on @garudamon 's proposal above?

thienlnam commented 3 years ago

@thienlnam can you comment on @garudamon 's proposal above?

It's not a PR that would patch the RN fork so not what we're looking for right now

Viacheslav80 commented 3 years ago

Hi! fixing the RN will not give a result! Webpack takes dependencies from react-native-web( FlatList and other ), not from RN. we need to fix this on the web, right?

thienlnam commented 3 years ago

react-native-web just uses the FlatList code from react-native https://github.com/necolas/react-native-web/blob/master/packages/react-native-web/src/vendor/react-native/FlatList/index.js It's bundled with react-native-web but the author's aren't modifying the code at all

thienlnam commented 3 years ago

Extra context here https://github.com/necolas/react-native-web/issues/1903#issuecomment-775464213

Viacheslav80 commented 3 years ago

@thienlnam Hi! what do you expect from patch RN? fix the inverted property? do something similar to bidirectional-infinite-scroll? I think that while the render will be from the bottom to the top, the selection will not be fixed. we need to move away from transform: scaleY: -1

thienlnam commented 3 years ago

@Viacheslav80 Yup, the expectation from the react-native patch is to fix the inverted prop. There was more discussion about this solution here. But the solution doesn't need to be merged into the main branch to be considered complete - it just needs to fix the inverted prop in FlatList and also continue to work when it is not inverted

mallenexpensify commented 3 years ago

Raised price to $8000, created a new Upwork job, and updated the title to React Native - Fix Inverted prop on the FlatList component with a RN Patch #3381 Expensify. https://www.upwork.com/jobs/~01a4a59d0c8d74eae5

dklymenk commented 3 years ago

Hello, I'm sorry but I have to ask again about the expected deliverable.

Yup, the expectation from the react-native patch is to fix the inverted prop. There was more discussion about this solution here. But the solution doesn't need to be merged into the main branch to be considered complete - it just needs to fix the inverted prop in FlatList and also continue to work when it is not inverted

The actual implementation for inverted prop is not in FlatList, but in VirtualizedList.

So if you want the deliverable to be a patch against react-native, I would assume it would be this file.

While those files are very similar, a diff of them is 513 lines long. Most of that diff is just different formatting and comment styles, but there are still some differences in logic. Even though I don't see any differences related to inverted prop, I'm expecting that the patch created for RN VirtualisedList won't apply to react-native-web VirtualisedList without fails/conflicts. So if someone were to create this patch for RN, it would still need to be manually applied for react-native-web by it's original developer. Or maybe you would create your own fork of react-native-web you can use in this app?

Also if the solution you request is an RN patch, how would it be verified on mobile? Via react dev tools to check the order of messages in component tree? I'm not sure there is a way to copy paste content on the screen of a phone, like you can with web.

So In light of the questions above my final question is: Is the deliverable just a patch for RN VirtualisedList or should it be 2 patches (1 for each library)?

Thanks.

shramee commented 3 years ago

Submitted a proposal on UpWork, will need to investigate the issue first before I can tell what the fix is going to be like.

thienlnam commented 3 years ago

@dklymenk Great questions! After taking a look at the files you linked it does look like the web implementation uses VirtualizedList which will be file that needs to be patched.

Also if the solution you request is an RN patch, how would it be verified on mobile? Via react dev tools to check the order of messages in component tree? I'm not sure there is a way to copy paste content on the screen of a phone, like you can with web.

We would just want to make sure that the FlatList still works as intended on mobile and the changes did not mess up its default functionality. Additionally, we'd probably test these changes on mobile web, web, and desktop since those are the platforms that these changes would apply to.

Is the deliverable just a patch for RN VirtualisedList or should it be 2 patches (1 for each library)?

Ideally, we'd have the both of the patches applied to the main branch but assuming that is not the case, the deliverable should be two patches, one for RN and another patch for react-native-web to use those patched changes.

thienlnam commented 3 years ago

@shramee Thanks for the interest! As part of the process of Working on Expensify Jobs, you'll have to propose a solution and post it in this Github issue. More details here

mallenexpensify commented 3 years ago

Spent a couple hours digging around, emailing some folks and posting in some repos/issues.

https://discord.com/channels/102860784329052160/103882387330457600/866765536410206239

adrien@harnay.me https://github.com/facebook/react-native/pull/31288

me@simek.dev Bartosz https://github.com/facebook/react-native/pull/31288

https://www.reddit.com/r/reactnative/ Checking with moderator before posting - https://www.reddit.com/user/cschultz1272/

https://github.com/jimmy623 https://github.com/kacieb

https://github.com/necolas/react-native-web/issues/1807#issuecomment-882900768 https://github.com/facebook/react-native/pull/23632#issuecomment-882901586

FredericoGauz commented 3 years ago

I spent sometime investigating the possible causes of this problem. From what I could understand this is a CSS bug where (possible by using the same implementation) many css-only ways of inverting elements on a list will create issues when selecting and copying those same elements.

https://jsfiddle.net/v09o1nr2/1/

I believe that a possible solution would be to investigate possible CSS only solutions (if they exist) and then apply it to the RN style. If we can achieve this, there wouldn't be much (or even any) change in the RN files themselves.

Another solution would be to implement a order-reversing algorithm in the JS itself, that would open perhaps a door for performance issues, but perhaps if it can be coupled with CSS and indexes it might not have any significant impact. In the end, like all other performance considerations, it would have to be measured.

A last solution would be to work toward a fix to the CSS it self. Unless I am wrong, it would demand a much larger scale change with browser patches and CSS standards compliance. If that was the case, I guess this would be totally out of the scope of this job, specially because it would depend of the concerned open source community.

FredericoGauz commented 3 years ago

A possible js-only solution would be something along the lines of:

in react-native/FlatList/index.js

` render(): React.Node { const {numColumns, columnWrapperStyle, ...restProps} = this.props; const invertedDataPatch = this.props.inverted ? this.props.data.reverse() : this.props.data

return (
  <VirtualizedList
    {...restProps}
    data={invertedDataPatch}
    getItem={this._getItem}
    getItemCount={this._getItemCount}
    keyExtractor={this._keyExtractor}
    ref={this._captureRef}
    viewabilityConfigCallbackPairs={this._virtualizedListPairs}
    {...this._renderer()}
  />
);

}`

Which would account for the data appearing inverted and with the removal of the styles that are related to it (like scaleY). It would still have to be checked in regards to the impact of the style removals (for the inverted prop), but at least it will be working and the copy and past works as expected. This is just an example of what could be done.

ghorbani-m commented 3 years ago

@thienlnam Based on my investigation I find out we just need a patch for react-native-web package because this package does not have a direct dependency on the react-native package. you can see the VirtualList implementation diffs here.

My approach is to override the getItem prop in the VirtualList component in the react-native-web package. Actually, this approach is done before for keyExtractor prop for the VirtualList component in the react-native package. Now we can override the getItem method prop like below and use it in the code, I have overridden the getItem somehow to invert the data order when the inverted prop is true, even with this approach we don't have any overload to invert big data.

_getItem = (data: any, index: number) => {
    const {
      inverted,
      getItem,
      getItemCount
    }=this. props;
    if(inverted){
      let invertedIndex=getItemCount(data)-index-1;
      let invertedItem= getItem(data,invertedIndex);
      if(Array.isArray(invertedItem))
        return invertedItem.reverse();
      return invertedItem;
    }
    return getItem(data,index);
  }

You can see the implementation changes here

thienlnam commented 3 years ago

@FredericoGauz Thanks for your investigation!

I believe that a possible solution would be to investigate possible CSS only solutions (if they exist) and then apply it to the RN style. If we can achieve this, there wouldn't be much (or even any) change in the RN files themselves.

I think this would probably be the most ideal solution and one that would probably be most likely to get merged into the main branch. If there is a bug with the CSS causing this odd behavior then it would be best to try to figure out why it is happening and how to fix it / how to update the css so that it is fixed

Another solution would be to implement a order-reversing algorithm in the JS itself, that would open perhaps a door for performance issues, but perhaps if it can be coupled with CSS and indexes it might not have any significant impact. In the end, like all other performance considerations, it would have to be measured.

Solving this in JS is definitely another solution, but I think we should try to solve the CSS issue before using this as a last resort. This would add extra performance overhead and likely take longer to be accepted since it is very different from the existing solution

A last solution would be to work toward a fix to the CSS it self. Unless I am wrong, it would demand a much larger scale change with browser patches and CSS standards compliance. If that was the case, I guess this would be totally out of the scope of this job, specially because it would depend of the concerned open source community.

I'm not totally sure what you mean by this - are you talking about overhauling the CSS functionality? I don't think we'd need to do that - ideally we would just be able to update the css so that it functions as expected

thienlnam commented 3 years ago

@ghorbani-m Hey Mati, thanks for your interest and time!

Yup, as you mentioned, the actual component that would be modified would be VirtualizedList which was talked about briefly https://github.com/Expensify/App/issues/3381#issuecomment-882752617. As I mentioned above, ideally we'd try to fix the CSS issue first

Solving this in JS is definitely another solution, but I think we should try to solve the CSS issue before using this as a last resort. This would add extra performance overhead and likely take longer to be accepted since it is very different from the existing solution

FredericoGauz commented 3 years ago

Hi @thienlnam,

What I meant by the css issue is the fact that this seems to be an issue within the css itself. As you can see in my fiddle:

https://jsfiddle.net/v09o1nr2/1/

It is very simple and you can copy the HTML and run directly in the browser. If you do that you can see that it has the same issues that you are having in RN.

What RN is doing is a double Y inversion CSS trick to have a visual inversion of the list without having to deal with changing any of the contents. This, however, creates issues with the copying and pasting and highlighting of the text content that was "inverted" through this. I believe it was probably done for performance reasons, but it may have other reasons as well.

I would also look forward to a css only solution but the first options that would come to mind (like using flex order and column-reverse would inherit the same issues. (What makes me think that they share a similar implementation) you can also see this behaviour in the fiddle.

I also shared a possible js only fix, where (if the goal would be the inversion of the data) we could apply it before going to the virtualized list and gave a single point of change by a simple .reverse() use. I think that any performance impact should be rather measured them assumed since it is a very straightforward approach.

However, both (and I believe that any) solutions would have necessarily to deal with the removal of the css property scaleY(-1) in the outer div container. This is specially important for long (big vertical height) containers by the way that the Y reflection works (you can think of it as getting an A4 piece of paper and turning it backward while holding its bottom and looking at it again a light. All front content will appear vertically inverted (as you are looking at it as if through a mirror) and most importantly, the position that the paper occupies will change (it will be exactly bellow the position where it was before). This will cause issues where the Flatlist component with the inverted prop was used taking into consideration this reflection.

This will take us to another design decision: should we "fix" this initially unintended reflexion behaviour (the vertical Y reflection) and then make the appropriate changes where the lack of it will cause issues because it was previously accounted for where it was used? Or should we leave it or recreate it somehow?

FredericoGauz commented 3 years ago

Solving this in JS is definitely another solution, but I think we should try to solve the CSS issue before using this as a last resort. This would add extra performance overhead and likely take longer to be accepted since it is very different from the existing solution

Yes, I was saying that one solution would be to fix the css itself as I don't think that this odd coping, pasting and highlight behavior when using things like flex order to invert divs blocks would be intended behaviour by the creators. It looks like a bug but I think that working toward fixing it in the CSS implementation itself would not be a reasonable thing to do within the scope of this issue. Nevertheless, it would definitely fix this odd Flexbox or at least allow us using Flexbox to do the same thing.

marcaaron commented 3 years ago

I just want to give this conversation a nudge in a different direction since I think there's been some good back and forth but we are sending some mixed signals.

working toward fixing it in the CSS implementation itself would not be a reasonable thing to do

Agree. Let's take this off the table. It seems like an unrealistic goal.

I also shared a possible js only fix, where (if the goal would be the inversion of the data) we could apply it before going to the virtualized list and gave a single point of change by a simple .reverse() use

Thanks for that, but not on the table and not what we are looking to do.

What RN is doing is a double Y inversion CSS trick to have a visual inversion of the list without having to deal with changing any of the contents.

Exactly! This unsatisfactory implementation of VirtualizedList is precisely what we are looking to improve. We are looking for someone to "deal with changing ... the contents" and making a true inverted virtualized list that:

Does not use CSS hacks
Looks like something that could get merged upstream into the RN codebase to improve the implementation of <FlatList inverted /> for everyone who uses it on web and needs accessibility or working copy/paste features.

So, maybe a better way to think about this problem would be...

Imagine that you can only modify the code in this file (and related files)... https://github.com/facebook/react-native/blob/main/Libraries/Lists/VirtualizedList.js

What could we change there to make it a true inverted list? Is it possible?

ghorbani-m commented 3 years ago

@marcaaron We could also apply my solution to the RN VirtualList component to improve the inverted feature and remove the CSS hacks.

Now we can override the getItem method prop like below and use it in the code, I have overridden the getItem somehow to invert the data order when the inverted prop is true, even with this approach we don't have any overload to invert big data.

Even though I don't think so changing the RN source code fixes the "Copying and Pasting on Web and Desktop" Issue!

FredericoGauz commented 3 years ago

Hi @marcaaron Thank you for the detailed reply!

I am looking for possible solutions, what would be a "true inverted list"? As far as I can see we are either looking at doing some reversing at the script level or trying to style it out via CSS. But from what I can see, all the solutions in CSS use either techniques that are not easy to be made general for such a tool like RN or they have the same flaws as the double inversion technique.

Since the whole goal of the inversion prop is to invert the display of the items think that we are a bit out of options unless the CSS implementation changes (what would be really great for all of us) or we use a temporary solution like data = { props.inverted ? data.slice().reverse() : data} and I would assume that the best place to do that would be before the VirtualizedList (since we are not really changing anything on it's implementation and just modifying the input data before displaying).

I totally recognize that this would be a rather unsatisfactory solution, specially because you are looking at a rendering component and not something that should play with the data (even if immutably) and what we would be implementing would be a shortcut of this js invertion via the "inverted" prop.

On regarding being accepted in the main RN trunk, from what I can see in different open source projects, (specially big ones) this is a much rather political matter then a technical one. Since people would be weighting many interests like: the goals, preferences and timescale restrictions of the main maintainers and sponsors, the main interests of the general users, and long term goals of the project.

Since this change (data reversal) is a fairly trivial change. I think that the reason why it was not changed was exactly because the lack of text highlighting (and its consequences in accessibility, copy and paste, etc) was not a big issue for them. And it is probably the same for the most of web users and developers because it comes from the CSS techniques themselves.

Any accessibility conscious developer would then have to skip the CSS only reversal and implement them in their preferred language instead. This brings me to last point, which is that (if styling only solutions for this issue are not available) you are restricted to a code solution. If that is right then making any changes to a highly used code framework means "forcing" its users to pay the performance cost (even for a slight thing like a .slice().reverse() operation) even they don't care about accessibility or if they just need a quick solution for a render only mirroring effect.

At the same time, all those that don't need would just have to write data={my_data.slice().reverse()} (or something similar) in their own code bases.

It's a shame, I am all for a better web for us all...

marcaaron commented 3 years ago

either looking at doing some reversing at the script level

I'm not sure we need to reverse anything. VirtualizedList basically uses fixed positions to place items + create whitespace offsets. So we would need to change the logic for positioning those items so it works in an inverted style.

Since people would be weighting many interests like: the goals, preferences and timescale restrictions of the main maintainers and sponsors, the main interests of the general users, and long term goals of the project.

Thanks for this feedback and perspective! We definitely understand that this won't be easy to get merged into the RN codebase for political or other reasons. But the deliverable is only to fix it against our own fork of react-native. There is no requirement to successfully get it merged into the main codebase as we can't guarantee that task is possible.

FredericoGauz commented 3 years ago

Thanks for the reply @marcaaron, I didn't understand what you meant by this:

I'm not sure we need to reverse anything. VirtualizedList basically uses fixed positions to place items + create whitespace offsets. So we would need to change the logic for positioning those items so it works in an inverted style.

From what I understand this component is "dumb" in regards to the data (and data ordering). It only renders the items in the ordering that it receives from the props.data, the inversion is purely "cosmetic" and, like you said, works on the style only.

I don't know what we could do code level to "fix" the style since this is a styling issue from the start.

This is something like: We want to have inverted items without inverting the items, so, in order to do that, we display the items and use cosmetic CSS styling to create the illusion of inversion (which creates the problems related to this issue). That way, without actually "inverting" the data order we are left with empty hands...

So we would need to change the logic for positioning those items so it works in an inverted style.

Wouldn't the logic be displaying the items in an inverted order? If that is so, we would need to create a logic to reverse the order of the items somewhere in the code. And the further down the line we do it, the more points of contact we would touch. By doing it once we would guarantee that we would only re-render (and consequently running data.slice().reverse() [or some similar algorithm]) the main component when the data itself changes as well.

All other optimizations related to virtual items in view will continue to work as expected because the VirtualList props remain the same.

marcaaron commented 3 years ago

To be clear, I don't know what it would take to do this. But I don't see why the order of the items themselves would need to change. I'd imagine it would work like this:

Data passed in with a certain order
List component starts at index 0 and determines the ideal placement of the first item starting from the bottom of the scroll view
Move to index 1 and place it above that one
And so on...

Expensify / App