[$1000] Android - Copy / Paste / Cut menu is not displayed when selecting text in compose box - Reported by: @parasharrajat

[isagoico]

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

---

Action Performed:

1. Navigate to a conversation
2. Type anything in compose box
3. Select it

Expected Result:

Native menu to cut / copy / paste should be displayed

Actual Result:

Native menu to cut / copy / paste is displayed for a second and then it's hidden

Workaround:

User is unable to cut / copy / paste text in the compose box.

Platform:

Where is this issue occurring?

• Android

Version Number: 1.1.23-0

Reproducible in staging?: yes
Reproducible in production?: yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

WhatsApp.Video.2021-12-22.at.11.37.59.AM.mp4

Expensify/Expensify Issue URL:

Issue reported by: @parasharrajat
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1640179428463500

View all open jobs on GitHub

[MelvinBot]

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

[MelvinBot]

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

[Christinadobrzyn]

Upwork job created!

Internal: https://www.upwork.com/ab/applicants/1473877014903103488/job-details
External: https://www.upwork.com/jobs/~01333b75dd01dc96cb

Hired @parasharrajat for reporting this. Waiting on proposals for the fix.

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to @stitesExpensify (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[kursat]

React native's TextInput re-renders when its selection property is controlled. There is an open issue (facebook/react-native#29063) about this.

I'm not sure how convenient is this but we did it for ios, so I'm proposing to omit selection prop at TextInputFocusable/index.android.js.

    render() {
        const propsToPass = _.omit(this.props, 'selection');

        return (
            <TextInput
                placeholderTextColor={themeColors.placeholderText}
                ref={el => this.textInput = el}
                maxHeight={CONST.COMPOSER_MAX_HEIGHT}
                rejectResponderTermination={false}
                editable={!this.props.isDisabled}
                /* eslint-disable-next-line react/jsx-props-no-spreading */
                {...propsToPass}
            />
        );
    }

[parasharrajat]

Will it affect our app's functionality? @kursat

[kursat]

@parasharrajat, sorry to say but yes it will. At first glance I noticed this:

When we replace a selected text with an emoji, we will not be able to move cursor at the end of emoji

App/src/pages/home/report/ReportActionCompose.js

Lines 418 to 433 in d6a6073

	addEmojiToTextBox(emoji, emojiObject) {
	EmojiUtils.addToFrequentlyUsedEmojis(this.props.frequentlyUsedEmojis, emojiObject);
	this.hideEmojiPicker();
	const newComment = this.comment.slice(0, this.state.selection.start)
	+ emoji + this.comment.slice(this.state.selection.end, this.comment.length);
	this.textInput.setNativeProps({
	text: newComment,
	});
	this.setState(prevState => ({
	selection: {
	start: prevState.selection.start + emoji.length,
	end: prevState.selection.start + emoji.length,
	},
	}));
	this.updateComment(newComment);
	}

I tried to replace it like below, but setting selection to a js object like this, fixes position of cursor and you literally write backwards after than that.

        this.textInput.setNativeProps({
            selection: {
                start: this.state.selection.start + emoji.length,
                end: this.state.selection.start + emoji.length,
            },
        });

We are also passing selection prop from a couple files, but for now I am not sure about the impact yet. I'll work on this one more day and see if I can eliminate all possible side effects.

[parasharrajat]

Any update @kursat

[kursat]

Hey @parasharrajat, I was dealing some personal problems, I totally forgot about this. So sorry about the absurd delay.

The only component that sets selection is ReportActionCompose. To keep selection setting feature we can manage selection state in TextInputFocusable component. And by setting another ref for TextInputFocusable itself, we can set selection state programmatically.

src/components/TextInputFocusable/index.android.js:

class TextInputFocusable extends React.Component {
    constructor(props) {
        super(props);

        this.state = {
            selection: null,
        };
    }

    // ...

    setSelection = (selection) => {
        this.setState({
            selection,
        });
    }

    render() {
        const propsToPass = _.omit(this.props, 'selection');
        return (
            <TextInput
                placeholderTextColor={themeColors.placeholderText}
                ref={el => this.textInput = el}
                maxHeight={CONST.COMPOSER_MAX_HEIGHT}
                rejectResponderTermination={false}
                editable={!this.props.isDisabled}
                selection={this.state.selection}
                /* eslint-disable-next-line react/jsx-props-no-spreading */
                {...propsToPass}
            />
        );
    }
}

// ...

export default React.forwardRef((props, ref) => (
    /* eslint-disable-next-line react/jsx-props-no-spreading */
    <TextInputFocusable {...props} ref={props.ownRef} forwardedRef={ref} />
));

src/pages/home/report/ReportActionCompose.js:

    constructor(props) {
        super(props);

        // ...
        this.setTextInputFocusableRef = this.setTextInputFocusableRef.bind(this);

    }

    setTextInputFocusableRef(el) {
        this.textInputFocusable = el;
    }

    addEmojiToTextBox(emoji, emojiObject) {

        // ...

        if (this.textInputFocusable) {
            this.textInputFocusable.setSelection({
                start: this.state.selection.start + emoji.length,
                end: this.state.selection.start + emoji.length,
            });
        }

        // ...
    }

    render() {
        <TextInputFocusable
          autoFocus={this.shouldFocusInputOnScreenFocus || _.size(this.props.reportActions) === 1}
          multiline
          ref={this.setTextInputRef}
          ownRef={this.setTextInputFocusableRef}
          { /* ... */ }
        />
        // ...
    }
}

This way we don't brake anything.

[parasharrajat]

Great thanks. I see. If we are able to migrate from selection prop to this.textInput.setSelection API then I would love that solution. But I need to look at the code to analyze it before we proceed. It is going to be a big change. Better to involve @marcaaron.
we are looking to make these fully uncontrolled and this could be a good initiative.

[kursat]

Happy to help! You can experiment on this fork if it helps: https://github.com/kursat/App/tree/proposal-6876

[marcaaron]

Would like to clarify some things about this proposal to be sure I understand.

Is this a good description of the problem and solution?

• TextInput won't re-render when the selection prop changes
• So we should force it to re-render by manually setting the selection state

If so, this seems like a workaround and ideally, we would get it fixed at the react-native level or at the very least open a tracking issue to see if we can unwind the proposed changes when it is fixed upstream.

But also wondering if we can move the logic so that a re-render is forced when the props change as a side effect instead of programmatically triggering them with textInputFocusableRef.setSelection(). It seems this would be easier to remember and require less manual setting of the selection?

Would something like this work in TextInputFocusable/index.android.js?

componentDidUpdate(prevProps) {
    if (_.isEqual(prevProps.selection, this.props.selection)) {
        return;
    }

    this.setState({selection: this.props.selection});
}

[parasharrajat]

I was thinking that it could be a good opportunity for us to drop the selection prop which makes it controlled. We only require setting the selection when we want to place the emoji so if we can do it directly via API then I think that is better.

I have seen a few side effects. Sometimes cursor jump between positions when we move the cursor placement manually on Android. I believe its the selection prop.

[marcaaron]

We only require setting the selection when we want to place the emoji so if we can do it directly via API then I think that is better.

Got it. Just to make sure I understand the point of view, why is this better?

I have seen a few side effects. Sometimes cursor jump between positions when we move the cursor placement manually on Android. I believe its the selection prop.

Are there bug reports for these with reproductions? Is there a known connection between passing a selection prop and seeing the "cursor jump"?

[parasharrajat]

why is this better?

Using selection prop will make it controlled and if use API it will remain uncontrolled. I think that would be better performance-wise.

Are there bug reports for these with reproductions?

I haven't reported it yet. I will do that.

Is there a known connection between passing a selection prop and seeing the "cursor jump"?

It is just a hunch. I haven't debugged this yet. Due to the jump that happens when we move the cursor and cursor positioning is directly related to selection, I believe that is the cause. I will report it and then we can get more eyes on it.

Update: I can't reproduce the same on PROD. It may be some glitch on dev. So please ignore above.

[marcaaron]

Using selection prop will make it controlled and if use API it will remain uncontrolled. I think that would be better performance-wise.

Oh right, because the same can be observed with a controlled value prop? That could be! I haven't observed any performance issues with the selection prop but it can probably be tested before making a decision. Using a controlled value definitely feels slow, is the same true for selection? Maybe we can test and figure that out as part of this issue.

[Christinadobrzyn]

Raised the Upwork job price to $500. It looks like we're still reviewing proposals correct @parasharrajat?

[mallenexpensify]

Added Help Wanted and Exported, created a new job in Upwork https://www.upwork.com/jobs/~013607bfacea338b43

[kidroca]

I've submitted a proposal some time ago, but a contributor was already working on the ticket, maybe you can consider it now: #6876 (comment)

The PR from the slack thread: #7815

[mallenexpensify]

@parasharrajat , can you review @kidroca 's proposal and PR above?

[parasharrajat]

I will @mallenexpensify. Reviewing others ATM.

[parasharrajat]

@kidroca has a clean start but there are a few problems that we faced during implementation.

1. Selection works differently on IOS|ANDROID|WEB. Can you confirm if your proposal works on all platforms? I don't see any steps to solve those in the proposal.

Also, PR is outdated.

[kidroca]

1. Selection works differently on IOS|ANDROID|WEB. Can you confirm if your proposal works on all platforms

Yes, the change solves the issue on all platforms

Making selection uncontrolled fixes the issue for android/ios

This change is covering web/desktop:

        const caret = this.currentSelection.start + emoji.length;

        if (this.textInput.setSelectionRange) {
            this.textInput.setSelectionRange(caret, caret);
        }

We can move the web/desktop specific change here: https://github.com/Expensify/App/blob/main/src/components/TextInput/index.js

Instead of passing a defaultSelection that never changes (to achieve the uncontrolled mode) we can introduce a defaultSelection prop on our TextInput components that

Everything depends on what we want to cover

• if all we want to fix is the ReportActionCompose the changes on the PR cover all platforms
• if we want to support uncontrolled selection on all TextInput components we can implement a defaultSelection prop
  • defaultSelection allows us to set initial selection for the field, but then leave it uncontrolled
  • other TextInputs might not need this - if we don't pass selection then it's uncontrolled by default (unless we changed this by adding a default prop value)

[parasharrajat]

Thanks for the explanation. I retested your PR and it has issues on IOS which I was expecting. The cursor is placed at the end of Text whenever we add an emoji. Another issue is that when you select a bunch of text on Android and replace it with an emoji, the selection is changed abruptly covering other text on the editor.

if all we want to fix is the ReportActionCompose the changes on the PR cover all platforms

This is what we are looking to solve on this issue for now.

[kidroca]

Sorry, I guess I've only tested popping the context menu on ios and not inserting an emoji.

Tested it again and added a fix to preserve caret position after inserting an emoji, by using the interaction manager

• PR updated with 4 more line changes: Fix Android text selection context menu (copy, paste) #7815

    addEmojiToTextBox(emoji) {
        const newComment = this.comment.slice(0, this.currentSelection.start)
            + emoji + this.comment.slice(this.currentSelection.end, this.comment.length);

        this.updateComment(newComment);

        const caret = this.currentSelection.start + emoji.length;

        InteractionManager.runAfterInteractions(() => {
            this.textInput.setNativeProps({selection: {start: caret, end: caret}});
        });

        if (this.textInput.setSelectionRange) {
            this.textInput.setSelectionRange(caret, caret);
        }
    }

Screen.Recording.2022-03-24.at.20.24.19.mov

---

Seeing that we have access to this.textInput.selectionStart we might not need to capture this.currentSelection at all

[parasharrajat]

Did you test it on Android as well? I think it will have some problems there too. in the previous PR we have to loose the selection after immediately setting it.

[kidroca]

Yeah there's an issue on Android:

Here's a summary:

1. If we set selection prop, context menu does not work on Android
2. If we don't set native props, like originally suggested, everything works kind-a OK, with the exception that (on Android) if you select text and then replace it with an emoji, then the emoji remains selected
3. If we set selection through native props the carret starts to act weird (on Android) always jumping to the position we set by setNativeProps, so we have to quickly clear what we just set

InteractionManager.runAfterInteractions(() => {
  this.textInput.setNativeProps({selection: {start: caret, end: caret}});
    global.requestAnimationFrame(() => this.textInput.setNativeProps({selection: null}));
  });

But setting { selection: null } messes it for iOS where, the behavior is the same as 2) above - after inserting an emoji it's selected

---

In light of this, Seeing that we have Composer/index.(ios|android).js, I would suggest to introduce a setSelection method and have different implementations based on platform:

Web/Desktop:

setSelection(start, end) {
  this.textInput.setSelectionRange(start, end);
}

iOS:

setSelection(start, end) {
        InteractionManager.runAfterInteractions(() => {
            this.textInput.setNativeProps({selection: {start, end});
        });
}

Android:

setSelection(start, end) {
        InteractionManager.runAfterInteractions(() => {
            this.textInput.setNativeProps({selection: {start, end});
            global.requestAnimationFrame(() => this.textInput.setNativeProps({selection: null}));
        });
}

And in ReportActionCompose:

    addEmojiToTextBox(emoji) {
        const newComment = this.comment.slice(0, this.currentSelection.start)
            + emoji + this.comment.slice(this.currentSelection.end, this.comment.length);

        const caret = this.currentSelection.start + emoji.length;
        this.updateComment(newComment);
        this.textInput.setSelection(caret, caret);
    }

[kidroca]

Alternatively we can find/submit a bug report to react-native regarding Android, where using setNativeProps causes point 3) above, and we have to use setNativeProps again to clear the selection

[parasharrajat]

@stitesExpensify so we kind of ended with the same solution as in the previous PR. I think that there are a few bugs in RN. RN suggests using controlled props to prevent cursor issues but on Android, the menu is not opening.

so my suggestion would be we just file a report on RN for the native Copy-paste menu and keep the same composer implementation.

What do you say?

[stitesExpensify]

Yep, a fix to RN is always preferable over a workaround in our codebase. We can file a report, but also I believe that if a contributor creates a PR to fix RN we still pay out the upwork issue even though it's not in our codebase. Is that correct @mallenexpensify ?

[kidroca]

Controlled selection is defeating the purpose of the uncontrolled TextInput, it causes a re-render each time we input a character - something we originally tried to avoid

The reason we're not setting value on the TextInput is to avoid re-rendering for each keystroke
I guess we've missed this when selection was added to the component

Selection.re-render.mp4

You can also observe how words sometime get selected while you type - that's another side effect of controlled selection

A problem with controlled props in general is that a component doesn't know we're echoing back it's own value. And echoing can be delayed resulting in bugs like selecting previous words while you type
Specifically for react-native echoing results in sending data back and forth through the native bridge, and this bridge is the bottleneck of react-native

---

It's seems, on Android, setting selection through props or through setNativeProps doesn't work as expected, the cause might be the same, but ideally the compose box would be more performant if it sets selection only when necessary through setNativeProps. Perhaps the focus for the react-native ticket should be on setNativeProps and not on controlled selection

[kidroca]

Yep, a fix to RN is always preferable over a workaround in our codebase.

How do we decide that?
For example we have this code, clearly stating it's a workaround:

App/src/pages/home/report/ReportActionCompose.js

Lines 272 to 276 in c04fca7

	// Keyboard is not opened after Emoji Picker is closed
	// SetTimeout is used as a workaround
	// https://github.com/react-native-modal/react-native-modal/issues/114
	// We carefully choose a delay. 50ms is found enough for keyboard to open.
	setTimeout(() => this.textInput.focus(), 50);

What I'm suggesting is similar, it also uses the InteractionManager and delays code using requestAnimationFrame (like setTimeout), yet my proposal is considered a "workaround better to be addressed in RN"

What I'm proposing is a fix of about 30 line changes, that also addresses the re-render per keystroke issue vs finding/waiting for someone to identify the issue in Android and submit a fix to react-native

We can apply my fix, and all we have to do would be to remove one line - requestAnimationFrame, when the issue in Android is resolved

[parasharrajat]

I have noticed that controlled props perform more badly on dev builds than PROD. I think ReactActionComposer is also lacking optimizations. A single render takes a bit of time. If optimize this. We can get better results.

it has many things to render

1. AttachmentModal
2. AttachmentPicker.
3. PopoverMenus.
4. TextInput.
5. Modals.

Also, inline Children is making it worse.

App/src/pages/home/report/ReportActionCompose.js

Line 425 in 9c50db7

<>

[stitesExpensify]

Yep, a fix to RN is always preferable over a workaround in our codebase.

How do we decide that?

I'm taking that from our design philosophy here Section 5.2

For example we have this code, clearly stating it's a workaround:

I think the main difference is that the maintainer of that package is not willing to update it whereas in this case the problem is that the platforms are not in sync because of react native itself. I think that in general, we should avoid workarounds.

IMO any solution that involves a timeout or similar mechanism is not ideal, including the code that you linked.

[rushatgabhane]

@sig5 you mind weighing in here? Maybe submit a proposal. Because your work on IOUAmount #7433 should help you solve this issue too.

[kidroca]

Thanks for the answers @stitesExpensify, sounds fair

As far as I see there's no way to address this issue on the javascript side then. We should make that clear when summoning other people to solve the problem, or at least let them know what have been tried so far, so they don't waste as much time ending on the same crossroad

Expensify has a fork of react-native where contributors can submit a PR with a fix for Android
Then Expensify submits a PR upstream and uses the forked version in App in the meantime

[stitesExpensify]

Great, sounds good. Do you think that we should keep this issue open, or create a new one specifying the solution should be in the fork?

[kidroca]

I lean toward creating a new ticket and summarizing what have been found out here

[mallenexpensify]

Missed this last week

Yep, a fix to RN is always preferable over a workaround in our codebase. We can file a report, but also I believe that if a contributor creates a PR to fix RN we still pay out the upwork issue even though it's not in our codebase. Is that correct @mallenexpensify ?

Yes, if someone fixes a bug anywhere that helps with the issue, they'll be compensated.

If we close this issue and open another, lets make sure to keep @parasharrajat on as C+.
@K4tsuki has already been compensated so we can close their PR (and anyone can use any of the code from there if wanted since @K4tsuki has been paid in full)

[mallenexpensify]

@kidroca can you create the new issue, count the time as part of your hourly work, include a link back to this issue and cc all active participants in this convo in the OP of the new issue? (I'll assign myself as CM and others once done)

[kidroca]

@mallenexpensify

@kidroca can you create the new issue, count the time as part of your hourly work, include a link back to this issue and cc all active participants in this convo in the OP of the new issue? (I'll assign myself as CM and others once done)

Yes, I'll post back when ready

[kidroca]

Created a new ticket of the same name here: #8349

[mallenexpensify]

Thanks @kidroca , should we close this one now?
I think we should double the price of the other, anyone object? cc @NicMendonca

[mallenexpensify]

Closing, doubled price of the other one