[HOLD 33725][$500] Web - Chat - Two Messages can not be selected when they are divided by reply

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.37-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): gocemate@gmail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

1. Navigate to staging.new.expensify.com
2. Go to any chat
3. Send a message
4. Add a reply to the message
5. Send another message
6. With mouse select last message and continue to select first message with reply

Expected Result:

Messages should be selected in a consistent manner where there is no 'jumping' or 'flashing' of the highlighted area. The highlighted area shouldn't be disrupted by 'reply to threads' in the chat.

Actual Result:

The highlighted area is no consistently selected when you drag the curser over it.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [X] MacOS: Chrome / Safari
• [X] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/eb044560-264f-4cdf-8a54-5c5504f0b6e9

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0135be761efeca8eb4
• Upwork Job ID: 1754943044230656000
• Last Price Increase: 2024-02-27

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0135be761efeca8eb4

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

[melvin-bot[bot]]

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[lanitochka17]

We think that this bug might be related to #vip-vsp CC @quinthar

[Christinadobrzyn]

testing this, sometimes you can highlight the chat. So I don't know if we want to fix this now - asking https://expensify.slack.com/archives/C066HJM2CAZ/p1707253470143849

https://github.com/Expensify/App/assets/51066321/739373e6-0e4b-42ee-a09e-e170fdf62012

[Christinadobrzyn]

I think we can broaden the scope of this OP to allow for users to highlight an entire chat thread regardless of breaks in the chat - asking the team for a review - https://expensify.slack.com/archives/C066HJM2CAZ/p1707429247200849

[mananjadhav]

@Christinadobrzyn Any updates on the discussion? I can't access the slack thread.

[Christinadobrzyn]

no response from the team yet, I'll keep monitoring our decision

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[Christinadobrzyn]

Gonna try the BZ room to see if I can get an answer - https://expensify.slack.com/archives/C01SKUP7QR0/p1707868596042429

Also asking QA if they know what is expected here - https://expensify.slack.com/archives/C9YU7BX5M/p1707868718740389

[Christinadobrzyn]

Talking this over with QA @isagoico we both agree this should be fixed to mimic the highlight abilities of Slack or GH

Expected behaviour:

You should be able to highlight an entire 'area' of a chat without it glitching or being disrupted by a 'reply to chat thread'.

So the behaviour should be more like this:

https://github.com/Expensify/App/assets/51066321/f0019e3d-1a7c-4c74-8329-a54c1c48d647

instead of how it is now where the highlighted area 'jumps' around and isn't consistent.

https://github.com/Expensify/App/assets/51066321/eb220076-40ba-4c17-94a0-f5440499c624

Let me know your thoughts @mananjadhav

[Santhosh-Sellavel]

@Christinadobrzyn I think you meant @mananjadhav

[mananjadhav]

Okay I get what you mean @Christinadobrzyn. This is now open for proposals.

[brandonhenry]

Proposal

Problem Statement

The issue at hand is that text selection within the chat messages is being reset when trying to copy the text of an entire chat thread

Root Cause

The root cause of this problem is our use of InvertedFlatList here.

Changing this to FlatList fixes the issue. This is odd considering our InvertedFlatList is just a FlatList under the hood, so I took a look at the BaseInvertedFlatList implementation and found that the root cause is without a doubt the inverted flag used on the FlatList object here

https://github.com/Expensify/App/assets/15656774/c31b8d59-f237-4ed6-9ee7-ac5771231875

Proposed Solution

To address this issue, we should stop using InvertedFlatList since the inverted property of a FlatList is causing weird issues with selection.

Instead, let's just use a normal FlatList and mimic the inverted styling that happens when passing in that property.

We can reverse the ordering of the flatlist items (sortedReportActions.reverse()) and also apply the same style that the FlatList would apply if we add inverted to it. That style is style={{transform: [{scaleY: -1}]}}. We should add that here

(See FlatList React Native Documentation)

These changes fix the issue as you can see in my recording. This may not be the most optimal solution but I do believe I'm on the right path.

https://github.com/Expensify/App/assets/15656774/2dccb6ca-ce1c-4111-bec1-90a5d0130a56

As these are no minor changes, and could have other implications (also took quite awhile to finally uncover) - i propose the bounty be raised on this ticket

[melvin-bot[bot]]

@mananjadhav @Christinadobrzyn this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[Christinadobrzyn]

Thanks @mananjadhav! We're accepting and reviewing proprosals

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[Victor-Nyagudi]

This one's tricky.

First, the text with the reply count in between messages is not meant to be selected or copied because of these lines. Both have styles.userSelectNone.

https://github.com/Expensify/App/blob/20ca041595e76cdd289977e55b2af3fbb9618868/src/pages/home/report/ReportActionItemThread.tsx#L61-L64

https://github.com/Expensify/App/blob/20ca041595e76cdd289977e55b2af3fbb9618868/src/pages/home/report/ReportActionItemThread.tsx#L67-L71

The dataSet value is used to remove them from the clipboard content in SelectionScraper.

https://github.com/Expensify/App/blob/20ca041595e76cdd289977e55b2af3fbb9618868/src/libs/SelectionScraper/index.ts#L91-L97

This pattern has been used in multiple places in the code base e.g. https://github.com/Expensify/App/issues/26694.

Secondly, if you inspect the HTML, you'll notice that the divs housing the different messages are arranged in reverse i.e. message 2 div is below message 1, but the messages rendered on the page are in the correct order.

After tinkering a bit on codepen, I found that even if there's a div or button in between that shouldn't be selected, you should still be able to select everything else just fine.

All that being said, I believe the order in which the messages are rendered or the HTML produced could be interfering with the selection behavior - unless maybe this worked at some point and is now broken.

I thought about changing the message order or playing around with how the HTML is generated, but I felt like this could have some unintended side effects.

Either way, I'm letting this one go, so hopefully this provides some context to the next person who decides to take a crack at it.

[Christinadobrzyn]

hi @mananjadhav could you take a peek at the proposals we have so far? Thanks!

[quinthar]

I'm curious if the new comment-linking code changes the order of to be correct? /cc @Szymon20000 I think right now the are in the opposite order they are displayed, and we're using some kind of hack on flat list to show it opposite?

[perunt]

We made a change some time ago and have been using this setup in production ever since. Now, it renders as truly inverted, meaning without any stylistic tricks

[melvin-bot[bot]]

@mananjadhav @Christinadobrzyn this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[melvin-bot[bot]]

@mananjadhav, @Christinadobrzyn Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[mananjadhav]

I don't think we have satisfactory proposals here. @brandonhenry The proposal you mentioned doesn't specifically mention any root cause. It would probably solve re-renders, but not message selection.

All that being said, I believe the order in which the messages are rendered or the HTML produced could be interfering with the selection behavior - unless maybe this worked at some point and is now broken.

I think the comment here disregards this right?

Still waiting for better proposals.

[Christinadobrzyn]

Thanks @mananjadhav do you think we should raise the bounty here?

[brandonhenry]

I think bounty should be raised here as it was a tough one to find. Updated proposal

Cc. @Christinadobrzyn @mananjadhav @perunt

[nagypite]

The problem clearly seems to be coming from the inverted rendering order, see recording below:

Screencast at: 2024-02-29 12-28-23.webm

First it shows on the left side the current staging selection behavior, then a modified dev version without the inversion, selection working as expected. The latter one was achieved by removing a single line:

diff --git a/src/components/InvertedFlatList/BaseInvertedFlatList.tsx b/src/components/InvertedFlatList/BaseInvertedFlatList.tsx
index e284005052..29f090d46f 100644
--- a/src/components/InvertedFlatList/BaseInvertedFlatList.tsx
+++ b/src/components/InvertedFlatList/BaseInvertedFlatList.tsx
@@ -10,18 +10,17 @@ function BaseInvertedFlatList<T>(props: FlatListProps<T>, ref: ForwardedRef<Flat
     return (
         <FlatList
             // eslint-disable-next-line react/jsx-props-no-spreading
             {...props}
             ref={ref}
             windowSize={WINDOW_SIZE}
             maintainVisibleContentPosition={{
                 minIndexForVisible: 0,
                 autoscrollToTopThreshold: AUTOSCROLL_TO_TOP_THRESHOLD,
             }}
-            inverted
         />
     );
 }

 BaseInvertedFlatList.displayName = 'BaseInvertedFlatList';

 export default forwardRef(BaseInvertedFlatList);

@perunt Could you give some context about the decision around the inverted rendering?

We made a change some time ago and have been using this setup in production ever since. Now, it renders as truly inverted, meaning without any stylistic tricks

Should I look into changing the rendering order, or is this something that shouldn't be meddled with?

[mananjadhav]

I checked this here for some additional feedback. Afaik, we shouldn't be changing the inverted flag and look at styling workarounds.

Tagging @roryabraham here for their feedback too.

cc - @rushatgabhane as they pointed out if bidirectional scrolling could potentially fix this.

[brandonhenry]

@mananjadhav i can't see that thread as I do not have access. Can you summarize the discussion? I understand not wanting to touch the inverted flag - are we also against using a FlatList for messages instead of inverting it?

I think if anything, inverting the FlatList is the wrong solution for out chat messages. I will dive deeper, but I don't see how we will be able to achieve the desired result if we rely on FlatList to re-order the div's of our react component (that's just how FlatList inverted works, it will re-order the div under the hood).

Scrolling seems to have nothing to do with this and also bi-directional scroll could have major implications for android

Since we can very easily reverse the list with our own styling (so it doesn't mess with the order of the DOM elements) - not sure why we wouldn't choose that route but hopefully the thread makes more sense of that

[brandonhenry]

https://github.com/Expensify/App/assets/15656774/5fed7a32-3cd9-46e5-b296-b86d9b80c31f

I have recorded a video to outline what I mean. This is a debug that I added to the page so we can see where the start of our selection is. Not only does it cause the "start" position of our selectin to constantly update, but you can clearly see just how badly it jumps around - all because we use the inverted property on the FlatList. There are several reasons for this:

1. Inverted Rendering: Since the list is inverted, the browser's default selection behavior does not align with the visual presentation of the list. The browser doesn't know that the list is visually inverted, so it treats selections as if the list were in normal order.

2. Selection Extension: When you drag to extend a selection, the browser updates the start and end points of the selection range based on the direction of the extension. In an inverted list, this behavior can cause the selection start to move in unexpected ways.

Apply this code to your console and you'll see just how much the "directin" of your selection changes when the FlatList has the inverted property

const checkSelectionDirection = () => {
  const selection = window.getSelection();
  if (selection.rangeCount > 0) {
    const range = selection.getRangeAt(0);
    const direction = range.startContainer === selection.anchorNode &&
                      range.startOffset === selection.anchorOffset
                      ? 'downwards'
                      : 'upwards';
    console.log('Selection Direction:', direction);
  }
};

document.addEventListener('selectionchange', checkSelectionDirection);

3. Browser Selection Heuristics: Different browsers have different heuristics for handling text selection, especially in complex scenarios like inverted lists. These heuristics will just further exacerbate issue and cause the selection behavior to vary across browsers.

---

Say all this again to support the claim we should not be using FlatList like this for our chat messages. We should just use a normal FlatList and not use inverted propety on it

[s77rt]

We already dealt with this bug here https://github.com/Expensify/App/issues/1341. The bug is in RNW and we have an upstream PR https://github.com/necolas/react-native-web/pull/2151 however that PR is closed in favor of a react-native fix https://github.com/facebook/react-native/issues/30383. Yet we applied the fix here https://github.com/Expensify/react-native-web/pull/2 and here https://github.com/Expensify/react-native-web/pull/3 but that change is no longer in place as I think we switched back to the upstream RNW repo here https://github.com/Expensify/react-native-web/pull/6 and left behind the patch.

I think our options are:

1. Reapply the patch (assuming we actually forgot it and it was not removed intentionally cc @roryabraham)
2. Hold on https://github.com/facebook/react-native/issues/30383

[Christinadobrzyn]

Thanks @s77rt let me know if I can help with making those decisions - @mananjadhav what do you think would be best?

[melvin-bot[bot]]

@mananjadhav @Christinadobrzyn this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

Current assignee @mananjadhav is eligible for the Internal assigner, not assigning anyone new.

[melvin-bot[bot]]

@mananjadhav @Christinadobrzyn this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

[Christinadobrzyn]

Just checking in on an update @s77rt and @mananjadhav - thanks!

[mananjadhav]

@roryabraham Could you please confirm if we intentionally did not add the patch?

I don't think the upstream issue is going to be worked upon anytime soon.

[Christinadobrzyn]

DMd @roryabraham re https://github.com/Expensify/App/issues/35946#issuecomment-1987329556

[melvin-bot[bot]]

@mananjadhav, @Christinadobrzyn Eep! 4 days overdue now. Issues have feelings too...

[brandonhenry]

Yep, this issue is known and has been around for some time https://github.com/Expensify/App/issues/1341

[brandonhenry]

I think we should just try to hold on this ticket and push on the original PR that React Native devs seem to be pursuing. See https://github.com/facebook/react-native/issues/30373#issuecomment-1524520237

The react native dev there reached out to an expensify team member who I think no longer is on the expensify team? We should respond to their inquiry

Cc. @mananjadhav @Christinadobrzyn @puneetlath (since you have context)

[Christinadobrzyn]

Ah thanks for the context @brandonhenry!

Do I have it right that we should HOLD this for https://github.com/Expensify/App/issues/33725

Does that sound like a plan @mananjadhav and @puneetlath?

[mananjadhav]

Yeah I think it makes sense to wait. Not sure why @mallenexpensify is tagged. But yeah we can hold.

[Christinadobrzyn]

HOLD for https://github.com/Expensify/App/issues/33725

[brandonhenry]

Just leaving a note here that we will need to constantly push on the react-native ticket in order for this to move 👍🏿

[Christinadobrzyn]

Just checking in here, @mananjadhav can you provide an update? I'm a little lost where we are here. Can you also confirm if this should stay on hold? https://github.com/Expensify/App/issues/37447

[Christinadobrzyn]

this is on hold for - https://github.com/Expensify/App/issues/33725

[Christinadobrzyn]

This is still on hold for - https://github.com/Expensify/App/issues/33725

I do think this still needs to be fixed

[Christinadobrzyn]

This is still on hold for - https://github.com/Expensify/App/issues/33725