Enable widget clipboard manipulation #3168

jacekbogdanski · 2019-06-14T14:35:26Z

What is the purpose of this pull request?

Feature request

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

Proposed changelog entry

* [#3138](https://github.com/ckeditor/ckeditor-dev/issues/3138): Use [`widgetDefinition.getClipboardHtml()`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_plugins_widget.html#method-getClipboardHtml) method to customize [Widget](https://ckeditor.com/cke4/addon/widget) HTML during copy, cut and drag operations.

What changes did you make?

Added new widget getClipboardHtml member allowing to customize HTML copied to the clipboard during copy, cut and drop operations.

Closes #3138.

Comandeer · 2019-07-10T12:49:55Z

Rebased onto latest major.

Comandeer

I've found an edge case:

Open manual test.
Select all content inside the editor.
Cut it and paste.

Expected:

Widget was replaced by prepared content.

Unexpected:

Widget was pasted as is, without any transformation.

I think it would be very counter-intuitive for users to see that widget behaves differently when pasted with some other content than when pasted alone.

Please also update version tags.

Comandeer · 2019-08-22T16:12:22Z

I've update the implementation to use internally our toDataFormat event. It allows us to downcast widgets before copying their content.

I haven't add unit tests, because I want some early review to see if the whole approach is feasible at all.

f1ames · 2019-08-23T09:56:39Z

@jacekbogdanski I remember you talked about some selection blinking when trying to come up with the solution, can you just take a quick glimpse on @Comandeer proposal to see if the solution is similar and same issue may occur?

f1ames

The biggest change here as far as I understand correctly, is that previously when selection containing widget was copied, simply entire widget HTML (upcasted) was copied. Now, it's downcasted version is copied. When pasted back it needs to be upcasted (does it also happen when copying single widget now?). I'm not sure yet, if there are any cases which may be negatively affected by this 🤔

Found two issues after some brief testing:

After cut/paste widget it is not possible to cut again if entire content is selected:

And the blinking mentioned in #3168 (comment) (you may talk to @jacekbogdanski if he tried to solved this issue):

Also please update version tags to 4.13.0.

Overall, it looks to be a good direction 👍And the fact that it doesn't required big clipboard refactor is a plus.

plugins/widget/plugin.js

f1ames · 2019-08-23T14:48:23Z

plugins/widget/plugin.js

@@ -2494,6 +2511,7 @@
 	// * FF tends to copy all blocks up to the copybin container.
 	// * IE tends to copy only the copybin, without its container.
 	// * We use spans on IE and blockless editors, but divs in other cases.
+	// We are really sorry for summoning Zalgo…


I wonder if we really need this line 😛

H̸̡̪̯ͨ͊̽̅̾̎Ȩ̬̩̾͛ͪ̈́̀́͘ ̶̧̨̱̹̭̯ͧ̾ͬC̷̙̲̝͖ͭ̏ͥͮ͟Oͮ͏̮̪̝͍M̲̖͊̒ͪͩͬ̚̚͜Ȇ̴̟̟͙̞ͩ͌͝S̨̥̫͎̭ͯ̿̔̀ͅ

plugins/widget/plugin.js

f1ames · 2019-08-23T15:06:59Z

plugins/widget/plugin.js

+					parserFragment = CKEDITOR.htmlParser.fragment.fromHtml( widget.getClipboardHtml() );
+
+					retElement = parserFragment.children[ 0 ];
+				}


I wonder how it will work with the code below, managing editables output 🤔 Does the fact that custom widget HTML (getClipboardHtml()) is provided means any editables will be simply replaced by this custom HTML? If so there is no need to process them in any way. WDYT?

And second thing, I wonder how it will (or should) work with nested widgets? Can you check it and also add such test (we don't have many nested widget cases but still worth checking)?

Everything seems to work correctly. Editables and nested widgets are replaced by the outermost widget's getClipboardHtml result.

Shouldn't we mentioned in the getClipboardHtml() docs that for nested widget it will overwrite/replace entire widget HTML (no matter nesting/editables content)? Or is it obvious? 🤔

TBH it seems rather obvious, but there will be no harm mentioning it directly in docs.

plugins/widget/plugin.js

jacekbogdanski

And the blinking mentioned in #3168 (comment) (you may talk to @jacekbogdanski if he tried to solved this issue):

Yep, I was talking about this selection blinking, unfortunately, didn't have time to work on it. As I already have taken a look at this PR, I'm providing small feedback:

Overall it looks nice, especially that widgets are now downcasted, but as already @f1ames mentioned, I'm wondering if we can go with such big change 🤔 Users may have already some integrations relaying on upcasted widget HTML, as it has been working like that from the beginning. Maybe we could provide some configuration option as a fallback in that case? I'm really for cleaning up clipboard data, not sure if it's not "too breaking" change.

plugins/widget/plugin.js

jacekbogdanski · 2019-08-25T15:07:15Z

plugins/widget/plugin.js

@@ -2494,6 +2511,7 @@
 	// * FF tends to copy all blocks up to the copybin container.
 	// * IE tends to copy only the copybin, without its container.
 	// * We use spans on IE and blockless editors, but divs in other cases.
+	// We are really sorry for summoning Zalgo…


Comandeer · 2019-08-25T17:12:15Z

After cut/paste widget it is not possible to cut again if entire content is selected:

Actually the new logic is not even fired when the whole content is selected due to #3352. I found it during trying to write unit test for the new logic handling multiple widgets.

Comandeer · 2019-08-26T20:22:10Z

Overall it looks nice, especially that widgets are now downcasted, but as already @f1ames mentioned, I'm wondering if we can go with such big change

Actually by default the upcasted HTML is returned, via default getClipboardHtml implementation. So without implementing custom getClipboardHtml everything should work the same as previously. toDataFormat is used only as a simple way to utilize our HTML parser.

I'm thinking about making getClipboardHtml compliant with downcast, so it wouldn't operate on HTML string, but rather htmlParser.nodes. WDYT of such a change?

Comandeer · 2019-08-27T15:07:17Z

I've fixed failing tests in IE8 and Safari. Additionally I've refactored copybin to have a nicer API via CopyBin class. Enhancing it further should allow us to finally move it to clipboard (see #3358).

The only issue left is the one with selecting all content and cutting, but I'm afraid that it's connected with deeper issues with selecting widgets, as can be seen in unit tests, e.g. Safari has incorrect selection, when only two nested widgets are selected and IE8 does not select widgets if whole wrappers are not contained inside the range.

f1ames · 2019-09-04T15:26:16Z

The only issue left is the one with selecting all content and cutting, but I'm afraid that it's connected with deeper issues with selecting widgets, as can be seen in unit tests, e.g. Safari has incorrect selection, when only two nested widgets are selected and IE8 does not select widgets if whole wrappers are not contained inside the range.

Does it always happen? I recall you mentioned something about widgetselection plugin, but can't find it now 🤔 If it happens always (and not only with/without the plugin) that's pretty bad news😭 Anyway, @Comandeer would you be trying to cover this issue in this PR or prefer to extract as a follow-up (still we will need to fix it soon probably).

Comandeer · 2019-09-04T15:30:56Z

It's already extracted as #3352. Probably it would be much more difficult to fix than the proposed refactor of clipboard handling and because of that I don't see much sense in delaying this change.

Comandeer self-requested a review July 10, 2019 11:58

Comandeer self-assigned this Jul 10, 2019

Comandeer force-pushed the t/3138 branch from 87ace0c to d4d116b Compare July 10, 2019 12:49

Comandeer previously requested changes Jul 10, 2019

View reviewed changes

CKEditorBot unassigned Comandeer Jul 10, 2019

jacekbogdanski self-assigned this Jul 11, 2019

Comandeer assigned Comandeer and unassigned jacekbogdanski Aug 19, 2019

Comandeer force-pushed the t/3138 branch from d4d116b to ab7bc08 Compare August 22, 2019 16:09

Comandeer removed their assignment Aug 22, 2019

Comandeer requested a review from f1ames August 22, 2019 16:12

f1ames self-assigned this Aug 23, 2019

f1ames suggested changes Aug 23, 2019

View reviewed changes

CKEditorBot unassigned f1ames Aug 23, 2019

Comandeer self-assigned this Aug 24, 2019

jacekbogdanski commented Aug 25, 2019

View reviewed changes

Comandeer force-pushed the t/3138 branch from 7d6df1e to 5d331ac Compare August 27, 2019 15:01

Comandeer requested review from Comandeer and f1ames August 27, 2019 15:07

Comandeer removed their assignment Aug 27, 2019

Comandeer removed their request for review August 27, 2019 15:26

f1ames self-assigned this Sep 6, 2019

Comandeer and others added 26 commits September 18, 2019 09:26

Add apologies for the rest of humanity.

21ae557

Use dataProcessor to parse copied content.

696cae2

Replace element with clipboard data during downcast whe copying widget.

c7e6762

Update conditional in clipboard events listener.

4206460

Restore selection after copying.

58c551b

Update cut support.

99f26d3

Update manual test.

587694d

Restore keyboard handling.

29e8326

Add unit tests for copying and cutting multiple widgets.

e673ac2

Update manual test.

7217d9b

Add unit tests for nested widgets.

204dc21

Refactor unit tests.

7f1aafd

Add unit tests for selection and undo.

1aad69c

Remove redundant undo snapshot.

ef0cda8

Update manual test.

0898b8c

Update version tags.

223a365

Update unit tests.

65b87a9

Refactor copybin handling.

dbb4d9d

Add info about overwriting nesting widgets.

623f615

Use rAF in copybin to prevent blinking.

5cc299e

Update formatting in test.

ca7f7a6

Add workaround for #3423.

7c7bb47

Add unit test.

95ba7dd

Add manual test.

7bf7373

Fix for cutting on Chrome on macOS restored.

bc03cd7

Docs rewording. [skip ci]

3697f3d

f1ames force-pushed the t/3138 branch from 213e5d1 to 3697f3d Compare September 18, 2019 07:29

Changelog entry. [skip ci]

f6166ec

f1ames merged commit 1b67bc3 into major Sep 18, 2019

CKEditorBot deleted the t/3138 branch September 18, 2019 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable widget clipboard manipulation #3168

Enable widget clipboard manipulation #3168

jacekbogdanski commented Jun 14, 2019 •

edited by Comandeer

Loading

Comandeer commented Jul 10, 2019

Comandeer left a comment

Comandeer commented Aug 22, 2019

f1ames commented Aug 23, 2019

f1ames left a comment

f1ames Aug 23, 2019

jacekbogdanski Aug 25, 2019

f1ames Sep 9, 2019

Comandeer Sep 12, 2019

f1ames Aug 23, 2019

Comandeer Aug 26, 2019

f1ames Sep 9, 2019

Comandeer Sep 12, 2019

jacekbogdanski left a comment

jacekbogdanski Aug 25, 2019

Comandeer commented Aug 25, 2019

Comandeer commented Aug 26, 2019

Comandeer commented Aug 27, 2019

f1ames commented Sep 4, 2019

Comandeer commented Sep 4, 2019

Enable widget clipboard manipulation #3168

Enable widget clipboard manipulation #3168

Conversation

jacekbogdanski commented Jun 14, 2019 • edited by Comandeer Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

Proposed changelog entry

What changes did you make?

Comandeer commented Jul 10, 2019

Comandeer left a comment

Choose a reason for hiding this comment

Comandeer commented Aug 22, 2019

f1ames commented Aug 23, 2019

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacekbogdanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer commented Aug 25, 2019

Comandeer commented Aug 26, 2019

Comandeer commented Aug 27, 2019

f1ames commented Sep 4, 2019

Comandeer commented Sep 4, 2019

jacekbogdanski commented Jun 14, 2019 •

edited by Comandeer

Loading