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

feature request: idempotent copy/paste behaviour #147

Closed
Pomax opened this issue Apr 16, 2018 · 14 comments
Closed

feature request: idempotent copy/paste behaviour #147

Pomax opened this issue Apr 16, 2018 · 14 comments
Labels
bug Something isn't working
Milestone

Comments

@Pomax
Copy link

Pomax commented Apr 16, 2018

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:

  • load up https://www.draftail.org/
  • copy the editor content to clipboard
  • reload the page
  • wipe the editor and paste the clipboard

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
Copy link
Collaborator

thibaudcolas commented Apr 17, 2018

Hey @Pomax,

Copy-paste (not) being idempotent is definitely a bug, but unfortunately my answer will be the same as wagtail/wagtail#4432 (comment) (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: facebookarchive/draft-js#787 (comment)

Roughly the steps are:

  • Learn more about the Draft.js handlePastedText handler
    • See if it can be easily overriden, detecting a "Draft.js source"
    • and whether we can reuse its built-in implementation for the non-Draft.js content sources (eg. Word)
  • If it's not doable, try to do the same with a React onPaste / onCopy
  • If that doesn't work, use global event handlers like shown below (taken from the above issue)
    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
Copy link
Author

Pomax commented Apr 17, 2018

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 facebookarchive/draft-js#1731, because maybe the draftjs folks had this on their radar as well and a fix might be closer than we think.

@mzedeler
Copy link

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

@Pomax
Copy link
Author

Pomax commented Apr 18, 2018

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
Copy link
Author

Pomax commented Apr 18, 2018

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

@thibaudcolas
Copy link
Collaborator

thibaudcolas commented Apr 18, 2018

@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
Copy link
Collaborator

I made a quick hack (#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
Copy link
Author

Pomax commented Apr 19, 2018

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

@mzedeler
Copy link

@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
Copy link
Collaborator

thibaudcolas commented May 20, 2018

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 thibaudcolas/draftjs-conductor#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 thibaudcolas added this to the v1.0.0 milestone May 24, 2018
thibaudcolas added a commit that referenced this issue Jun 3, 2018
…#147

This makes Draftail always preserve the full content as-is when copy-pasting between editors. See also thibaudcolas/draftjs-conductor#2.
@thibaudcolas
Copy link
Collaborator

#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 wagtail/wagtail#4274.

thibaudcolas added a commit that referenced this issue Jun 4, 2018
…#147

This makes Draftail always preserve the full content as-is when copy-pasting between editors. See also thibaudcolas/draftjs-conductor#2.
@thibaudcolas
Copy link
Collaborator

thibaudcolas commented Jun 4, 2018

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
Copy link
Author

Pomax commented Jun 4, 2018

Fantastic! I will give it a go!

@thibaudcolas
Copy link
Collaborator

Now PR-ed into Wagtail: wagtail/wagtail#4582 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants