App: [Ready for payment via newdot][$16000] Android - Copy / Paste / Cut menu is not displayed when selecting text in compose box - Reported by: @parasharrajat

Hey @Julesssss, @kadiealexander,

Thanks for reaching out and considering my contributions. While I did invest time in research and provided insights, I wasn’t tracking my hours as I was aiming for the fixed price job. I remember highlighting that the issue might naturally resolve with our migration to the new architecture. However, I’m aware that the solution wasn’t as straightforward and there were overlaps with other issues.

Ultimately, the main work was carried out by another contributor. I genuinely appreciate the acknowledgment, but I’m not seeking any compensation for this. It’s challenging to measure the exact impact of my input, and I’m just glad I could assist in some way.

Best, @kidroca

Hello, we have identified the root cause of the issue and have a fix for it upstream in RN as well as our fork. See https://github.com/Expensify/react-native/pull/55 fro a complete explanation of the problem and the fix.

I agree that this should not be increased any more.

@Julesssss

I thought of Hermes as well 😄

The only suggestion I have before moving forward is that we share the Fabric proposal in the public Slack channel, just in case anyone calls out an exception. I’m sure everyone will be on board, but let’s just run it by the rest of the team. Does that work for you @kidroca?

I was actually planning to post about the “New RN Architecture” anyway because it literally replaces the bridge with Fabric and Turbo Modules, the bridge is the main RN bottle neck and source of some performance issues I’ll definitely post on slack now

While switching on the new architecture is as easy as flipping a switch to true we’ll indeed need to keep this switch to false until our dependencies are compatible I see this as a continuous work where we try to do everything necessary to make App work/build with Fabric and the new architecture The good thing is that we can indeed achieve this gradually

• update react-native to at least v0.68
• make a list of dependencies who already have newer versions that support fabric and we can update
  • use this list to Upgrade App
• make a list of dependencies who don’t yet have a fabric update
  • use this list to help get those package to a Fabric compatible version
• flip the newArchEnabled switch to true when above steps are satisfied
• if it turns out it’s still too unstable we can just flip the switch back, but at least we’ll be ready to put it to true shortly

Thank you @kidroca!! That’s very generous of you to be willing to share your expertise with us like that.

In that case @parasharrajat if you could please request your portion via newdot we should be good here. Thanks to everyone who worked on this!

Sure.

Related issues and PR.

1. This issue, Main PR https://github.com/Expensify/App/pull/11684

2. Issue https://github.com/Expensify/App/issues/6876 PR https://github.com/Expensify/App/pull/7717 , PR https://github.com/Expensify/App/pull/7815

3. Issue https://github.com/Expensify/App/issues/6154, PR https://github.com/Expensify/App/pull/7433

4. PR https://github.com/Expensify/App/pull/17687, We closed this in favor of this issue.

5. PR https://github.com/Expensify/react-native/pull/50

6. PR https://github.com/Expensify/react-native/pull/55

7. This issue, Main PR https://github.com/Expensify/App/pull/11684

8. Slack discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1645191589301989. I might be missing a few.

9. Now, the Agency team has been working on upgrading many libs internally to upgrade the RN version to the latest and enable new architecture. Those all issues are handled separately but required before we could merge the final solution in our app.

These are the main issues and PR which I remember but I think there have been more in the past which we closed.

I have been involved directly or indirectly in all of these PRs where I have run tests from time to time and tracked them.

Just a quick update: team Margelo identified a native fix for that upstream in react native, we will post a proposal this week 😊

Great. Thanks for the update. Please do tag me on the native PR as well if you want another hand in testing.

because it literally replaces the bridge with Fabric and Turbo Modules, the bridge is the main RN bottle neck and source of some performance issues

Oh wow, that’s great! Can’t wait for this.

So we should probably create a new issue for the migration, and put this one on hold. We’ll then consider this one fixed once we’ve migrated to Fabric successfully. Does that work for you?

@mdneyazahmad There is a long discussion on this issue and its past instance. I would strongly recommend going over it. It should be enough to understand the problem at hand. Issue details clearly explain the expectation from this issue. Sorry, but I didn’t see anything new on the proposal to discuss.

Reviewed the details for @parasharrajat. Approved for payment in NewDot based on the BZ summary above.

There is also this issue which should be fixed now:

• https://github.com/Expensify/App/issues/12854

Pardon, we do have an issue for the RN 0.72 upgrade:

• https://github.com/Expensify/App/issues/18444

Seems like for RN 0.72 we will still use the fork. We can hold this issue for the one named above (Version 0.72 includes the fix for this issue)

Hey team! I’m just adding a note that I’ll be OOO until June 26th. Since this is on hold I won’t reassign it, but if you need a bug0 teammate this week, please reapply the label! Thanks.

Proposal was posted in public here: https://expensify.slack.com/archives/C01GTK53T8Q/p1686159512184629

I’m afraid this was just discussed internally at the moment. I’ll keep an eye out for the final outcome and will share the issue that hopefully will be created to track it.

The fix was cherry-picked into 0.72, and released as 0.72.0-rc.6 https://github.com/facebook/react-native/releases/tag/v0.72.0-rc.6

Watch out for other merged/open PRs on the fork. They might already be releasing a new version.

Here’s the issue where I try to update and release our React Native fork. Wish me luck

Going to assign @Julesssss to review the Fork PR for 2 here

@s77rt It very likely will, I can allocate some time this week to update the PR and then test for the bug behaviour. If it fixes, I will let you know 😊

PR is in another round of testing review. Some light bugs were reported, which I will sort out today.

I will check.

In this PR: https://github.com/Expensify/App/pull/11684 i completely got rid of the selection prop (functionality is of course preserved), and on my android devices the menu is being displayed. Can anyone of you who had the issue test the PR and can confirm it’s fixed?

@kadiealexander There is no specific date for the migration. But we are in the process of upgrading the App to the latest version. Then only we can think about migrating to the new architecture.

Updated to Monthly since I don’t think anyone thinks the bug currently warrants a higher price tag. Correct me if you think I’m wrong (please… any/everyone, not just employees)

Bumped to Weekly since, I think, we’re awaiting proposals and we don’t need daily updates (let me know if that’s not the case)

We def want to upgrade to fabric. There was a discussion in past on this. Only challenge was that many of the third party libs does not support fabric yet. So transition will not be smooth.

Hi all, thanks again for all the proposals and discussion. @ahmdshrif while I agree that there may be some instability, it looks to me that Fabric is close to release. If released, it would certainly be the optimal choice.

Since Fabric is the future and the issue is resolved with it, we should not try to work on a fix for the legacy renderer, but make App meet the requirements to enable Fabric (update react, react-native and other libraries)

I agree with this, and think we should go with @kidroca’s proposal. Though not quite the same scenario, we upgraded to Hermes early and saw nothing but improvements to the app. Let’s live at the bleeding edge.

The only suggestion I have before moving forward is that we share the Fabric proposal in the public Slack channel, just in case anyone calls out an exception. I’m sure everyone will be on board, but let’s just run it by the rest of the team. Does that work for you @kidroca?

Hmm, I have to agree that I am a little behind on this. I will read the whole discussion and follow up.

@ahmdshrif

There’s already one workaround code that deal with selection here: https://github.com/ahmdshrif/react-native/blob/9f8bf5003a5f62b44b03a980654f83c2fe105f4f/Libraries/Components/TextInput/TextInput.js#L932-L942

  // Android sends a "onTextChanged" event followed by a "onSelectionChanged" event, for
  // the same "most recent event count".
  // For controlled selection, that means that immediately after text is updated,
  // a controlled component will pass in the *previous* selection, even if the controlled
  // component didn't mean to modify the selection at all.
  // Therefore, we ignore selections and pass them through until the selection event has
  // been sent.
  // Note that this mitigation is NOT needed for Fabric.
  // discovered when upgrading react-hooks
  // eslint-disable-next-line react-hooks/exhaustive-deps
  let selection: ?Selection =

Seems your proposal and the existing code around lastNativeSelectionState are very similar, both compare selection to the last captured native selection: https://github.com/ahmdshrif/react-native/blob/9f8bf5003a5f62b44b03a980654f83c2fe105f4f/Libraries/Components/TextInput/TextInput.js#L996-L1004

    if (
      selection &&
      lastNativeSelection &&
      (lastNativeSelection.start !== selection.start ||
        lastNativeSelection.end !== selection.end)
    ) {
      nativeUpdate.selection = selection;
      setLastNativeSelection({selection, mostRecentEventCount});
    }

Passing null seems like a simpler solution and I wonder why they didn’t go with it initially but instead went with using mostRecentEventCount approach

From the logic in the code comment above it seems the idea is to pass selection even if it’s the same as the nativeSelection, and they are only passing null until the event count settles, e.g. they avoid passing selection for a brief time, the are not trying to pass null when the native selection is already the same as the selection prop

3- set the selection to null if it’s not changed to not rewrite the native state.

         selection={isSelectionChange ? selection : null}

If I remember correctly this would not work on ios and require a workaround to skip the fix for that platform

@Julesssss It’s not exactly a bug in App - we’re using the selection prop to set initial selection / cursor position, and because we’re passing this prop we have to update it whenever the selection changes, otherwise it would remain with the initial value (Imagine using the value prop, you have to keep track of it in state and pass it as it changes otherwise the field would be stuck with whatever you’ve initially passed for value. This is called a controlled component and it causes a rerender for each character change)

I am following the proposals. Need to setup a few things to analyze the proposals/explanations above before I can leave some review.

Hi @ahmdshrif, thanks for the proposal. It looks promising, but I’m a bit confused by a couple of things:

• First, this Android context menu is really old and I’m not sure what it’s being shown for because it looks like an Android 4.4 component 😅. Was this just for testing? You said you built a native app to test the context menu? Or are you just using an older phone/native app?

1. setSelction on native does not show the context menu by default…
2. the copy/paste menu only appears when the selection is triggered by os itself

• Second, don’t these statements say different things? I’m pretty sure 2 is correct. The OS handles this for you on native android apps and I would expect this to also be true for RN. Do you mean that setSelection is a React Native function, and it isn’t working correctly?

It seems like your solution is to not send the event to native unless the text has actually changed? But I’m not sure why we are sending the text to native in the first place if the text hasn’t changed – is it instead a bug in our code which is causing us to resend the same text?

save any changes that come from native to separate state.

And finally, what changes are we receiving from native?

@parasharrajat @Julesssss i update the proposal now sorry for making it confusing.

i just do more test and the above proposal have a big issue so for not waste your time … please ignore it for now .

Proposal

Removing this line fixes the issue.

https://github.com/Expensify/App/blob/fe432c30fdd46db9c34d714c64db78c496761b7c/src/pages/home/report/ReportActionCompose.js#L550

                                    isDisabled={isComposeDisabled || isBlockedFromConcierge}
-                                   selection={this.state.selection}
                                    onSelectionChange={this.onSelectionChange}

We are omitting this prop from ios therefore it works there. We are only using this prop on android. We can remove this on android to be consistent. Finally, we need to refactor to remove unused variables.

https://github.com/Expensify/App/blob/fe432c30fdd46db9c34d714c64db78c496761b7c/src/components/Composer/index.ios.js#L75-L76

Result:

Any reason I shouldn’t double this @Julesssss @parasharrajat ?