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

fix: copy-paste markdown/raw text inconsistencies #5487

Merged
merged 2 commits into from
Apr 9, 2024
Merged

Conversation

elzody
Copy link
Contributor

@elzody elzody commented Mar 14, 2024

📝 Summary

Fix #5331

As described in #5331 and #5212, there are some issues with the copy-paste behavior and some inconsistencies regarding how markdown is copied vs. raw text. I think it would be helpful to define what the expected behavior would be in certain situations so that this can be refined.

This draft PR currently contains some additions I made which traverses ProseMirror Nodes upon copying text, and tries to determine if markdown should be copied or if the raw text should be copied. In my opinion, it works a bit better than before and behaves more consistently, but I did still notice some issues, such as losing block nesting sometimes (discovered when copying a code block nested in a list).

Currently defined behavior

  • Copying from a code block should just paste the code in question
  • Copying multiple list items should result in something that resembles a list
  • Copying a larger fragment of a doc should result in markdown as this can be useful for creating github issues
  • Copying an address from inside a info block should only result in the address
  • Copying version numbers should not result in an escaped character (28\.0.3 e.g.)

All testing and feedback is welcome.

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Gave this a quick test and think the behaviour is quite nice now, left a question inline

@mejo-
Copy link
Member

mejo- commented Mar 23, 2024

Nice, I just gave it a try and indeed it improves the copy and paste situation quite a lot! When adding the missing return to line 117 it is even better 😆

Still, I want to share some feedback that I got from different users after Nextcloud instances got upgraded to 28: The fact that the markdown syntax is copied into the text/plain clipboard creates a confusing user experience for them. They use Text/Collectives as a log and do a lot of copy+paste from it into different other systems. For them, copying to the Signal Desktop client and to any mail client is literally broken the way it works now. And while this PR improves the situation a bit (when the copied content stays in one paragraph or block node), the annoyance stays when copying larger parts of a document.

In essence I think we should revert the behaviour to copy markdown syntax in to the text/plain clipboard, like @susnux already commented here.

To my understanding, the only use-case where having markdown content in the text/plain clipboard is helpful is when you copy+paste to Github. In most cases where you copy into a rich text editor, the text/html clipboard is used anyway. And in the majority of cases where you copy into a plaintext only editor, having markdown pasted is not what you expect from a user perspective.

Some concrete examples with a broken user experience:

  • Copy a list of email address links that spans over several blocks (e.g. [mail@example.org](mailto:mail@example.org)\n\n[mail2@example.org](mailto:mail2@example.org) into the recipient field of any mail client (Thunderbird, Roundcube)
  • Copy two paragraphs of text with bold, italic, etc formatting into the Signal Desktop Client or the text-only body field of your mail client

@juliusknorr
Copy link
Member

Results from the call discussing this:

  • Bring back the old Ctrl-Shift-C shortcut to copy the markdown with the new traverseNodes logic
  • Change back default to copy text
  • For later (file as separate issue)
    • Traverse nodes when copying plain text, Lists/Blockquotes could still be preserved for example
    • There may be further cases that need some thinking on how they should be mapped to plaintext (e.g. links with label, mailto links)

@max-nextcloud
Copy link
Collaborator

Results from the call discussing this:

* Bring back the old Ctrl-Shift-C shortcut to copy the markdown with the new traverseNodes logic

* Change back default to copy text

My impression is that we are going back and forth between two coarse grained approaches. Once we run into the downsides of one, we switch back to the other... until we run into the downsides of the previous.

Let me just illustrate what the change to markdown improved:

source before after
grafik grafik grafik

I'd clearly prefer the latter even when pasting into a mail or signal.

I'm not convinced we need two different copy and paste modes. After all the actual markdown source can be downloaded and I think there is limited use in the pure text content of the cells. Rather than offering two bad options I'd propose to iterate on a solution that produces a good enough output in most cases.

@susnux
Copy link
Contributor

susnux commented Apr 9, 2024

I think for most users having "random" characters inside the copy result is unexpected behavior. I really have a lot complains from my users that can not copy anything from code blocks. Especially as codeblocks are used to preserve the formatting.

When copying multiple elements the behavior is often not defined, so it would be fine to have the markdown e.g. for a table.
But for e.g. code blocks - especially if only selected parts from it - then you expect to also only copy that part.

Another common solution is to populate the clipboard with multiple mimetypes, e.g. office programs often provide HTML besides plain text.


Thats why I suggested to either provide multiple mime types so the application pasted to can select the content it supports. Or have a special command for copying markdown only for experienced users, but most normal users expect simply text no "weird" characters.

@max-nextcloud
Copy link
Collaborator

I think for most users having "random" characters inside the copy result is unexpected behavior. I really have a lot complains from my users that can not copy anything from code blocks. Especially as codeblocks are used to preserve the formatting.

I totally agree. This PR fixes these issues as far as i can tell.

Thats why I suggested to either provide multiple mime types so the application pasted to can select the content it supports.

We do provide html and plain mime types. I'm not aware of any app that uses the plain/markdown mime type when pasting. So while this would be a good approach in general it would not solve the problems in the before column when pasting into plain text fields.

@susnux
Copy link
Contributor

susnux commented Apr 9, 2024

We do provide html and plain mime types. I'm not aware of any app that uses the plain/markdown mime type when pasting. So while this would be a good approach in general it would not solve the problems in the before column when pasting into plain text fields.

Yes makes sense

@elzody elzody marked this pull request as ready for review April 9, 2024 15:39
elzody and others added 2 commits April 9, 2024 11:42
…ste behavior

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Max <max@nextcloud.com>
@mejo-
Copy link
Member

mejo- commented Apr 9, 2024

My impression is that we are going back and forth between two coarse grained approaches. Once we run into the downsides of one, we switch back to the other... until we run into the downsides of the previous.

Let me just illustrate what the change to markdown improved:

Thanks for looking into this @max-nextcloud. While I agree with most of your analysis, the concrete pain points that I lined out that users (regularly) report to me with the current implementation remain with this pull requests. I think two things need to change at least to satisfy them:

  • links/URLs should be plain text, not in markdown syntax
  • marks like bold, italic, underline have to be stripped off

Without these changes, copying from Text documents to any plain text processors will confuse people, and - most importantly - breaks their current workflows.

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

After discussing this with @max-nextcloud I'm confident that you have a good path forward in mind. Makes sense to get this PR merged first and improve the situation in a folllow-up afterwards 😊

@elzody elzody merged commit f10d3dc into main Apr 9, 2024
58 checks passed
@elzody elzody deleted the fix/copy-code-block branch April 9, 2024 19:37
@max-nextcloud

This comment was marked as outdated.

@max-nextcloud

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

@max-nextcloud
Copy link
Collaborator

/backport f10d3dc to stable29

backportbot bot pushed a commit that referenced this pull request Apr 10, 2024
* fix: traverse through nodes in order to determine the correct copy-paste behavior

---------

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Max <max@nextcloud.com>
Co-authored-by: Max <max@nextcloud.com>
@max-nextcloud
Copy link
Collaborator

/backport f10d3dc to stable28

backportbot bot pushed a commit that referenced this pull request Apr 10, 2024
* fix: traverse through nodes in order to determine the correct copy-paste behavior

---------

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Max <max@nextcloud.com>
Co-authored-by: Max <max@nextcloud.com>
max-nextcloud added a commit that referenced this pull request Apr 11, 2024
* fix: traverse through nodes in order to determine the correct copy-paste behavior

---------

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Max <max@nextcloud.com>
Co-authored-by: Max <max@nextcloud.com>
max-nextcloud added a commit that referenced this pull request Apr 11, 2024
* fix: traverse through nodes in order to determine the correct copy-paste behavior

---------

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Max <max@nextcloud.com>
Co-authored-by: Max <max@nextcloud.com>
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can no longer copy content from code blocks
5 participants