springload / draftail

πŸ“πŸΈ A configurable rich text editor built with Draft.js
https://www.draftail.org/
MIT License
616 stars 64 forks source link

feature request: idempotent copy/paste behaviour #147

Closed Pomax closed 6 years ago

Pomax commented 6 years ago

Right now it is impossible to copy and paste the content of one draftail editor into another. It seems fairly important (I'm tempted to say crucial?) to make sure that a copy action puts all the data in the clipboard that is necessary to properly paste it back in, but I haven't dug into the code and have no idea how much work that would be. Hopefully not so much as to make it a low priority job =)

STR that clearly demonstrate copy/paste failing:

expected result: the original content actual result: loads of markup has disappeared (links are gone, hr are gone, empty lines are gone both inside and outside of code blocks)

Does draftail use the clipboard API to make sure that a copy action uses the true markup as referenced by the renderer, rather than a derivative markup based on what the browser rendered?

thibaudcolas commented 6 years ago

Hey @Pomax,

Copy-paste (not) being idempotent is definitely a bug, but unfortunately my answer will be the same as https://github.com/wagtail/wagtail/issues/4432#issuecomment-377624630 (this is the exact same issue) – Fixing these would require overriding Draft.js copy-paste handling within Draftail with a new implementation, which I'd love to do but likely won't have time for in the foreseeable future. (or fixing those bugs directly in Draft.js).

It's definitely not low priority, but at this point in time I'm the only regular contributor to the client-side parts of the "rich text in Wagtail" work. Over the last 5 months I've only been doing this in my free time, so even things that are the most urgent tend to take longer than ideal. For copy-paste in particular, I decided to focus on preventing unwanted formatting from getting in (https://github.com/thibaudcolas/draftjs-filters), and decided to leave out the "preserve wanted formatting as-is" troubleshooting for later.

You can see all of the other issues I'm aware of and decided to not prioritise here: #138


Back to solving this – my thoughts are that I would like to avoid forking Draft.js at all costs (maintenance hell), and would also rather override as little of its paste processing as possible. Here is what I would like to try: https://github.com/facebook/draft-js/issues/787#issuecomment-263132062

Roughly the steps are:

    function addExtraData(e) {
        var clipboardData = e.clipboardData;

        var someObj = {
            "test": { "test": "test" }
        }

        var selection = window.getSelection()

        console.log("called addExtraData")
        console.log(selection)

        e.clipboardData.setData('text/plain', selection)
        e.clipboardData.setData('test', JSON.stringify(someObj))

        e.preventDefault()
    }

    function getExtraData(e) {
        var data = e.clipboardData.getData('text/plain')
        var data2 = e.clipboardData.getData('test')
        console.log("called getExtraData")
        console.log(data)
        console.log(JSON.parse(data2))
    }

    document.addEventListener('copy', addExtraData);
    document.addEventListener('paste', getExtraData);

Alternatively I would love to attempt to contribute a fix for this directly in Draft.js, but I would expect this to take even more time (both in work involved, and timespan), so can't reasonably do this in my free time.

Pomax commented 6 years ago

Hi, @thibaudcolas, thanks for taking the time to do that detailed writeup, that will be super useful information for anyone who might want to jump in to help. I've also filed https://github.com/facebook/draft-js/issues/1731, because maybe the draftjs folks had this on their radar as well and a fix might be closer than we think.

mzedeler commented 6 years ago

Note that Draft.js maintains an internal clipboard in addition to the normal clipboard. I got surprised a few times because of this.

Pomax commented 6 years ago

Oh that's really good to know, do you have any code or docs that we can add a link to in this issue for future reference?

Pomax commented 6 years ago

@mzedeler didn't realise you were already potentially working on this - are you still planning to tackle this on the draftjs side?

thibaudcolas commented 6 years ago

@mzedeler as far as I know the internal clipboard you're referring to is set per-editor (paste from an editor to itself without a page reload is already idempotent). For pastes between editors, Draft.js uses its normal paste processor that uses convertFromHTML (https://github.com/facebook/draft-js/blob/4c4465f6c05b6dbb9eb769f98e659f917bbdc0f6/src/component/handlers/edit/editOnPaste.js#L154, https://draftjs.org/docs/api-reference-data-conversion.html)

thibaudcolas commented 6 years ago

I made a quick hack (https://github.com/springload/draftail/pull/148) that partly addresses this, but only for editors that are on the same page, and not if the page is reloaded. See the PR for more details.

Pomax commented 6 years ago

that's already a super useful hack, awesome work @thibaudcolas!

mzedeler commented 6 years ago

@Pomax - I expect to write my own copy/paste handlers for an extension of the Draft editor that I'm working on at the moment (not currently Open Source). Anyway, I still think that it would be useful if Draft didn't have an internal clipboard, @thibaudcolas. It is really strange when you're debugging Draft and you can see that it is pasting something else than what is on the clipboard. Of course the replacement should have the same features as the current implementation has.

thibaudcolas commented 6 years ago

This issue is quite popular amongst my Patreon patrons (https://www.patreon.com/posts/may-poll-first-18494448) so I've been working on a fix that's as good as possible.

Since the initial PR #148 I found two other approaches that are more promising, one in particular which seems to fix the issue completely (no limitation of "editors have to be on the same page" or anything, it just works everywhere).

You can follow along over at https://github.com/thibaudcolas/draftjs-conductor/pull/2 if you want to know more or help out. I moved this to a separate repository because fixing this issue isn't specific to Draftail, and I'd rather make the code easier to reuse for the wider Draft.js community.

thibaudcolas commented 6 years ago

155 completely fixes this (except for IE11, where rich text copy-paste isn't supported to start with).

Once merged, we can update the copy-paste part of #138:

-* Sequential unstyled blocks are merged into the same block on paste https://github.com/facebook/draft-js/pull/927, https://github.com/facebook/draft-js/issues/738, https://github.com/facebook/draft-js/issues/523, https://github.com/facebook/draft-js/issues/1082
* Rich text pasting creates an empty block at the beginning of the pasted content https://github.com/facebook/draft-js/pull/1052
* Pasting `<pre><code>` should not add the CODE inline style https://github.com/facebook/draft-js/issues/394
* Copy pasting from one code block to another removes line breaks https://github.com/facebook/draft-js/issues/407
# TODO: double check, not sure this was an issue to start with
-* Edge copy and paste issue (and IE11) https://github.com/facebook/draft-js/issues/659
* Copying text in IE adds extra new lines https://github.com/facebook/draft-js/issues/615, https://github.com/facebook/draft-js/issues/1448
* Cannot paste HTML in IE https://github.com/facebook/draft-js/issues/986
* Caret focus position incorrect after pasting text in IE10/11 https://github.com/facebook/draft-js/issues/612
* Cannot paste text with angle brackets in IE https://github.com/facebook/draft-js/issues/656
-* Copy/paste of entities between editors https://github.com/facebook/draft-js/issues/787
-* Copy/paste between editors strips soft returns https://github.com/facebook/draft-js/issues/1154
# TODO: should be solved, double check
-* Minor issue with copy-pasting rich text styles https://github.com/facebook/draft-js/issues/1163
# TODO: should be solved, double check
-* Styles/colors don't paste in IE/Edge https://github.com/facebook/draft-js/issues/1164
# TODO: should be solved, double check
-* Extra newlines added to text pasted between two Draft editors. https://github.com/facebook/draft-js/issues/1389
* convertFromHTML inserts paragraphs in place of newline https://github.com/facebook/draft-js/issues/1426

…and once this reaches Wagtail do the same over at https://github.com/wagtail/wagtail/issues/4274.

thibaudcolas commented 6 years ago

This should now be testable over at https://www.draftail.org/examples/. For example, copying the whole content of "Custom formats" into "Wagtail features" should preserve the embed, and document link.

Pomax commented 6 years ago

Fantastic! I will give it a go!

thibaudcolas commented 6 years ago

Now PR-ed into Wagtail: https://github.com/wagtail/wagtail/pull/4582 πŸŽ‰