[HOLD for payment 2022-11-29] [Hold for payment 2022-11-21] [$250] Mac / Safari : After selecting a text via mouse and then usingCMD+C to copy it does not work reported by @parasharrajat

[kavimuru]

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. Go to any chat
2. Select a part of any message
3. Use shortcuts to copy the text.
4. Try pasting the text in the composer.

Expected Result:

the selected text should be copied and pasted in the composer.

Actual Result :

1. Select part of the old message, right click my mouse, our context menu shows rather than the browser's menu, click Copy to Clipboard, and paste into input area, the whole message been pasted, not the selection text.
2. Select part of the old message, press Cmd+C to copy selection, and press Cmd+V or the browser's menu to paste into input area, nothing been pasted.

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.2.24-4
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Recording.8.mp4

Expensify/Expensify Issue URL:
Issue reported by: @parasharrajat
Slack conversation:
https://expensify.slack.com/archives/C01GTK53T8Q/p1667834088430199
View all open jobs on GitHub

[melvin-bot]

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

[melvin-bot]

Current assignee @puneetlath is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

Current assignee @puneetlath is eligible for the External assigner, not assigning anyone new.

[yevheniitsuryk]

Proposal

The actual problem is in these lines of code:

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

Lines 171 to 174 in ea35c46

	onKeyDown={(event) => {
	// Blur the input after a key is pressed to keep the blue focus border from appearing
	event.target.blur();
	}}

This fix was made according to this PR #8443 for this issue #7853

It's unfocusing the whole message including selected text, when you press any button.

So, I propose to add next condition/exception for shortcuts meta keys (CMD + Ctrl):

onKeyDown={(event) => {
    if (event.metaKey) { return; }
    event.target.blur();
}}

It's perfectly working in Safari, and other platforms. The previous issue is also fixed.

CleanShot.2022-11-08.at.22.49.06.mp4

[parasharrajat]

Good find @yevheniitsuryk.

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

Lines 171 to 174 in ea35c46

	onKeyDown={(event) => {
	// Blur the input after a key is pressed to keep the blue focus border from appearing
	event.target.blur();
	}}

looks very problematic to me. It could have many unknown side-effects. This issue is just one of them.

Based on the real issue, looks like we want to disable the blue border on Shift+click or say it Shift only. So What about we restrict this keyDown handler to these combinations? It is important that this blue border is shown when the user uses Shift+Tab or Tab` for navigation.

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[yevheniitsuryk]

@parasharrajat actually if we remove fix with .blur(), it triggers focus on every keyPress (not only Shift key). Check it on video, on Safari all symbol keys, on Chrome literally every key (except functional keys like Ctrl/cmd alt/option):

CleanShot.2022-11-09.at.10.18.11.mp4

CleanShot.2022-11-09.at.12.28.14.mp4

So the problem turned out to be more serious than it seemed at first)

I tried to research this strange behavior with focusing on every key press, but no results for now.

Also tried to fix behavior by adding more conditions (exception) on shift and tab keys, but problem is that you can't catch shift + tab shortcut - firstly you catch Shift button press, after you are catching Tab (with pressed shift).

                onKeyDown={(event) => {
                    console.log('event', event)

                    if (event.metaKey || (event.shiftKey && event.keyCode === 9) || event.keyCode === 9) { return; }

                    event.target.blur();
                }}

And if we catch Shift - we make blur() for event.target element and after that we have different behaviors in browsers (Safari loses previous focusable element and then start focusing on first element on page, and Chrome loses focus on current element, but after that still able to catch previous and next focusable elements), you may check next videos:

CleanShot.2022-11-09.at.12.42.59.mp4

CleanShot.2022-11-09.at.12.46.20.mp4

I think in this case we should solve the root of the issue - disable focusing on elements on every key press. So I'm still researching.

[parasharrajat]

Thanks for digging into this.

I think in this case we should solve the root of the issue - disable focusing on elements on every key press.

Does this proposal still work? And you are prefering fix the above root cause instead of this.

Question:

1. So we are blurring on each key press. Does it have any issues with it?

[yevheniitsuryk]

Updated proposal

@parasharrajat yes, this proposal works okay, it's fixing problem with text selection on messages. Answering your question - no issues with blurring on each keypress. I made modification according to your comment - now it will blur on single Shift key press, navigation on Tab and Shift+Tab works, and there is final code:

  onKeyDown={(event) => {
     if (event.metaKey || (event.shiftKey && event.keyCode === 9) || event.keyCode === 9) { return; }
     event.target.blur();
   }}

(Tested on Safari)

@parasharrajat But please check this part of my previous comment, where I described different behavior on Safari and Chrome

[parasharrajat]

Ok, so you are saying that your proposal is working and has no inconsistencies across browsers. And changing the conditon(as per my request) has different effects on different browsers.

I will test your solution and I think it also needs a general discussion as I don't think blurring on the key press is a good idea. May be we can seek a better solution as part of this issue.

[WestDon]

Proposal

Instead of a blur that breaks copy, we can just add, or remove the style. Method onKeyUp needs to restore default Tab focus highlight behaviour.

                onKeyDown={(event) => {
                    const target = event.target;

                    // Hide blue formatting if Div and not s Tab
                    if (target.nodeName === 'DIV' && event.code !== 'Tab') {
                        target.style.outline = 'none';
                        target.style.boxShadow = 'none';
                    }
                }}
                onKeyUp={(event) => {
                    const target = event.target;

                    // Restore blue formatting if any combination with Tab
                    if (target.nodeName === 'DIV' && event.code === 'Tab') {
                        target.style.outline = null;
                        target.style.boxShadow = null;
                    }
                }}

[henriquefalconer]

Proposal

The Pressable component inside src/components/PressableWithSecondaryInteraction/index.js, when processed by react-native-web, is rendered as a div with the default HTML attribute tabindex="0".

<div tabindex="0" class="css-view-175oi2r r-cursor-1loqt21 r-touchAction-1otgn73" style="touch-action: auto; -webkit-user-select: none; -webkit-user-drag: none;">

The tabindex attribute makes the div focus when the user presses a key, which seems to be the root cause of the problem noted in this issue and the one in issue #7853.

So I propose to add in componentDidMount() inside PressableWithSecondaryInteraction the following code, which sets the native prop tabindex of the Pressable component to undefined, removing tabindex from the page:

this.pressableRef.setNativeProps({
    tabIndex: undefined,
});

IMPORTANT: This solution is not aligned with your migration to Fabric. Read "P.S" for more details.

The entire componentDidMount() would look like this:

    componentDidMount() {
        // Prevent unwanted focusing of element (Pressable auto implements tabindex="0")
        this.pressableRef.setNativeProps({
            tabIndex: undefined,
        });
        if (this.props.forwardedRef && _.isFunction(this.props.forwardedRef)) {
            this.props.forwardedRef(this.pressableRef);
        }
        this.pressableRef.addEventListener('contextmenu', this.executeSecondaryInteractionOnContextMenu);
    }

Also, this would mean that this portion of the code would be eliminated:

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

Lines 171 to 174 in ea35c46

	onKeyDown={(event) => {
	// Blur the input after a key is pressed to keep the blue focus border from appearing
	event.target.blur();
	}}

Leaving the PressableWIthSecondaryInteraction free of the code that seemed problematic, and without creating unnecessary side-effects.

It's perfectly working in Safari, and other platforms. The previous issue is also fixed.

Screen.Recording.2022-11-09.at.13.12.20.mov

P.S.: I know you are migrating to Fabric. setNativeProps is not allowed with it. I'm looking for alternate solutions right now, but I imagine the easiest would include altering the file src/modules/createDOMProps/index.js on your forked @expensify/react-native-web lib and adding a new option to the existing focusable prop values, allowing for tabindex to not be included (focusable controls tabindex). Here's how focusable works nowadays. None of the existing focusable values currently removes tabindex.

[parasharrajat]

Both are interesting solutions. Resetting the style is a good idea but very hacky.

@henriquefalconer Correct, we are migrating to the fabric so we can't use setNativeProps. I also think we should not deep into workarounds for this issue. If you think that we should add a new feature to RN-web. I would say your propose that on the RN-web and let's see what the maintainer has to say.

I will prefer the best simple solution for this task.

[henriquefalconer]

Updated proposal

As requested, here's an alternate and simpler solution for this task.

I propose changing the Pressable component inside src/components/PressableWithSecondaryInteraction/index.js into a TouchableWithoutFeeback component. It has all Pressable props needed and doesn't generate a tabindex attribute.

Here's what it looks like currently:

App/src/components/PressableWithSecondaryInteraction/index.js

Lines 67 to 81 in ed86487

	return (
	<LongPressGestureHandler onHandlerStateChange={this.callSecondaryInteractionWithMappedEvent}>
	<Pressable
	style={this.props.inline && styles.dInline}
	onPressIn={this.props.onPressIn}
	onPressOut={this.props.onPressOut}
	onPress={this.props.onPress}
	ref={el => this.pressableRef = el}
	// eslint-disable-next-line react/jsx-props-no-spreading
	{...defaultPressableProps}
	>
	{this.props.children}
	</Pressable>
	</LongPressGestureHandler>
	);

And here's what it will look after my alterations:

        return (
            <LongPressGestureHandler onHandlerStateChange={this.callSecondaryInteractionWithMappedEvent}>
                <TouchableWithoutFeedback
                    onPressIn={this.props.onPressIn}
                    onPressOut={this.props.onPressOut}
                    onPress={this.props.onPress}
                    // eslint-disable-next-line react/jsx-props-no-spreading
                    {...defaultPressableProps}
                >
                    <>
                        <View style={this.props.inline && styles.dInline} ref={el => this.pressableRef = el}>
                            {this.props.children}
                        </View>
                    </>
                </TouchableWithoutFeedback>
            </LongPressGestureHandler>
        );

In order for this to work, I inserted this.props.children inside a fragment and a View, as TouchableWithoutFeeback only accepts one child (and for some reason removing the fragment made the blue border appear again), and also altered the location of the ref prop, as TouchableWithoutFeeback won't accept ref as a functional prop.

The outcome does not produce any unwanted effects, and retains all current funcionality:

• onPressIn prop used on src/pages/home/report/ReportActionItem.js and src/pages/settings/AppDownloadLinks.js behaves the same way
• onPressOut prop used on src/pages/home/report/ReportActionItem.js and src/pages/settings/AppDownloadLinks.js behaves the same way
• inline style prop used on src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js behaves the same way
• preventDefaultContentMenu prop used on src/pages/home/report/ReportActionItem.js behaves the same way

P.S.: Applying this would also mean that this portion of the code could be eliminated:

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

Lines 171 to 174 in ea35c46

	onKeyDown={(event) => {
	// Blur the input after a key is pressed to keep the blue focus border from appearing
	event.target.blur();
	}}

It's perfectly working in Safari, and other platforms. The previous issue is also fixed.

Screen.Recording.2022-11-09.at.20.14.10.mov

[parasharrajat]

Awesome @henriquefalconer that looks more reasonable to do. I will test it just to be sure first. https://github.com/Expensify/App/blob/f654cc13794aa749a29b6561bad94d80a9580fd4/src/components/PressableWithSecondaryInteraction/index.js is deeply involved in many things.

[parasharrajat]

Ok, I tested it. It seems to work great.

@henriquefalconer's proposal looks good.

[s77rt]

Won't the @henriquefalconer proposal's remove the tabbing functionality? We can't tab thought messages?

[parasharrajat]

I think I checked that. let me do that again.

Working great.

[s77rt]

@parasharrajat
Can you upload a video?
I can't tab from my side

[parasharrajat]

I am going to revert the Regression PR #8443 which is causing a trail of regressions.

cc: @puneetlath

What do you think?

[s77rt]

@parasharrajat
I still think my proposal is better than the revert, any reasons why you think we should revert instead?

Also i think we should close this issue and track only on #12010

[parasharrajat]

I think all of these issues originated from that PR and closing it gets rids of them so it is better.

If you think we should solve #7853, you can open a new discussion on slack to reopen that.

Thanks.

[s77rt]

@parasharrajat Understood. I was for the revert at first but then I found that the issue effect every key not only SHIFT.
Click on any chat, press any key, the border appears, it's a little annoying. If you think it's still acceptable then I'm okay with the revert.

[parasharrajat]

Yup, I discussed that already. We are good to keep that for now. But as I said, if you have a better solution in mind, you can raise the same on slack and tag the previous issue.

[puneetlath]

@parasharrajat I agree let's revert that.

[henriquefalconer]

Alright @parasharrajat! Thanks for considering my proposal 😉

[parasharrajat]

Unfortunately, there is no clean solution available for this. I was happy to do #12554 (comment) but it still has one issue so reverting is a good call.

Looking forward to your contributions on other issues.

[melvin-bot]

📣 @rushatgabhane You have been assigned to this job by @puneetlath!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[puneetlath]

Agreed, thanks for the engagement everyone! We look forward to continuing to work with you on other issues.

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• @puneetlath A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: n/a we're discussing internally whether Applause should test on Mac safari
• @parasharrajat @rushatgabhane @puneetlath The PR that introduced the bug has been identified. Link to the PR: workaround to hide blue border box on ReportActionItem #8443
• @puneetlath The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: workaround to hide blue border box on ReportActionItem #8443 (comment)
• @puneetlath A discussion in #contributor-plus has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1669042323944389
• @puneetlath Payment has been made to the issue reporter (if applicable)
• @puneetlath Payment has been made to the contributor that fixed the issue (if applicable)
• @puneetlath Payment has been made to the contributor+ that helped on the issue (if applicable)

[parasharrajat]

The PR that introduced the bug has been identified. Link to the PR:

#8443

[melvin-bot]

@puneetlath, @parasharrajat, @rushatgabhane Eep! 4 days overdue now. Issues have feelings too...

[puneetlath]

@parasharrajat @rushatgabhane sent you hiring offers in Upwork.

https://www.upwork.com/jobs/~01e0681ffe1aaa24a7

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.29-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Revert "workaround to hide blue border box on ReportActionItem" #12718

If no regressions arise, payment will be issued on 2022-11-29. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[puneetlath]

@rushatgabhane quick bump on accepting the hiring offer. Then we'll be able to close this out. Thanks!

[rushatgabhane]

Thanks @puneetlath, accepted!

[puneetlath]

All paid!