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

Pasting plain text on link breaks link #8158

Closed
jodator opened this issue Sep 25, 2020 · 12 comments · Fixed by #8272
Closed

Pasting plain text on link breaks link #8158

jodator opened this issue Sep 25, 2020 · 12 comments · Fixed by #8272
Assignees
Labels
package:link squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.

Comments

@jodator
Copy link
Contributor

jodator commented Sep 25, 2020

📝 Provide detailed reproduction steps (if any)

  1. Use an example in docs (ie Classic editor).
  2. Create a link (ie on "example")
  3. Copy text without formatting (ie "foo").
  4. Put the cursor inside the link (ie "exam|ple").
  5. Use paste or plain-text paste.

✔️ Expected result

The link is set on whole text (ie on "examfoople")

❌ Actual result

The Pasted text breaks the link apart:

📃 Other details

cc @cksource/ckeditor-5-triage

  • Browser: any (Firefox/Chrome tested)
  • CKEditor version: master @ iteration36

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@jodator jodator added type:bug This issue reports a buggy (incorrect) behavior. package:link type:regression This issue reports a bug that was not present in the previous releases. squad:core Issue to be handled by the Core team. release:potential-blocker This issue potentially blocks the upcoming release (should be checked). labels Sep 25, 2020
@jodator
Copy link
Contributor Author

jodator commented Sep 25, 2020

@Reinmar it works on the current docs:

@Reinmar
Copy link
Member

Reinmar commented Sep 25, 2020

Let's have this bisected. It's semi-serious. It's not a big UX issue, as it worked this way for the last 4 years. However, it's a pity that we broke something that we have just fixed in the last iteration.

@jodator, could you bisect it?

@mlewand
Copy link
Contributor

mlewand commented Sep 25, 2020

Reason for this is that pasting as plain text clears anything except formatting attributes:

keyValuePair => editor.model.schema.getAttributeProperties( keyValuePair[ 0 ] ).isFormatting

It was introduced as a part of recent paste as plain text improvements.

@Reinmar
Copy link
Member

Reinmar commented Sep 25, 2020

I don't understand. We were going in the direction to keep all the formatting on plain text paste. Not lose it. So, if we changed anything it should change it so we keep the formatting.

@Reinmar
Copy link
Member

Reinmar commented Sep 25, 2020

The good thing is that this is broken only for links. So, I wouldn't consider this a release blocker and I can also understand how we missed that (since this works for other attributes). So, from my side, no alarm :D

@Reinmar
Copy link
Member

Reinmar commented Sep 25, 2020

cc @cksource/ckeditor-5-triage (testing, as above it did not work)

@jodator
Copy link
Contributor Author

jodator commented Sep 25, 2020

cc @cksource/ckeditor-5-triage (testing, as above it did not work)

Looks like there's no such team (not highlighted).

The first bad commit is: cb08c3e.

@mlewand
Copy link
Contributor

mlewand commented Sep 25, 2020

Yea, I guess that all attributes should be preserved - it's enough to remove the filter function.

It will change

it( 'should not preserve selection attributes when the changes are not caused by typing (pasting check)', () => {
though, so we'll need to decide whether we're ok with this or not (edit: that's the same thing I referenced in the commit message 😂).

I think we should revisit it at the start of next iteration.

@Reinmar
Copy link
Member

Reinmar commented Sep 25, 2020

Looks like there's no such team (not highlighted).

We need to cc teams from the respective orgs (@ckeditor or @cksource). I fixed that in our notes.

@Reinmar Reinmar removed the release:potential-blocker This issue potentially blocks the upcoming release (should be checked). label Sep 25, 2020
@jodator
Copy link
Contributor Author

jodator commented Sep 25, 2020

It will change

At first glance, it looks like different cases:

  1. First case: a collapsed selection.
  2. The second case: would be plain-text pasting inside the selection (inside, touching start, touching end).
  3. The third (the linked one): a selection that fully covers the attribute range.

ps.: I didn't check the rest of the tests if the above cases are covered.

@Reinmar
Copy link
Member

Reinmar commented Sep 29, 2020

To be checked:

  • Formatting vs other attributes (link, mention).
  • Pasting in the middle of that content vs at the end/start of it (interesting scenario in case of mentions: what happens when you paste at the end of an existing mention).

@mlewand
Copy link
Contributor

mlewand commented Oct 19, 2020

Another feature that was not mentioned and we should care about is restricted editing.

I checked how other text editors handle pasting plain text in relation to links and both Word and GDocs are consistent: link is removed only when pasting with link entirely selected. Otherwise the link is preserved.

In fact leaving non-formatting attributes when pasting plain text has some sense:

  • Restricted editing is based on text attributes and you definitely want to keep restricted editing exception marking then pasting a plain text in an existing exception field.
  • Link inheriting also is the expected behavior.

As for mentions, their attribute is theoretically inherited but is then filter out by the mentions plugin (I'm assuming this is where its post-fixer kicks in).

PR with fix in #8272.

@Reinmar Reinmar modified the milestones: iteration 37, iteration 38 Oct 26, 2020
@Reinmar Reinmar assigned pomek and unassigned mlewand Oct 26, 2020
@Reinmar Reinmar modified the milestones: iteration 38, nice-to-have Nov 9, 2020
@niegowski niegowski self-assigned this Dec 1, 2020
@niegowski niegowski modified the milestones: nice-to-have, iteration 38 Dec 1, 2020
Reinmar added a commit that referenced this issue Dec 2, 2020
Fix (clipboard): Pasting plain text inside a link or restricted editing editable region will no longer break them. Closes #8158.

Tests (link): Added tests for pasting plain text over the link.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:link squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants