Copying a rendered issue/comment will paste into GFM textareas as actual GFM

This is awesome! I see one regression: if I copy a long piece of text from a comment (longer than the comment textarea initially), the textarea doesn't resize, and isn't scrollable, until my next input in that textarea. That's not the case at the moment.

I also think this should only happen if you copy within a single GFM container. At the moment, copying across a comment boundary (for instance) will also copy the next commenter's avatar etc. as GFM. While that's cool, I think it's unexpected - but it's a pretty minor issue.

Ah, one more thing: could HTML elements that are allowed in Markdown, but don't have special syntax (like kbd), be passed through using outerHTML or whatever? We'd still sanitize it when rendering, after all.

@smcgivern ~~I am g2g~~. ~~Feel free to merge when you are ready. ~~

Nice job @DouweM, Mr JavaScript

document.querySelector will run 40 times when you press copy. This is my only qualm.

@jschatz1 I don't think that's true. We run node.matches(selector) 1..40 times per node, to find the first selector that matches, and this is always in the context of a small document fragment (covering the selected text), not the full document. The performance impact in such a small document is negligible.

I see one regression: if I copy a long piece of text from a comment (longer than the comment textarea initially), the textarea doesn't resize, and isn't scrollable, until my next input in that textarea.

@smcgivern Yeah, I saw that too. I added $(target).trigger('input'); when inserting the text, but that doesn't help. ~~@jschatz1 Any ideas?~~ Fixed it!

I also think this should only happen if you copy within a single GFM container. At the moment, copying across a comment boundary (for instance) will also copy the next commenter's avatar etc. as GFM. While that's cool, I think it's unexpected - but it's a pretty minor issue.

Yep, saw that too. I'll see if there's anything I can do about it! Edit: Done!

Ah, one more thing: could HTML elements that are allowed in Markdown, but don't have special syntax (like kbd), be passed through using outerHTML or whatever? We'd still sanitize it when rendering, after all.

Let me have a look at that :) Edit: Done!

mentioned in issue #26860 (closed)

added 4 commits

359c4176 - Properly copy/paste allowed HTML
9b800bc6 - Remove unneeded code
b6ac5332 - Don't copy as GFM when more than GFM is selected
dd6f91cd - Trigger autosize on the textarea after pasting

Compare with previous version

added 1 commit

f928e19c - Remove unneeded checks

Compare with previous version

@smcgivern Would you like to review the backend?

assigned to @smcgivern

added 173 commits

f928e19c...52762df2 - 172 commits from branch master
2b37e4c1 - Merge branch 'master' into copy-as-md

Compare with previous version

resolved all discussions

@DouweM Did you check this for iphone and android. I see a few JS things that aren't supported on mobile browsers that could have unforseen results. I can't check right now. If not we need to gracefully degrade.

@DouweM handing over frontend reviewing to @mikegreiling

changed milestone to %8.17

removed ~149423 label

I'll be going to go through the code changes in more depth soon, but just playing around with it a bit right now.

First of all, awesome feature . I can't wait to get this one in.

That said, I've noticed some small issues.

I couldn't get tables to copy/paste quite right.
Also lists which contain blockquotes or other indented items were somewhat flaky.

source:

copypasta:

result:

still, even with these issues the output is improved from what it would have been without the attempt at "markdownifying" it, so I wouldn't be opposed to implementing an imperfect copy translator.

@mikegreiling Good catches! But I'm a perfectionist so I'll fix the table and nested blockquote cases :D

assigned to @DouweM

Here's the source markdown from my test:

## Test header 1

Test header 2
==========

here's a paragraph of text  
and here's some attachments:

![10696169_10202947311905447_735692905731778_n](/uploads/d1c8e08691510d83bb30a30a3f448c64/10696169_10202947311905447_735692905731778_n.jpg)

* list item 1
* list [item 2](http://google.com/)
* list item 3
* list item 4

```javascript
function helloWorld () {
  alert("hello world!");
}
```
|       |   x   |   y   |
|-------|-------|-------|
|   a   |   1   |   0   |
|   b   |   0   |   1   |

here's another thingie

> and here's a block quote!

* here's a list item
* and another list item

    > with a block quote inside it!!

* wut??

@mikegreiling All fixed!

added 1 commit

d89f5616 - Improve support for linebreaks, tables and nested blockquotes in lists

Compare with previous version

added 1 commit

bd2880bb - Improve handling of code blocks containing triple backticks

Compare with previous version

assigned to @mikegreiling

resolved all discussions

added 262 commits

bd2880bb...8c9a06c3 - 260 commits from branch master
79440890 - Merge branch 'master' into copy-as-md
43dc6263 - Run tests in a single browser session

Compare with previous version

assigned to @smcgivern

resolved all discussions

@DouweM this is good to go, but there are conflicts and I'm not sure how they should be resolved. @mikegreiling can you help, please? Once those are fixing, @DouweM feel free to set MWPS!

assigned to @mikegreiling

@smcgivern and @DouweM please wait to get the green light from @mikegreiling before merging.

This doesn't appear to function in IE11. I don't think that would block us from shipping as long as it gracefully falls back to the old style of copy/paste, but I'm spending some time investigating why right now.

As long as it doesn't screw up IE11.

added 87 commits

43dc6263...b55c1bc4 - 86 commits from branch master
21755ac9 - Merge branch 'master' into copy-as-md

Compare with previous version

Alright, this all LGTM

There are a handful of eslint violations to fix up: no-param-reassign on three lines, and once case of no-multiple-empty-lines.

I wish we didn't need to disable so many linter rules on this file, but the ones you've disabled seem sensible for now.

The IE11 thing is fine. It gracefully degrades to the normal copy/paste behavior when Event.clipboardData isn't found. I understand IE exposes this as document.clipboardData but it has other compatibility issues as well and I don't think it would be worth the time to get this running in a legacy browser.

Should be good to merge once the linter errors are addressed and once the builds pass.

Awesome work! I've definitely encountered times where this would have been helpful in the past... I'm excited to see this one merged.

assigned to @DouweM

added 1 commit

6c2d8f35 - Statisfy eslint

Compare with previous version

@mikegreiling eslint satisfied :) Merge if you're happy!

assigned to @mikegreiling

enabled an automatic merge when the pipeline for 6c2d8f35 succeeds

merged

mentioned in commit a24e9a0e

mentioned in merge request !9080 (merged)

mentioned in merge request !9874 (merged)

mentioned in merge request !9876 (merged)

mentioned in issue #29732 (closed)

mentioned in issue #29788 (moved)

mentioned in issue #31193 (closed)

mentioned in issue #32546 (moved)

mentioned in issue gitlab#9964

Admin message

Admin message

Copying a rendered issue/comment will paste into GFM textareas as actual GFM

Merge request reports

Activity

source:

copypasta:

result: