Opened 4 years ago

Last modified 3 years ago

#14358 review Bug

[Blink, FF] Block Elements removed when we copy & paste

Reported by: Satya Minnekanti Owned by: kkrzton
Priority: Nice to have (we want to work on it) Milestone:
Component: General Version: 4.5.0
Keywords: ibm Cc: chrisgui, Irina

Description

Steps to reproduce

  1. Open a sample with ACF disabled.
  2. Create a Block level element ( ex: H1)
  3. select the Block lelvel elemnt( ex: H1) with mouse or use Shift + Home
  4. Ctrl + C to copy
  5. Press Enter to go to a new paragraph
  6. Ctrl + V to paste

Expected result

Block Level element(ex: H1) pasted properly with all attributes(if any applied)

Actual result

Pasted as plain text with out Block level element and it's attributes

This is a regression from 4.5.x & it's HIGH priority defect for us & we have customer PMR

Change History (26)

comment:1 Changed 4 years ago by Marek Lewandowski

cc

comment:2 Changed 4 years ago by Jakub Ś

Status: newconfirmed
Version: 4.5.64.5.0

I can't confirm that this is ACF dependent (got same results with ACF on/off) but I can definitely confirm this issue happening in all browsers.

Last edited 4 years ago by Jakub Ś (previous) (diff)

comment:3 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8

comment:4 in reply to:  2 Changed 4 years ago by Satya Minnekanti

Replying to j.swiderski:

I can't confirm that this is ACF dependent (got same results with ACF on/off) but I can definitely confirm this issue in Firefox, Blink and Webkit (IE is free of this bug - didn't check EDGE).

@j.swiderski Thanks for reply. I could reproduce the issue in IE11 & EDGE as well on nightly build. http://nightly.ckeditor.com/16-01-29-07-07/full/samples/

comment:5 Changed 4 years ago by Jakub Ś

@satya you are right. There is one case which works and it happens I have tested it - keyboard only.

So set H1 in format dropdown, type, press Home, press Shift+End, Ctrl+C, End, Enter, Ctrl+V - that way and actually only that way it works. I have modified my description.

comment:6 Changed 4 years ago by kkrzton

Owner: set to kkrzton
Status: confirmedassigned

comment:7 Changed 4 years ago by Jakub Ś

#14391 was marked as duplicate.

Please check TC mentioned in that ticket. If you have any doubts if this behaviour should also be applied to paragraphs we can discuss it with m.lewandowski and IBM

comment:8 Changed 4 years ago by kkrzton

Summary: Block Elements removed when we copy & paste[Blink, FF] Block Elements removed when we copy & paste

It is a regression from 4.5.x, it worked fine in 4.4.8 (in all browsers but IE). Since 4.5.x custom copy handling was introduced (for all browsers but IE). In earlier versions copying (copy/cut) was handling natively.

Custom copy handling introduced in 4.5.0 does not support IE because of some limitations in ClipboardAPI in IE implementation. It means copy handling in IE hasn't change across previously mentioned versions, copying headers also doesn't work in versions below 4.5.0 in IE. Due to this differences in copy handling, we decided to split this ticket into two, one is for fixing this issue in other browsers than IE and the other one is to support this in IE. It will help us to work on this issues separately and provide solution faster.

IE issue #14397.

Last edited 4 years ago by kkrzton (previous) (diff)

comment:9 in reply to:  7 Changed 4 years ago by kkrzton

Replying to j.swiderski:

#14391 was marked as duplicate.

Please check TC mentioned in that ticket. If you have any doubts if this behaviour should also be applied to paragraphs we can discuss it with m.lewandowski and IBM

We also reopened #14391 due to differences in handling paragraphs versus other block elements #14391#comment:4.

comment:10 Changed 4 years ago by kkrzton

Status: assignedreview

Changes in t/14358.

comment:11 Changed 4 years ago by Marek Lewandowski

Status: reviewreview_failed

editor.getSelectedHtml methods seems to be the right place to implement it, however current solution has couple of flaws:

  • When one copies only part of header text, header markup is applied. Header (block styling) should be copied only when whole block's content is selected.

Consider following example:

<h3>foo [bar baz] bom</h3>

Editor should copy only bar baz text. Ofc it might contain inline styles, that's fine.

Block style should only be copied when the element is selected entirely, like so:

<h3>[foo bar baz bom]</h3>
  • It only covers headers.

In fact we should care about any block styles.

List handling will be problematic, so we can extract it to a separate ticket.

Having this done it will fix #14391.

  • It should apply to as many blocks in path as possible.

Actually it should also be possible to include multi level blocks.

Consider blockquote p or ul li p.

Here is a TC:

<blockquote>
	<p>[foo bar baz]</p>
</blockquote>

should be expanded to contain the block:

[<blockquote>
	<p>foo bar baz</p>
</blockquote>]

Note "Expansion" of copied path should also be stopped by [contenteditable=false] elements (e.g. widget wrappers).

comment:12 in reply to:  11 Changed 4 years ago by kkrzton

When one copies only part of header text, header markup is applied. Header (block styling) should be copied only when whole block's content is selected.

Yes, it works this way and it was made intentionally. This bug was introduced when the custom copy/paste was introduced (4.5.0). Before, it was handled by native copy/paste browser implementation. In Chrome and FF native copy/paste works this way, so copying part of header also creates header element. I'm not saying that this is a behaviour we would like to implement only that this solution was based on how it worked before.

In fact we should care about any block styles.

Yes, I agree we should introduce more general solution to handle as many block elements as possible which should also fix #14391 (or any related future issues). Partial coping/pasting as mentioned above may not make sense for headers but considering #14391 I think it make sense and is rather intuitive.

One more thing worth mentioning is that while native copy/paste works for headers as described above it does not work for e.g. paragraphs (so #14391 is more like a new feature than bug - it is about copying element attributes not only element). It shouldn't matter much for general solution but it should be tested solidly.

comment:13 in reply to:  11 Changed 4 years ago by kkrzton

Status: review_failedassigned

We agreed with @m.lewandowski that:

  • Header (block styling) should be copied only when whole block's content is selected. This applies to all block elements. All block elements attributes except id also should be copied.
  • We should care about any block styles/elements so we will introduce more general solution.
  • It should apply to as many blocks in path as possible. "Expansion" of copied path should also be stopped by [contenteditable=false] elements (e.g. widget wrappers) and table elements (like td, th).

comment:14 Changed 4 years ago by kkrzton

I have some doubts about removing id while copying whole block. Copying is not an issue and we don't know what user is going to do with copied content. The solution could be to remove id while pasting but then we should only remove it if the element with given id already exists in editor.

I think it is broader issue, not only connected to this ticket but to the whole pasting logic. Probably it should be handled in separate ticket as bug / feature request.

@m.lewandowski what do you think?

comment:15 Changed 4 years ago by Marek Lewandowski

@k.krzton Completely agreed, it's logical. Now I wonder why we didn't think about it that way from the very beginning. :)

comment:16 Changed 4 years ago by kkrzton

@m.lewandowski Created new feature request #14455 for issue mentioned in #comment:14.

comment:17 Changed 4 years ago by kkrzton

Status: assignedreview

Changes in t/14358.

Introduced mechanism for recognizing if entire block element is selected. If there is, the outermost entirely selected block will be copied which results in preserving styling while pasting such elements. As mentioned in above comment

"Expansion" of copied path should also be stopped by [contenteditable=false] elements (e.g. widget wrappers) and table elements (like td, th).

non-editable elements, table elements (td, th and also caption) and lists elements stops path expansion. Copying whole list while one item is entirely selected is under consideration and will be handled separately, but still all formatting inside lists will be copied correctly.

This solution also fixes #14391.

There is one difference in Blink vs. other browsers in handling empty blocks (reported here #14501) so for case mentioned in this ticket, it will work differently. While CKEditor keeps clean html and such empty elements are not a common case it may be handled in future (basically by fixing this issue #14501).

comment:18 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:19 Changed 4 years ago by Marek Lewandowski

Rebased the branch to the latest master with a minor adjustment. Still under the review though.

comment:20 Changed 4 years ago by Jakub Ś

It turns out we have quite a few issues with pasting and they all may be related as they all can be reproduced from 4.5.0:
Styles Stripping: #13860, #13926, #14250, #14358
Styles stripping on Chrome in various test cases: #14921, #14593, #13753, #13751, #16454
ForcePasteAsPlainText pastes formatted HTML with Ctrl+V: #13969
Weird event pasting relation (not sure): #13763
White spaces not preserved: #14614
Problem with pasting in LibreOffice: #14622 (we do not support it but I have added this issue because general solution might also bring back old behaviour).

Last edited 3 years ago by Jakub Ś (previous) (diff)

comment:21 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:22 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.10CKEditor 4.5.11

Moving tickets to the next milestone.

comment:23 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.11CKEditor 4.6.1

comment:24 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2

Moving to 4.6.2 minor release, as 4.6.1 is mostly about polishing 4.6.0.

comment:25 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.2CKEditor 4.7.0

comment:26 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.7.0
Priority: NormalNice to have (we want to work on it)

We won't be able to deliver it safely in 4.7.0. Moving to backlog.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy