Closed mvtglobally closed 2 years ago
Triggered auto assignment to @iwiznia (Engineering
), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
Triggered auto assignment to @adelekennedy (External
), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
We have to change how we strip html when copying to clipboard, at the moment we are only replacing everything within <> for an empty string here: https://github.com/Expensify/expensify-common/blob/f77bb4710c13d01153716df7fb087b637ba3b8bd/lib/str.js#L306-L318
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported
)
Triggered auto assignment to @puneetlath (Exported
), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Thanks for the proposal @mateusbra. Could you please root cause details to your proposal? and tell us how did you reach your solution.
We need to change how we handle pasted HTML here.
Change this
To
Only strip off valid html tags
/**
* Gets the textual value of the given string.
*
* @param {String} str The string to fetch the text value from.
* @return {String} The text from within the HTML string.
*/
stripHTML(str) {
if (!this.isString(str)) {
return '';
}
return str.replace(/<\/?(a|abbr|acronym|address|applet|area|article|aside|audio|b|base|basefont|bdi|bdo|bgsound|big|blink|blockquote|body|br|button|canvas|caption|center|cite|code|col|colgroup|data|datalist|dd|del|details|dfn|dir|div|dl|dt|em|embed|fieldset|figcaption|figure|font|footer|form|frame|frameset|h1|h2|h3|h4|h5|h6|head|header|hgroup|hr|html|i|iframe|img|input|ins|isindex|kbd|keygen|label|legend|li|link|listing|main|map|mark|marquee|menu|menuitem|meta|meter|nav|nobr|noframes|noscript|object|ol|optgroup|option|output|p|param|plaintext|pre|progress|q|rp|rt|ruby|s|samp|script|section|select|small|source|spacer|span|strike|strong|style|sub|summary|sup|table|tbody|td|textarea|tfoot|th|thead|time|title|tr|track|tt|u|ul|var|video|wbr|xmp|path|svg|...)\b[^<>]*>/g, '');
},
SCREEN RECORDING
https://user-images.githubusercontent.com/4717301/155875274-e12f58b4-d934-4a93-b3b7-bfe7df2cbc84.mov
Thanks for the proposal @dennismunene. Did you try?
`<someComponent />`
as message.
@parasharrajat yes I have , works as expected.
Ok. Thanks but I don't consider this a proper approach.
const parser = new ExpensiMark();
const markdownText = parser.htmlToMarkdown(html);
This step is important for the app.
@parasharrajat updated my proposal.
Thanks, @dennismunene but I still think we can do better.
Now there are two things to compare.
Ctrl+C
. Copy to clipboard works. But the selection does not. So it must have given you the idea by now what is wrong.
Thanks for the proposal @mateusbra. Could you please root cause details to your proposal? and tell us how did you reach your solution.
@parasharrajat thank you for the review. I think this is gonna be more complex than I thought, please ignore my proposal by now
Current assignee @parasharrajat is eligible for the Exported assigner, not assigning anyone new.
Current assignee @adelekennedy is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to @marcaaron (Exported
), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@marcaaron I'm about to head out on my two-month sabbatical, so handing this off to you. Thanks!
Still waiting for a proposal.
Doubling the issue!
Not overdue - just doubled the issue 4 days ago
@parasharrajat, @marcaaron, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!
π¦
Still no volunteers π¬ we're a day out from me doubling the issue
π€ doubled π€
Melv needs a spa day
Maybe this should be weekly
π
?
In case someone is looking to solve this, https://github.com/Expensify/App/issues/7912#issuecomment-1053609946 indicates what needs to be done.
We're at the mark to double the issue again! @parasharrajat you've pointed out how this issue can be resolved above @mateusbra and @dennismunene any interest in giving a proposal another go?
Hi,
I just looked into this issue now. As mentioned in the previous comment, stripHTML function from expensify-common library strips out the text within <>.
https://github.com/Expensify/expensify-common/blob/main/lib/str.js#L312
Modifying the regex works fine. The video is attached below:
I know the above solution works. but what if we take this opportunity to build a text formatting toolbar like slack(Video attached below):
With this approach basically, We would replace the
Edit: @brianmuks seems to have nailed it below. So retracting my proposal.
Proposal
The issue is coming from this line :
https://github.com/Expensify/App/blob/b347866b2af95bd2c51eb6bb65098bd1d756e7d0/src/libs/SelectionScraper/index.js#L135
Wrong html content is being passed to parser.htmlToMarkdown(newHtml);
If you compare with what is being passed to :
https://github.com/Expensify/App/blob/b347866b2af95bd2c51eb6bb65098bd1d756e7d0/src/pages/home/report/ContextMenu/ContextMenuActions.js#L69
When a user clicks on copy to cliboard
for this code block : "<someComponent />
" what is being passed to parser.htmlToMarkdown(newHtml);
is '<code><someComponent /></code>'
. This is similar to the data coming from :
https://github.com/Expensify/App/blob/b347866b2af95bd2c51eb6bb65098bd1d756e7d0/src/libs/actions/Report.js#L946
html: '<code><someComponent /></code>'
With that said in order to pass the right format to parser.htmlToMarkdown(newHtml);
when a user presses CTR+C
we need to know what the original text looked like . To achieve this we need a new function to generate the text:
const getTextFromSelection = (selectionHtml) => {
const div = document.createElement('div');
div.innerHTML = selectionHtml;
//get the last child from parent node
let lastNode = div.lastChild;
let innerHTML = '';
//loop through the node until we get the content of the lastNode which has the original text
while (lastNode && lastNode.hasChildNodes) {
innerHTML = lastNode.innerHTML ? lastNode.innerHTML : innerHTML;
lastNode = lastNode.lastChild;
}
return innerHTML;
}
The results of getTextFromSelection
is " <someComponent />"
So all we are missing here is just a wrapper which can be obtained from the results of :
https://github.com/Expensify/App/blob/b347866b2af95bd2c51eb6bb65098bd1d756e7d0/src/libs/SelectionScraper/index.js#L131
the results of render(domRepresentation);
is "<code><span><someComponent /></span></code>"
At this point we just need to replace "<someComponent />"
with the results from getTextFromSelection
.
const newHtmlWithText = newHtml.replace(selection, originalText);
Now we can pass newHtmlWithText
to parser.htmlToMarkdown(newHtmlWithText);
.
The modification of getAsMarkdown
will be :
const getAsMarkdown = () => {
const selectionHtml = getHTMLOfSelection();
const selection = window.getSelection();
const domRepresentation = parseDocument(selectionHtml);
domRepresentation.children = _.map(domRepresentation.children, c => replaceNodes(c));
const newHtml = render(domRepresentation);
const originalText = getTextFromSelection(selectionHtml);
console.log(':domRepresentation', newHtml);
//replace the the rendered text with the original text
const newHtmlWithText = newHtml.replace(selection, originalText);
const parser = new ExpensiMark();
return parser.htmlToMarkdown(newHtmlWithText);
};
https://user-images.githubusercontent.com/4520893/160298441-5eb4cd87-012d-4d41-8441-e38534a5eebf.mov
Fun-Fact π : I had a hard time typing this because even github
renders "<code><someComponent /></code>"
as ""
.
https://user-images.githubusercontent.com/4520893/160299224-dcb3c348-a936-4621-a163-70524fb9bed2.mov
Edit: @brianmuks seems to have nailed it below. So retracting my proposal.
Thanks @barun1997
@barun1997 Thanks for the proposal. Could be please test your proposal against the following:
Paste this in composer and send as a message.
> This is a quote
> Mul
*Bold* _italic_ https://google.com
`yo yo code block` *bold inline*
On which fact are you proposing the string replacement as a fix? What is the base of this solution?
const originalText = getTextFromSelection(selectionHtml);
console.log(':domRepresentation', newHtml);
//replace the the rendered text with the original text
const newHtmlWithText = newHtml.replace(selection, originalText);
Why does doing this solve the problem? Will it work in general with every tag?
Edited: the function returns text not html. Thanks @parasharrajat
My solution does not cover this scenario:
"Sentence with <code/> in it"
So I though of some minor modifications:
const getAsMarkdown = () => {
const selectionHtml = getHTMLOfSelection();
const originalText = getTextFromSelection(selectionHtml);
// const domRepresentation = parseDocument(selectionHtml);
// domRepresentation.children = _.map(domRepresentation.children, c => replaceNodes(c));
// const newHtml = render(domRepresentation);
const parser = new ExpensiMark();
return parser.htmlToMarkdown(originalText);
};
const getTextFromSelection = (selectionHtml) => {
const div = document.createElement('div');
div.innerHTML = selectionHtml;
let node = div;
let text = '';
let counter = 0;
//loop through the node until we get the content of the lastNode which has the original text
while (node && node.hasChildNodes) {
//ignore the content of first child as experiment shows that the main parent's
//content is equal to the first child's content
if (node && node.children && node.children.length && counter !== 1) {
// console.log('nodeChildrenHasChild:' + node.children);
var children = [].slice.call(node.children);
children.forEach(child => {
const container = { ...child.dataset }.testid;
//if child is a code component wrap it in its original component
text += container ? `<${container}>${child.innerHTML}</${container}>` : child.innerHTML;
});
}
node = node.lastChild;
counter++;
}
return text;
}
Could you please help with answers to my questions as well? Also, small(any) details will help me understand your plan. https://github.com/Expensify/App/issues/7912#issuecomment-1080342870 does not tell what are you trying to establish.
The previous proposal suggests that you are proposing string replacement as a solution and in the new code snippet, your function getTextFromSelection
is returning HTML. Why?
Although you are looking at the correct place it seems you may have been distracted from the main solution here. I will suggest finding another simpler approach to the problem.
Could you please help with answers to my questions as well? Also, small(any) details will help me understand your plan. #7912 (comment) does not tell what are you trying to establish.
The previous proposal suggests that you are proposing string replacement as a solution and in the new code snippet, your function
getTextFromSelection
is returning HTML. Why?Although you are looking at the correct place it seems you may have been distracted from the main solution here. I will suggest finding another simpler approach to the problem.
So what we are trying to achieve here is to extract/determine the original text that was saved to the database as html
.
The copy to clipboard
function basically uses the html
content and passes it to parser.htmlToMarkdown(html);
then writes it to the clipboard. So I feel that the function handling CTR+C
should be as similar as possible to the
copy to clipboard
function.
If there was an easy way of extracting/determining the original html
fetched from the database for a message from getHTMLOfSelection();
then we could use that.
By the way copy to clipboard
fails to copy quotes properly. So I was wondering if we want to fix that all at once because I reported a bug for it few days ago.
https://user-images.githubusercontent.com/4520893/160365106-2f28baf3-49b4-4657-90a2-f9c7fa1d437a.mov
So far my latest solution works for tags . I am still looking at how to handle scenarios you asked me to test.
"> This is a quote"
"> Mul"
"*Bold* _italic_ https://google.com"
"
yo yo code block*bold inline*"
https://user-images.githubusercontent.com/4520893/160367877-cfbed48f-db44-4a52-b28d-9a74adaea9e2.mov
So what we are trying to achieve here is to extract/determine the original text that was saved to the database as html.
Thanks for the explanation. It would be technically impossible with the current implementation of message parsing. Also, it does not seem like an elegant solution.
text += container ?
<${container}>${child.innerHTML}</${container}>
: child.innerHTML;
It does sound like HTML.
By the way copy to clipboard fails to copy quotes properly. So I was wondering if we want to fix that all at once because I reported a bug for it a few days ago.
we can only focus on solving the code blocks in a way that does not block or break others.
in src/libs/SelectionScraper :
const replaceNodes = (dom) => {
+ if (dom.type == "text") {
+ dom.data = dom.data.replaceAll('<', '<').replaceAll('>', '>')
+ }
let domName = dom.name;
Result :
Thanks for the proposal @gladiator-1. It seems to do what we need here. Could you please confirm if we can use Str.htmlEncode
here instead of manual replacement?
@parasharrajat Sure
import {parseDocument} from 'htmlparser2';
import {Element} from 'domhandler';
import _ from 'underscore';
+ import Str from 'expensify-common/lib/str';
const replaceNodes = (dom) => {
+ if (dom.type == "text") {
+ dom.data = Str.htmlEncode(dom.data)
+ }
let domName = dom.name;
Could you please confirm if we can use
Str.htmlEncode
here instead of a manual replacement?
Is it working fine without breaking other tags?
Is it working fine without breaking other tags?
Yes
Thanks, I was really looking for a simpler approach like @gladiator-1 has presented.
One improvement would be to do this manipulation at the parsing step onText.
Thanks, @barun1997 and @brianmuks for your efforts in looking into getting a proper solution. But I like @gladiator-1's simpler approach to the problem. https://github.com/Expensify/App/issues/7912#issuecomment-1081094208 https://github.com/Expensify/App/issues/7912#issuecomment-1081733070
cc: @marcaaron
:ribbon: :eyes: :ribbon: C+ reviewed
One improvement would be to do this manipulation at the parsing step onText.
Excuse me, Is it possible to update the parsed document object at the parsing step ?
const domRepresentation = parseDocument(selectionHtml)
I looked at the wrong name. It seems not, then your proposal is fine.
@gladiator-1 I'm struggling to find you in Upwork, do you mind applying for the job here?
@adelekennedy applied with code : 75340
π£ @gladiator-1 You have been assigned to this job by @adelekennedy! 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 π
@parasharrajat PR ready https://github.com/Expensify/App/pull/8449
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:
Expected Result:
Exactly what you copied should be pasted
Actual Result:
The code gets stripped Issue happens when selecting with a mouse, not when using the Copy button
Workaround:
unknown
Platform:
Where is this issue occurring?
Version Number: 1.1.40-0 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: Any additional supporting documentation
https://user-images.githubusercontent.com/43995119/155665755-5ae5607a-9621-4cb0-bf88-3606dd32f313.mov
Expensify/Expensify Issue URL: Issue reported by: @Beamanator Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1644856333330159
View all open jobs on GitHub