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 for: Editor with enterMode set to ENTER_DIV alters content on paste #2776

Merged
merged 19 commits into from
Jun 18, 2019

Conversation

engineering-this
Copy link
Contributor

@engineering-this engineering-this commented Jan 23, 2019

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

What changes did you make?

When a div is to be inserted in another empty div I've forced triggering logic which is normally trigerred when element is not allowed in selection.

Closes #2751

@Comandeer Comandeer self-requested a review January 29, 2019 13:20
@Comandeer Comandeer self-assigned this Jan 29, 2019
Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

The fix doesn't work correctly with div plugin:

  1. Cut content from the editor.
  2. Insert div using div button in the toolbar.
  3. Select the newly added div by pressing it in elements path.
  4. Paste content.

Expected:

The pasted content is untouched.

Actual:

The first div is replaced by parent div:

<div>This is some sample text.<div>New line.</div></div>

I'm wondering if just removing body > div tag when it's the only element and it's empty, wouldn't be a better solution.

core/editable.js Outdated
@@ -2081,6 +2082,17 @@
return nodesData;
}

// Prevent inserting div into div when Editor is in ENTER_DIV mode, and target element is an empty div.
function preventInsertingDiv( nodeName, element, editor ) {
Copy link
Member

Choose a reason for hiding this comment

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

The second parameter can use some better name, e.g. container. Now nodeName and element seem to be about the same element.

};

var tests = {
// 2751
Copy link
Member

Choose a reason for hiding this comment

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

Our convention for tests is // (#<issue number>).

@engineering-this
Copy link
Contributor Author

I'm wondering if just removing body > div tag when it's the only element and it's empty, wouldn't be a better solution.

Simplified solution by doing as above.

Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

I was able to produce such output:

<div><div><div><div><div><div><div><div><div><div><div><div><div><div><div>This is some sample text.</div><div>New line.</div></div></div></div></div></div></div></div></div></div></div></div></div></div></div>
  1. Cut editor's content.
  2. Insert div using div button from the toolbar.
  3. Select the newly created div element via element's path.
  4. Paste.
  5. Select all.
  6. Paste.
  7. Repeat steps 5-6 several times.

@engineering-this
Copy link
Contributor Author

That's an issue with selection. In Chrome and Safari using native SelectAll produces selection as following:
<div><div>{This is some sample text.</div><div>New line.}</div></div>
While it should be:
[<div><div>This is some sample text.</div><div>New line.</div></div>]

Extracted to #2801

@mlewand
Copy link
Contributor

mlewand commented Feb 12, 2019

Guys, just FYI:

<div><div>{This is some sample text.</div><div>New line.}</div></div>

Is a perfectly valid selection, and should not be a condition to any bug.

@engineering-this engineering-this requested review from Comandeer and removed request for Comandeer February 12, 2019 13:08
@engineering-this engineering-this self-assigned this Feb 12, 2019
@engineering-this
Copy link
Contributor Author

engineering-this commented Feb 12, 2019

@mlewand I've thought this through. Merging divs doesn't sound like good option to me. User can have div as a style container where he wants to paste content. If we limit merging divs only when they doesn't have any extra attributes, then there will be following case:

  1. User has content:
    <div class="red-border"><div class="hightlight">Foo</div></div>
  2. SelectAll.
  3. Copy.
  4. Paste.

Content will be wrapped by duplicated divs.

I don't think we can safely cover every possible case by merging divs.

However I have another idea for this case. I'll skip details for now, if it works I will push updates.

@engineering-this
Copy link
Contributor Author

engineering-this commented Feb 12, 2019

The more I dig into it the more I think we should close original issue as an upstream and report to Chromium.

There are a lot of issues because of Chrome/Safari selection.

First case:

  1. Open editor sample, clear it.
  2. Apply special container style from styles combo.
  3. Type 'foo'.
  4. Select all.
  5. Copy.
  6. Press 'New Page' button.
  7. Paste.

Expected

Pasted content is the same.
screenshot 2019-02-12 at 17 40 12

Actual

Special container style is missing.
screenshot 2019-02-12 at 17 40 23
Because of such selection on paste dataTransfer text/html data is foo.

Second case:

Repeat each step, but after step 3. press Enter.

Expected

screenshot 2019-02-12 at 17 40 41

Actual

screenshot 2019-02-12 at 17 41 05
The first line is preserved with styling, second is missing.

Additional case

When using editor with divarea plugin, and selectAll button from toolbar pasted content is always the same as before paste, because created selection starts and ends at editable instead of text. However Ctrl+A will reproduce bug.

Every above case results in expected behaviour on Firefox and Edge.

Edit: The cut

There is one case that needs to be fixed by our side and it is reproduced in all browsers:

  1. Editor has ENTER_DIV mode.
  2. Content with selection: [<div>Foo</div>].
  3. When content is cut, editor will still place an empty div as it is required to wrap content.
  4. On paste content goes into that div, so we have <div><div>Foo</div></div>.

This case is fixed by current proposed solution in this PR. However if nested divs are cut/copied - it won't work on Chrome/Safari, because of selection issue. So we can keep this PR to close part of original issue which is our bug.

@engineering-this
Copy link
Contributor Author

Reported it on chromium and team confirmed that they can reproduce it:
https://bugs.chromium.org/p/chromium/issues/detail?id=931285

@engineering-this
Copy link
Contributor Author

The original case which is reproduced also on Firefox is covered by this PR. Other cases which are present only on Chrome/Safari are upstream issues.

I've updated manual test, to reflect that.

@Comandeer Comandeer self-assigned this Jun 3, 2019
@Comandeer Comandeer changed the base branch from master to major June 3, 2019 11:02
Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

LGTM!

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

Successfully merging this pull request may close these issues.

Editor with enterMode set to ENTER_DIV alters content on paste
3 participants