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

Pasted plain text should inherit formatting of the surrounding text #1006

Closed
gauravjain028 opened this issue Apr 26, 2018 · 12 comments · Fixed by #7778
Closed

Pasted plain text should inherit formatting of the surrounding text #1006

gauravjain028 opened this issue Apr 26, 2018 · 12 comments · Fixed by #7778
Assignees
Labels
squad:core Issue to be handled by the Core team. status:discussion support:2 An issue reported by a commercially licensed client. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@gauravjain028
Copy link

Is this a bug report or feature request? (choose one)

🐞 Bug report

💻 Version of CKEditor

1.0.0.beta.2

📋 Steps to reproduce

  1. Add some content in any H tag and make it bold.
  2. Copy some plain text from somewhere else like from address bar of Browser.
  3. Paste the content in between the H tag added in step 1

✅ Expected result

The new text should be Bold as well.

❎ Actual result

The new text is not Bold.

📃 Other details that might be useful

copy-paste-issue

@scofalik
Copy link
Contributor

We could check if the pasted content is only text. If so, we should insert it using writer.insertText() which would take care of using selection's attributes.

@gauravjain028
Copy link
Author

gauravjain028 commented Apr 27, 2018

@scofalik Is it any workaround for it until you fix this issue or you are fixing it soon ?

@Reinmar Reinmar added type:improvement This issue reports a possible enhancement of an existing feature. status:confirmed type:bug This issue reports a buggy (incorrect) behavior. and removed type:improvement This issue reports a possible enhancement of an existing feature. labels Apr 27, 2018
@Reinmar Reinmar added this to the backlog milestone Apr 27, 2018
@Reinmar
Copy link
Member

Reinmar commented Apr 27, 2018

CKEditor 5 behaves in this case like CKEditor 4. However, what historically was a must-work-this-way solution in CKEditor 4, might be changed now in CKEditor 5.

The thing is that this needs to work differently when pasting different type of content:

  • When pasting HTML, the inserted content should not inherit attributes of the surrounding text. That's because the lack of <strong> or other styles in the clipboard indicates that an unstyled text was copied so we should maintain this. That's how CKEditor 4 and 5 work.
  • When pasting plain text, the inserted content can inherit attributes of the surrounding text. Since it was copied without any styles we can say that the styles should be inherited. However, this is a tough call, because we don't know the intention of the user.

In CKEditor 4 we didn't have much choice with the plain text because we didn't know whether plain text was pasted or HTML which looked like plain text (had no styles). In CKEditor 5 we know this because we read the content from the clipboard. So we could, potentially, treat HTML and plain text differently.

However, as I said, this is a tough call. If I copied 30 lines of plain text and pasted that somewhere where I had bold or any other style enabled... should all that be bolded? OTOH, when I copied a couple of words, applied the "code" style, pasted it I think it'd expect the pasted content to be preformatted. I guess the latter is a more common scenario, but I don't want to make a hasty decision here.

Let's gather feedback, then!

@Reinmar Reinmar changed the title Missing formatting while pasting plain text inside H+Bold tag Pasted plain text should inherit formatting of the surrounding text Apr 27, 2018
@Reinmar Reinmar added type:improvement This issue reports a possible enhancement of an existing feature. status:discussion and removed status:confirmed type:bug This issue reports a buggy (incorrect) behavior. labels Apr 27, 2018
@scofalik
Copy link
Contributor

There are two workarounds I can see:

  1. In Clipboard#event:inputTransformation.
  2. In Model#event:insertContent.

In the first one, you need to overwrite the default Clipboards event inputTransformation callback:

https://github.com/ckeditor/ckeditor5-clipboard/blob/master/src/clipboard.js#L160-L176

You basically need to copy it and modify a little.

After data.content is converted to modelFragment, you'd need to check if the model fragment has exactly one model.Text node with no attributes. If so, you'd have to re-write editor.model.document.selection attributes on that text node (using model.change() and the model.Writer). Refer to API docs. The last one thing is to call evt.stop(). AFAICS, at this moment, there are no callbacks added with lower priority to inputTransformation event so you should be fine.

Second solution is similar in a way that you will also add a callback to an event. This time it is model.Model#event:insertContent event. insertContent is a decorated method, which means that calling it fires an event. In the event you have a chance to modify the parameters.

So, basically, you add a callback to that event. As above, you need to check if inserted model is just a text node. If so, re-write attributes. Keep in mind that selection is given as one of arguments (the second one) so you should not get it from the Editor instance.

Note, that the second solution may affect other features. Every feature that use model.Model#insertContent will be affected. This may not be a desired effect, although insertContent is not used that heavily.

@scofalik
Copy link
Contributor

BTW. there's one more approach -- similar to the first one I described. The difference is that you'd also need to add a callback to view.Document#event:clipboardInput. In the callback, you'd have to check if plain text was copied to the clipboard. If so, in Clipboard#event:inputTransformation you would not have to check what is in modelFragment. Instead, you'd use the information obtained in clipboardInput callback.

Refer here: https://github.com/ckeditor/ckeditor5-clipboard/blob/master/src/clipboard.js#L137-L158

In your callback, you'd have to set a flag in this fashion:

if ( !dataTransfer.getData( 'text/html' ) && dataTransfer.getData( 'text/plain' ) ) {
    wasPlainTextPasted = true;
}

Then, use the flag to decide if Selection attributes should be re-written to the modelFragment contents.

@scofalik scofalik reopened this Apr 27, 2018
@dtwist
Copy link

dtwist commented May 4, 2018

2¢: This is a trade-off either way you slice it. Which use-case for copy/paste is more prevalent?:

  • copy a small bit of text and insert into content you're working on (such as in the screencast above)
  • copy and paste a large chunk of (unformatted) text

Perhaps the second case is less likely, because a person copying whole paragraphs, etc will have already formatted it in another editor? Ultimately though, I think the answer will depend on the project using CKE. Maybe pick a default and allow for a project level config option?

@scofalik
Copy link
Contributor

scofalik commented May 7, 2018

Quick note: Some desktop editors have multiple options of pasting -- paste as copied, paste unformatted, etc.

@Reinmar
Copy link
Member

Reinmar commented May 14, 2018

Quick note: Some desktop editors have multiple options of pasting -- paste as copied, paste unformatted, etc.

We hoped to implement such a feature in the past, but it's not an easy feature due to its UX.

@lslowikowska lslowikowska added the support:2 An issue reported by a commercially licensed client. label Jul 15, 2020
@wwalc
Copy link
Member

wwalc commented Jul 16, 2020

$0.02 I juts looked at how pasting works and confirm that using Ctrl+Shift+V to paste plain text into formatted text results in a wrong behavior - the formatting of the text is not as expected. In CKEditor 4 it looks fine:

CKEditor 4

Screen Shot 2020-07-16 at 3 46 53 PM

CKEditor 5

Screen Shot 2020-07-16 at 3 46 46 PM

My intuition as an end user is that whenever I paste any portion of a content as plain text I expect it to be formatted exactly in the same way as the place where I'm pasting it.

@Reinmar Reinmar modified the milestones: backlog, nice-to-have Jul 21, 2020
@Reinmar
Copy link
Member

Reinmar commented Jul 21, 2020

I think it's ok if we change the current behavior. That's even a bit of logic that could be configurable (config.clipboard.shouldPlainTextInheritStyles).

Ideally, the user should be able to decide how the content should be pasted. The dropdown (displayed after a past happened) with multiple options is something that we should consider long-term. Short-term, we need to tune the automatic behavior.

@Reinmar
Copy link
Member

Reinmar commented Jul 21, 2020

I wonder if we should differentiate between:

  • Shift+Ctrl+V
  • pasting something that was originally a plain text (from notepad, VSCode, etc)

The former means that the user intentionally chose plain-text. The latter is more accidental. 

WDYT?

@Reinmar Reinmar added squad:core Issue to be handled by the Core team. and removed squad:red labels Jul 28, 2020
@Reinmar Reinmar removed this from the nice-to-have milestone Jul 28, 2020
@jodator
Copy link
Contributor

jodator commented Aug 6, 2020

We could detect shift+ctrl+v either by listening or just by checking for sole text/plain in the pasted data.

Because of the editor configuration there is one case that is tricky: pasting unsupported format. It would end up as plain text in the model. I'd go with PR's implementation to unify behaviour for all of this and remove this one use-case.

More on this: #7778 (comment).

jodator added a commit that referenced this issue Aug 6, 2020
Feature (clipboard): Pasting a plain text will inherit selection attributes. Closes #1006.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:core Issue to be handled by the Core team. status:discussion support:2 An issue reported by a commercially licensed client. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants