Skip to content

Ignore images from embedded html when pasting, paste plaintext instead. #21476

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Aug 7, 2023
Merged

Ignore images from embedded html when pasting, paste plaintext instead. #21476

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
782b48f
Ignore images from embedded html when pasting, paste plaintext instead.
qispark Jun 23, 2023
fe399ad
Reinstate paste plaintext for emoji, and otherwise paste HTML
qispark Jul 5, 2023
1bc59ae
Merge remote-tracking branch 'origin/main' into qispark-composer-past…
qispark Aug 3, 2023
4f99a5c
Apply suggestions from code review
qispark Aug 3, 2023
a6cbd61
Remove unused variables to pass lint
qispark Aug 7, 2023
5b99ed4
Merge remote-tracking branch 'robin/qispark-composer-paste-ignore-ima…
qispark Aug 7, 2023
cf43831
Update src/components/Composer/index.js
qispark Aug 7, 2023
ebb3932
Update prettier
qispark Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 14 additions & 38 deletions src/components/Composer/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,16 @@ class Composer extends React.Component {
this.paste(parser.htmlToMarkdown(html));
}

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

/**
* Check the paste event for an attachment, parse the data and call onPasteFile from props with the selected file,
* Otherwise, convert pasted HTML to Markdown and set it on the composer.
Expand All @@ -308,52 +318,18 @@ class Composer extends React.Component {
const domparser = new DOMParser();
const embeddedImages = domparser.parseFromString(pastedHTML, TEXT_HTML).images;

// If HTML has img tag, then fetch images from it.
// If HTML has img tag, then we only paste the plain text. This is because
// fetching the image via fetch triggers a 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') {
const plainText = event.clipboardData.getData('text/plain');
this.paste(plainText);
return;
}
fetch(embeddedImages[0].src)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to revert the logic for .stringifyType === 'emoji' and for the rest pasteHTML. let's do that.

.then((response) => {
if (!response.ok) {
throw Error(response.statusText);
}
return response.blob();
})
.then((x) => {
const extension = IMAGE_EXTENSIONS[x.type];
if (!extension) {
throw new Error(this.props.translate('composer.noExtensionFoundForMimeType'));
}

return new File([x], `pasted_image.${extension}`, {});
})
.then(this.props.onPasteFile)
.catch(() => {
const errorDesc = this.props.translate('composer.problemGettingImageYouPasted');
Growl.error(errorDesc);

/*
* Since we intercepted the user-triggered paste event to check for attachments,
* we need to manually set the value and call the `onChangeText` handler.
* Synthetically-triggered paste events do not affect the document's contents.
* See https://developer.mozilla.org/en-US/docs/Web/API/Element/paste_event for more details.
*/
this.handlePastedHTML(pastedHTML);
});
this.handlePastePlainText(event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we not pasting the HTML? embeddedImages[0].dataset && embeddedImages[0].dataset.stringifyType === 'emoji' require pasting text but other than this we can use html pasting...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat If you read my proposal carefully, I explain the caveat of why we should paste plaintext instead of pasting the HTML (#18583 (comment))

If any images have links attached, and we try to paste them, it leads to formatting issues in the markdown that is generated.

Please let me know what is the desired behaviour in this instance as I thought you already approved my proposal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did approve the removal of fetching of images but the whole formatting. Can you share a screenshot/video of formatting issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what happens when I set it to paste HTML:

Screen.Recording.2023-06-27.at.8.40.55.am.mov

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay. But I think we need to keep formatting. It is strange that image tags are converted to links. I am not sure where that happens in out parser.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qispark If images are wrapped in a link then it is fine if they are parsed as links in the pasted code.

But if the image itself is being converted to links then that is a problem. If we want to solve this problem, then we should be modifying the link rules in ExpensiMark to filter out images.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat Let me know if I should action on anything based on your feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@techievivek cc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qispark Based on Rajat's comment, If images are wrapped in a link then it is fine if they are parsed as links in the pasted code. I think the image was wrapped in a link in the recording above, right? (Can you confirm this by inspecting that element?) and when we are pasting it is being rendered as links.

@parasharrajat

But if the image itself is being converted to links then that is a problem

I don't think that is happening in this case. @qispark can you please confirm this?

And now for the last part

Why are we not pasting the HTML? embeddedImages[0].dataset && embeddedImages[0].dataset.stringifyType === 'emoji' require pasting text but other than this we can use html pasting.

@qispark What do you think can be done to fix this? Do we need to update the ExpensiMark logic? I think that is very much doable, so if that's the case please push a PR in the expensify-common repo and let's keep this moving.

Copy link
Contributor Author

@qispark qispark Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@techievivek Your understanding is correct.
When the image is wrapped in a link, it gets rendered as an empty link.
When the image is not wrapped in a link, it gets rendered as its alt text.

I think we can update the ExpensiMark logic based on what I mentioned in my last comment. Happy to make a PR there to reflect this functionality. I will share an update when this is done.

return;
}

this.handlePastedHTML(pastedHTML);
return;
}

const plainText = event.clipboardData.getData('text/plain');

this.paste(plainText);
this.handlePastePlainText(event);
}

/**
Expand Down