Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Hold for payment 2024-02-13] [$500] Private Note - Code block paste in private notes does not work (works fine in native apps) #29806

Closed
4 of 6 tasks
kbecciv opened this issue Oct 17, 2023 · 81 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Oct 17, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.3.85.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):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697530399668279

Action Performed:

  1. Open the app
  2. Open any user report
  3. Send 3 backtick code block eg: Hii
  4. Hover and click copy to clipboard
  5. Click on header to open user details
  6. Click on privates notes-> my notes
  7. Paste the copied text and observe that app removes backticks while pasting

Expected Result:

App should paste text with backticks in private notes

Actual Result:

App does not paste text with private notes (works fine only in native apps)

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Android: Native
Android: mWeb Chrome
Android.chrome.copy.to.clipboard.code.block.paste.notes.mp4
iOS: Native
iOS: mWeb Safari
ios.safari.paste.code.block.in.notes.mov
MacOS: Chrome / Safari
windows.chrome.code.block.paste.notes.mp4
mac.chrome.paste.code.block.notes.not.working.mov
MacOS: Desktop
mac.desktop.paste.code.block.notes.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013e368c7fef7e59bc
  • Upwork Job ID: 1714338003846836224
  • Last Price Increase: 2023-11-07
@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 17, 2023
@melvin-bot melvin-bot bot changed the title Private Note - Code block paste in private notes does not work (works fine in native apps) [$500] Private Note - Code block paste in private notes does not work (works fine in native apps) Oct 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

Job added to Upwork: https://www.upwork.com/jobs/~013e368c7fef7e59bc

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot
Copy link

melvin-bot bot commented Oct 17, 2023

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Oct 17, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Code block paste doesn't work properly in private notes text input

What is the root cause of that problem?

When user copies code block message we set the code in 'html' prop of the clipboard and the plain text (found by parsing it with ExpensiMark htmToText function) to 'text' prop of the clipboard in here when canSetHtml is true( for web only, that's why it works fine for native apps).

} else if (content) {
const parser = new ExpensiMark();
if (!Clipboard.canSetHtml()) {
Clipboard.setString(parser.htmlToMarkdown(content));
} else {
const plainText = parser.htmlToText(content);
Clipboard.setHtml(content, plainText);
}
}

So when the user pastes in PrivatesNotesEditPage text input the plain text is copied by default as we have not implemented a code that pastes the html whenever the clipboard has html code inside it as we did in our Composer component.

What changes do you think we should make in order to solve the problem?

We need to take the code inside Composer component that handles paste of html code and reimplement it in a reusable code (custom hook or HOC) and then use it both in here privates notes and in composer.
So for instance we can use a custom hook like this and use it both in Private notes and Composer

function useCodePaste(textInputRef) {
    const navigation = useNavigation();

    /**
     * Set pasted text to clipboard
     * @param {String} text
     */
    const paste = useCallback((text) => {
        try {
            document.execCommand('insertText', false, text);
            // Pointer will go out of sight when a large paragraph is pasted on the web. Refocusing the input keeps the cursor in view.
            textInputRef.current.blur();
            textInputRef.current.focus();
            // eslint-disable-next-line no-empty
        } catch (e) {}
    }, []);

    /**
     * Manually place the pasted HTML into Composer
     *
     * @param {String} html - pasted HTML
     */
    const handlePastedHTML = useCallback(
        (html) => {
            const parser = new ExpensiMark();
            paste(parser.htmlToMarkdown(html));
        },
        [paste],
    );

    /**
     * Paste the plaintext content into Composer.
     *
     * @param {ClipboardEvent} event
     */
    const handlePastePlainText = useCallback(
        (event) => {
            const plainText = event.clipboardData.getData('text/plain');
            paste(plainText);
        },
        [paste],
    );

    const handlePaste = useCallback(
        (event) => {
            const isFocused = textInputRef.current.isFocused();

            if (!isFocused) {
                return;
            }

            if (textInputRef.current !== event.target) {
                // To make sure the text input does not capture paste events from other inputs, we check where the event originated
                // If it did originate in another input, we return early to prevent the text input from handling the paste
                const isTargetInput = event.target.nodeName === 'INPUT' || event.target.nodeName === 'TEXTAREA' || event.target.contentEditable === 'true';
                if (isTargetInput) {
                    return;
                }

                textInputRef.current.focus();
            }

            event.preventDefault();

            const {types} = event.clipboardData;
            const TEXT_HTML = 'text/html';

            // If paste contains HTML
            if (types.includes(TEXT_HTML)) {
                const pastedHTML = event.clipboardData.getData(TEXT_HTML);

                const domparser = new DOMParser();
                const embeddedImages = domparser.parseFromString(pastedHTML, TEXT_HTML).images;

                // Exclude parsing img tags in the HTML, as fetching the image via fetch triggers a connect-src Content-Security-Policy error.
                if (embeddedImages.length > 0 && embeddedImages[0].src) {
                    // If HTML has emoji, then treat this as plain text.
                    if (embeddedImages[0].dataset && embeddedImages[0].dataset.stringifyType === 'emoji') {
                        handlePastePlainText(event);
                        return;
                    }
                }
                handlePastedHTML(pastedHTML);
                return;
            }
            handlePastePlainText(event);
        },
        [handlePastedHTML, handlePastePlainText],
    );

    useEffect(() => {
        // we need to handle listeners on navigation focus/blur as Composer is not unmounting
        // when navigating away to different report
        const unsubscribeFocus = navigation.addListener('focus', () => document.addEventListener('paste', handlePaste));
        const unsubscribeBlur = navigation.addListener('blur', () => document.removeEventListener('paste', handlePaste));

        if (textInputRef.current) {
            document.addEventListener('paste', handlePaste);
        }

        return () => {
            unsubscribeFocus();
            unsubscribeBlur();
            document.removeEventListener('paste', handlePaste);
            // eslint-disable-next-line es/no-optional-chaining
            if (!textInputRef.current) {
                return;
            }
        };
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, []);
}

And in both PrivateNotesEditPage and Composer we call the custom hook passing the appropriate textInputRef parameter like this (or we can also return the ref from the custom hook and assign it to the ref of our text inputs):

    useCodePaste(privateNotesInput);

In Composer we change

const {files, types} = event.clipboardData;
const TEXT_HTML = 'text/html';
// If paste contains files, then trigger file management
if (files.length > 0) {
// Prevent the default so we do not post the file name into the text box
onPasteFile(event.clipboardData.files[0]);
return;
}
// If paste contains HTML
if (types.includes(TEXT_HTML)) {
const pastedHTML = event.clipboardData.getData(TEXT_HTML);
const domparser = new DOMParser();
const embeddedImages = domparser.parseFromString(pastedHTML, TEXT_HTML).images;
// Exclude parsing img tags in the HTML, as fetching the image via fetch triggers a connect-src Content-Security-Policy error.
if (embeddedImages.length > 0 && embeddedImages[0].src) {
// If HTML has emoji, then treat this as plain text.
if (embeddedImages[0].dataset && embeddedImages[0].dataset.stringifyType === 'emoji') {
handlePastePlainText(event);
return;
}
}
handlePastedHTML(pastedHTML);
return;
}
handlePastePlainText(event);
},
[onPasteFile, handlePastedHTML, checkComposerVisibility, handlePastePlainText],
);

to

            const {files} = event.clipboardData;

            // If paste contains files, then trigger file management
            if (files.length > 0) {
                // Prevent the default so we do not post the file name into the text box
                onPasteFile(event.clipboardData.files[0]);
                return;
            }
        },
        [onPasteFile, checkComposerVisibility],
    );

    useCodePaste(textInput);

(Optional) We can use also HOC (whichever is appropriate I am just trying to explain the basic idea).
*** We can also use HOC 👍 ***
Here is the result:

10.New.Expensify.1.mp4

What alternative solutions did you explore? (Optional)

@neonbhai
Copy link
Contributor

Cannot reproduce on latest main

@FitseTLT
Copy link
Contributor

Updated comment
And also I still can reproduce it in the latest main :-

3.New.Expensify.2.mp4

@melvin-bot melvin-bot bot added the Overdue label Oct 20, 2023
@garrettmknight
Copy link
Contributor

I was able to repro it on web via Chrome and also on the MacOS app.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 20, 2023
@garrettmknight
Copy link
Contributor

@ArekChr what do you think of that proposal?

@melvin-bot melvin-bot bot removed the Overdue label Oct 23, 2023
@ArekChr
Copy link
Contributor

ArekChr commented Oct 24, 2023

@FitseTLT, Could you provide more details about where the change should be applied?

@FitseTLT
Copy link
Contributor

@ArekChr we need to add this code to PrivateNotesEditPage and of course use withNavigation HOC on the PrivateNotesEditPage component exported.


    /**
     * Set pasted text to clipboard
     * @param {String} text
     */
    const paste = useCallback((text) => {
        try {
            document.execCommand('insertText', false, text);
            // Pointer will go out of sight when a large paragraph is pasted on the web. Refocusing the input keeps the cursor in view.
            textInput.current.blur();
            textInput.current.focus();
            // eslint-disable-next-line no-empty
        } catch (e) {}
    }, []);

    /**
     * Manually place the pasted HTML into Composer
     *
     * @param {String} html - pasted HTML
     */
    const handlePastedHTML = useCallback(
        (html) => {
            const parser = new ExpensiMark();
            paste(parser.htmlToMarkdown(html));
        },
        [paste],
    );

    /**
     * Paste the plaintext content into Composer.
     *
     * @param {ClipboardEvent} event
     */
    const handlePastePlainText = useCallback(
        (event) => {
            const plainText = event.clipboardData.getData('text/plain');
            paste(plainText);
        },
        [paste],
    );

    const handlePaste = useCallback(
        (event) => {
            const isFocused = privateNotesInput.current.isFocused();

            if (!isFocused) {
                return;
            }

            if (privateNotesInput.current !== event.target) {
                // To make sure the text input does not capture paste events from other inputs, we check where the event originated
                // If it did originate in another input, we return early to prevent the text input from handling the paste
                const isTargetInput = event.target.nodeName === 'INPUT' || event.target.nodeName === 'TEXTAREA' || event.target.contentEditable === 'true';
                if (isTargetInput) {
                    return;
                }

                privateNotesInput.current.focus();
            }

            event.preventDefault();

            const {types} = event.clipboardData;
            const TEXT_HTML = 'text/html';

            // If paste contains HTML
            if (types.includes(TEXT_HTML)) {
                const pastedHTML = event.clipboardData.getData(TEXT_HTML);

                const domparser = new DOMParser();
                const embeddedImages = domparser.parseFromString(pastedHTML, TEXT_HTML).images;

                // Exclude parsing img tags in the HTML, as fetching the image via fetch triggers a connect-src Content-Security-Policy error.
                if (embeddedImages.length > 0 && embeddedImages[0].src) {
                    // If HTML has emoji, then treat this as plain text.
                    if (embeddedImages[0].dataset && embeddedImages[0].dataset.stringifyType === 'emoji') {
                        handlePastePlainText(event);
                        return;
                    }
                }
                handlePastedHTML(pastedHTML);
                return;
            }
            handlePastePlainText(event);
        },
        [handlePastedHTML, handlePastePlainText],
    );

    useEffect(() => {
        // we need to handle listeners on navigation focus/blur as Composer is not unmounting
        // when navigating away to different report
        const unsubscribeFocus = navigation.addListener('focus', () => document.addEventListener('paste', handlePaste));
        const unsubscribeBlur = navigation.addListener('blur', () => document.removeEventListener('paste', handlePaste));

        if (privateNotesInput.current) {
            document.addEventListener('paste', handlePaste);
        }

        return () => {
            unsubscribeFocus();
            unsubscribeBlur();
            document.removeEventListener('paste', handlePaste);
            // eslint-disable-next-line es/no-optional-chaining
            if (!privateNotesInput.current) {
                return;
            }
        };
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, []);

@ArekChr
Copy link
Contributor

ArekChr commented Oct 24, 2023

@FitseTLT, I'm a bit confused. Previously, you mentioned that we don't handle pasted HTML like in the Composer, but now you suggest adding it to the PrivateNotesEditPage.
I'm unsure about the differences in the code you provided. It is similar to the code in Composer. Please update your proposal with the specific code changes and provide more details, especially regarding the exact file where these changes should be applied, by linking it in the proposal.

If you suggest using the same code from the Composer, we should create a reusable functionality instead of copying it to a different place.

@FitseTLT
Copy link
Contributor

FitseTLT commented Oct 24, 2023

@FitseTLT, I'm a bit confused. Previously, you mentioned that we don't handle pasted HTML like in the Composer, but now you suggest adding it to the PrivateNotesEditPage. I'm unsure about the differences in the code you provided. It is similar to the code in Composer. Please update your proposal with the specific code changes and provide more details, especially regarding the exact file where these changes should be applied, by linking it in the proposal.

If you suggest using the same code from the Composer, we should create a reusable functionality instead of copying it to a different place.

Yes, reusable code, that's exactly what I proposed under What alternative solutions did you explore? (Optional) @ArekChr .
Now I have updated my proposal with a working custom hook that can be used in both places. Thx!!

@melvin-bot
Copy link

melvin-bot bot commented Oct 24, 2023

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

@FitseTLT
Copy link
Contributor

@ArekChr did you check my updated proposal? I updated it with the custom hook implementation and result video but if it is better I can also implement the HOC implementation 👍 .

@FitseTLT
Copy link
Contributor

Updated comment

@ArekChr
Copy link
Contributor

ArekChr commented Oct 26, 2023

@FitseTLT I’m going to check your proposal again soon. I just need a bit more time to look into how code blocks can be pasted on web platforms to make sure your solution works properly.

@melvin-bot melvin-bot bot added the Overdue label Oct 30, 2023
@garrettmknight
Copy link
Contributor

@ArekChr will you be able to review this week?

@flodnv flodnv added Weekly KSv2 and removed Monthly KSv2 labels Jan 11, 2024
@flodnv
Copy link
Contributor

flodnv commented Jan 11, 2024

Thanks for the bump, done. Let me know if I misunderstood something. All yours @Santhosh-Sellavel 👍

@FitseTLT
Copy link
Contributor

@Santhosh-Sellavel will let u know when to review as the pr is too old will have to merge main and test

@garrettmknight
Copy link
Contributor

PR is being actively reveiwed.

@Santhosh-Sellavel
Copy link
Collaborator

@garrettmknight It's been deployed to production last week. Can you leave a payment summary please?

@garrettmknight garrettmknight added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Feb 13, 2024
@garrettmknight garrettmknight changed the title [$500] Private Note - Code block paste in private notes does not work (works fine in native apps) [Hold for payment 2024-02-13] [$500] Private Note - Code block paste in private notes does not work (works fine in native apps) Feb 13, 2024
@garrettmknight
Copy link
Contributor

garrettmknight commented Feb 13, 2024

Summarizing payments for this issue:

Reporter: @dhanashree-sawant $50 paid via Upwork
Contributor: @FitseTLT $250 (50% for the regression) paid via Upwork
Reviewer: @Santhosh-Sellavel $500 (since they didn't approve the original PR that resulted in a regression)

@garrettmknight

This comment was marked as resolved.

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Feb 19, 2024
@garrettmknight
Copy link
Contributor

@Santhosh-Sellavel please fill out the checklist when you get a chance and request payment. Dropping to weekly till then.

@melvin-bot melvin-bot bot removed the Overdue label Feb 21, 2024
@garrettmknight garrettmknight added Weekly KSv2 and removed Daily KSv2 labels Feb 21, 2024
@melvin-bot melvin-bot bot added the Overdue label Mar 1, 2024
@garrettmknight
Copy link
Contributor

Just waiting on checklist and payment.

@melvin-bot melvin-bot bot removed the Overdue label Mar 1, 2024
@melvin-bot melvin-bot bot added the Overdue label Mar 11, 2024
@garrettmknight
Copy link
Contributor

@Santhosh-Sellavel can you request payment when you get a chance? The checklist was completed in the original issue.

@melvin-bot melvin-bot bot removed the Overdue label Mar 11, 2024
@Santhosh-Sellavel
Copy link
Collaborator

Feel free to close this one, I'll request later

@Santhosh-Sellavel
Copy link
Collaborator

Requested on ND

@JmillsExpensify
Copy link

$500 approved for @Santhosh-Sellavel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests