Introduce Paste Tools and Paste from GDocs #3257

Comandeer · 2019-07-09T15:18:59Z

What is the purpose of this pull request?

New feature

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 is the proposed changelog entry for this pull request?

* [#835](https://github.com/ckeditor/ckeditor-dev/issues/835): Added new `pastetools` and `pastefromgdocs` plugins.

What changes did you make?

I've introduced pastetools plugin, which allows registering paste handlers:

class PasteHandler {
	// Array of external filters that have to be loaded before executing current paste handler handle method.
	filters: string[]

	// Checks if handler can handle current paste event.
	canHandle( evt ): boolean

	// Actual paste handling
	handle( evt ): void
}

I've refactored pastefromword to use the new mechanism. I've also split PfW filter into two:

pastefromword/filter/default.js – Word specific stuff,
pastetools/filter/common.js – stuff that can be useful also in other filters.

Additionally I've introduced simple API for creating filters: CKEDITOR.plugins.pastetools.createFilter. Every filter definition is divided into two parts:

rules – array of functions that will return filter rules. Every rule-creating function receives html (pasted HTML), editor and filter parameters.
additionalTransforms – function that transforms pasted HTML before passing it into filter. It receives html and editor parameters.

I've also added pastefromgdocs plugin, with its own filter and paste handler.

Currently pastefromgdocs does not have any tests, except the generic manual one.

I've also had to modify some PfW tests:

generated/functions.js – part of it moved to pastetools directory,
generated/heuristics.js,
helpers.js
image.js
parsestyles.js – moved to pastetools directory
pasteimage.js.

Some API docs are missing or not updated.

There is also small issue with config variables. Currently some parts of common filter (CKEDITOR.plugins.pastetools.filters.common.styles) use some Paste from Word config variables:

config.pasteFromWordRemoveFontStyles,
config.pasteFromWord_keepZeroMargins.

I'm thinking about introducing their pastetools counterparts:

config.pasteTools_removeFontStyles,
config.pasteTools_keepZeroMargins.

It would also mean that PfW variables become deprecated and if user set both of them, pastetools ones will take precedence.

I'm also thinking about namespaces. Currently CKEDITOR.plugins.pastetools.filters is used to store filters' config and CKEDITOR.pasteFilters is used to store filters' instances. However it introduces superlong namespaces (CKEDITOR.plugins.pastetools.filters.common.styles.normalizedStyles – ouch…). Not sure if it's fixable in some really sensible way?

I'm also not a big fan of pastetools name. Maybe something like pastehandler, specialpaste, maybe even uberpaste? pastetools isn't really about tools for pasting…

Big part of this PR is based on #2472 by @jacekbogdanski, so big kudos to him, as I could skip a huge part of initial research! 👏👏👏Too bad that simple cherry-picking wasn't possible due to the fact how much current approach is different from the one in mentioned PR.

Closes #835.

Comandeer · 2019-07-10T09:00:04Z

There is also one more important change from the previous PR: Paste from GDocs is no longer a part of Paste from Word, therefore it doesn't fire pastefromword or afterpastefromword events. Additionally it doesn't have its own paste button. Filtering GDocs content is totally transparent for the user.

plugins/pastefromgdocs/plugin.js

Comandeer · 2019-07-12T16:04:44Z

I've introduced middleware architecture described in #3258.

Comandeer · 2019-07-13T16:55:01Z

Introducing fixture-based tests revealed that filter has one visible flaw: it removes elements with [id^=docs-internal-guid-]. However in Firefox and Edge such element also contains styles and without them pasted content has changed layout. I'll provide fix in the nearest days.

Comandeer · 2019-07-14T12:46:03Z

I've updated rules for dealing with elements with GDocs specific id. Instead of removing these elements, I just remove their ids.

I've also added rule to remove all comments from the code.

f1ames

Code

Check my in-code comments.

There is also small issue with config variables. Currently some parts of common filter (CKEDITOR.plugins.pastetools.filters.common.styles) use some Paste from Word config variables:

config.pasteFromWordRemoveFontStyles,

config.pasteFromWord_keepZeroMargins.

I'm thinking about introducing their pastetools counterparts:

config.pasteTools_removeFontStyles,

config.pasteTools_keepZeroMargins.

It would also mean that PfW variables become deprecated and if user set both of them, pastetools ones will take precedence.

🤔 I think that would be a good direction. However, I'm not sure if we should deprecated the Word ones as one may still want to use them only for pasting from Word and not from other office-like applications. Do you think this might be common use case? Or we can deprecate, but mention that they still can be used if one wants to affect only Word input.

I'm also thinking about namespaces. Currently CKEDITOR.plugins.pastetools.filters is used to store filters' config and CKEDITOR.pasteFilters is used to store filters' instances. However it introduces superlong namespaces (CKEDITOR.plugins.pastetools.filters.common.styles.normalizedStyles – ouch…). Not sure if it's fixable in some really sensible way?

Yes, it's quite long TBH. We may think of introducing e.g. CKEDITOR.filters.common.styles.normalizedStyles, but then it might cause naming conflicts as different plugins might try to use the same namespace. I think we can live with long namespace like that.

I'm also not a big fan of pastetools name. Maybe something like pastehandler, specialpaste, maybe even uberpaste? pastetools isn't really about tools for pasting…

specialpaste and uberpaste has even less precise meaning that pastetools. If we thinking about changing it I would be for pastehanlder, pastecore or something similar.

Tests

Few test for build versions fails on CI (check recent Travis build) and some unit tests are failing on Edge (checked with Edge 17, but you may also check on Edge 18 if possible):

And two manual tests related to loading filters fails on IE (checked on IE11 and IE8):

When it comes to generic paste manual test I would add some nested lists to sample GDocs document.

One more thing, do we assume manual test with pasting from GDocs could be also run on browsers which doesn't support GDocs itself? If so it should be mentioned that you could open the document in different browser (however, I am not sure if specific browser would not somehow affect final result). I have tested pasting on IE8 from Chrome and it's almost perfect (👏❤️):

Docs

I have checked API docs and there are a few empty namespaces - not sure what we can do here 🤔

And also an uberpaste leftover:

And some globals:

UI

I was thinking if we should unify pasting buttons as suggested in one issue (somewhere here #3258) - there would be only Paste and Paste as plain text buttons. However, I'm not sure if this would not cause some users to thing that we dropped support for paste from Word (like after dropping paste dialog - #469) - but I think it would be good to extract to a follow-up issue about unifying paste buttons.

f1ames · 2019-07-16T10:34:55Z

plugins/pastefromgdocs/filter/default.js

+		plug = {};
+
+	/**
+	 * Set of Paste from Word plugin helpers.


Shouldn't it be "Set of Paste from Google Docs plugin helpers."? 🤔

plugins/pastefromgdocs/filter/default.js

f1ames · 2019-07-16T10:42:45Z

plugins/pastefromgdocs/filter/default.js

+					return value === 'ltr' ? false : value;
+				},
+				'style': function( styles, element ) {
+					// Returning false deletes the attribute.


This comment is probably not needed. Or it can be moved in the beginning of attributes object as it concerns all attributes filters.

f1ames · 2019-07-16T10:43:22Z

plugins/pastefromgdocs/filter/default.js

+				},
+				'style': function( styles, element ) {
+					// Returning false deletes the attribute.
+					return Style.normalizedStyles( element, editor ) || false;


You could use falseIfEmpty() function here too?

f1ames · 2019-07-16T11:03:24Z

plugins/pastefromgdocs/filter/default.js

+	 * @private
+	 * @member CKEDITOR.plugins.pastetools.filters.gdocs
+	 */
+	plug.rules = function( html, editor, filter ) {


You could also do this like:

CKEDITOR.plugins.pastetools.filters.gdocs = { rules: function... };

was there any particular reason you went with the plug object?

Such method was used in the original PR. I've used it here without much reflection TBH. I'll simplify it.

plugins/pastefromgdocs/plugin.js

plugins/pastefromword/plugin.js

f1ames · 2019-07-17T09:02:41Z

tests/plugins/pastefromword/generated/_helpers/pfwTools.js

@@ -124,6 +124,21 @@
 			}

 			return tests;
+		},
+
+		loadFilters: function loadFilters( filters, callback ) {


This function is also defined in tests/plugins/pastetools/_helpers/ptTools.js - https://github.com/ckeditor/ckeditor-dev/pull/3257/files#diff-dc1abc73b7621243c62e4e78abb3be72R19, do we need it in both places?

No, I did it this way to avoid complicated bender imports. However it can be moved to ptTools and removed from pfwTools.

tests/plugins/pastetools/manual/loadingfilters.html

f1ames · 2019-07-17T11:27:42Z

Also extracted follow-ups:

Documentation for PFGD - Paste from Google Docs - documentation and example pages #3274
Documentation for pastetools- Pastetools documentation and samples #3275
Unify pasting UI (mentioned in above review in UI section) - Unify pasting UI #3276

msamsel · 2019-08-01T14:26:35Z

During work on Past from Google Docs in CKE5, I noticed a few cases which maybe you would also like to check in CKE4.

Content in Safari like this one, after cmd + a, cmd + c and cmd + v in editor doesn't contain guid-... so might won't be recognized as content from google docs. It might be importnatn if there is no general content fixer regardless of data source.
There are cases with lists which have invalid html syntax. Things worth checking:

Provide support for pasting lists from google docs ckeditor5-paste-from-office#72 (comment)
partially selected list for example like this one:

Comandeer · 2019-08-05T09:31:21Z

I think that would be a good direction. However, I'm not sure if we should deprecated the Word ones as one may still want to use them only for pasting from Word and not from other office-like applications. Do you think this might be common use case? Or we can deprecate, but mention that they still can be used if one wants to affect only Word input.

I'm afraid that it's not possible with our current architecture, as all filters connected with fonts are moved to the shared filter. It will have to know that the content being filtered is from Word, but I don't know if there is any easy way to pass there such info 🤔

Comandeer · 2019-08-09T11:33:20Z

Content in Safari like this one, after cmd + a, cmd + c and cmd + v in editor doesn't contain guid-... so might won't be recognized as content from google docs. It might be importnatn if there is no general content fixer regardless of data source.

We have very similar issue with Excel in Safari – #2609 (comment)
I've reported bug to Safari: https://bugs.webkit.org/show_bug.cgi?id=200569

Comandeer · 2019-08-09T16:06:09Z

Few test for build versions fails on CI (check recent Travis build)

As errors were nonsensical (claiming that basic pastetools methods aren't defined), I triggered a custom build → https://travis-ci.org/ckeditor/ckeditor-dev/builds/569799069

It seems that Travis is unstable again 😞

some unit tests are failing on Edge

Can't reproduce.

Comandeer · 2019-08-11T16:18:25Z

When it comes to generic paste manual test I would add some nested lists to sample GDocs document.

I've updated tests, however I'm not able to generate sensible fixtures for Edge and IE11 due to unexpected bug in GDocs (seems to be introduced recently): content is copied incorrectly, when there's is an image at its beginning (in fact, in Edge only the image is copied).

And two manual tests related to loading filters fails on IE (checked on IE11 and IE8):

It was connected with the fact that IE does not expose proper MIME type of pasted content. I relaxed the conditional to fire paste handler.

One more thing, do we assume manual test with pasting from GDocs could be also run on browsers which doesn't support GDocs itself? If so it should be mentioned that you could open the document in different browser (however, I am not sure if specific browser would not somehow affect final result).

I've added the note. Probably more troublesome is copying in such browser, not pasting (as the pasted HTML will be the same as in modern browser).

I've also added handling legacy config names.

I haven't touch nested lists normalizations yet.

Comandeer · 2019-08-13T13:24:23Z

I've fixed issue with pasting fragments of nested lists.

f1ames

Unit tests

CI fails for build editor (one test checking handler priorities) - restarted it but the error is the same.

Also, some unit tests still fails on Edge for me:

Tested with http://localhost:1030/#tests/is:unit,path:/tests/plugins/pastefromgdocs,path:/tests/plugins/pastefromword,path:/tests/plugins/pastetools,path:/tests/plugins/pastetext

Manual tests

Copying nested lists fails on IE8:

From Chrome (other type of content is also bolded):

From Firefox:

Copying to IE8 from Google Docs (from different browser) is rather an edge case IMHO. Please take a look at the above case, but if it requires any significant amount of work we will extract it as a follow-up.

In IE11 the first image is not centered after paste:

I have also checked some cross browser pasting and it works quite well 👍

Docs

Some JSDuck errors during docs building are also visible:

plugins/pastefromgdocs/filter/default.js

f1ames · 2019-08-28T14:10:27Z

plugins/pastefromgdocs/plugin.js

+					if ( editor.config.forcePasteAsPlainText === true ) {
+						// If `config.forcePasteAsPlainText` set to true, force plain text even on Word content (#1013).
+						data.type = 'text';
+					} else if ( !CKEDITOR.plugins.clipboard.isCustomCopyCutSupported && editor.config.forcePasteAsPlainText === 'allow-word' ) {


My proposal is to leave allow-word to work as before (so only for Word) and extract new flags to a separate issue so we can handle it later if necessary, WDYT?

f1ames · 2019-08-28T15:17:17Z

plugins/pastefromgdocs/plugin.js

+						// In browsers using pastebin when pasting from Word, evt.data.type is 'auto' (not 'html') so it gets converted
+						// by 'pastetext' plugin to 'text'. We need to restore 'html' type (#1013) and (#1638).
+						data.type = 'html';
+					}


Ok 👍 Could you extract it to a separate issue then? I think we could cover also additional allow-* flags (as mentioned in https://github.com/ckeditor/ckeditor-dev/pull/3257/files#r303850250 thread) in this new issue, WDYT?

f1ames · 2019-08-28T15:27:09Z

tests/plugins/pastefromgdocs/generated/basic.js

+					'test Basic gdocs firefox': CKEDITOR.env.ie && CKEDITOR.env.version <= 11,
+					'test Basic gdocs safari': CKEDITOR.env.ie && CKEDITOR.env.version <= 11,
+					// 'test Basic gdocs edge': CKEDITOR.env.ie && CKEDITOR.env.version <= 11,
+					// 'test Basic gdocs ie11': !CKEDITOR.env.ie || CKEDITOR.env.version > 11


Why the above are commented?

#3257 (comment)

I've updated tests, however I'm not able to generate sensible fixtures for Edge and IE11 due to unexpected bug in GDocs (seems to be introduced recently): content is copied incorrectly, when there's is an image at its beginning (in fact, in Edge only the image is copied).

It seems that GDocs resolved the issue, so I updated fixtures.

Comandeer · 2019-09-13T17:09:28Z

The bug with list is not fixable on our side… GDocs just pass such data to us: last list item is outside the list. Sample data: https://gist.github.com/Comandeer/d851b67e47c81baeef1e416dc4e1521b

As you can see, the last element is after </ul>, enclosed inside span.

There is also no way to detect such situation, as there are no signs that such span is from the list. Even if we assume that all spans after lists are in fact last list elements (which is a very silly assumption), we still don't know, on which level of nesting the last item was.

I've updated docs with deprecated info for both CKEDITOR.plugins.pastefromword namespace and several PfW config variables. I've also updated @since tags for elements in CKEDITOR.plugins.pastetools as it didn't make any sense to have something added in 4.13.0 with @since 3.1.0 tag. However there is one issue with such approach:

Despite this little issue, I feel that the current shape of docs is much more readable and sensible than before.

f1ames

LGTM 👍 Good job 🎉🍰

The bug with list is not fixable on our side… GDocs just pass such data to us: last list item is outside the list. Sample data: https://gist.github.com/Comandeer/d851b67e47c81baeef1e416dc4e1521b

As you can see, the last element is after </ul>, enclosed inside span.

There is also no way to detect such situation, as there are no signs that such span is from the list. Even if we assume that all spans after lists are in fact last list elements (which is a very silly assumption), we still don't know, on which level of nesting the last item was.

👍 Let's leave it this way then.

I've updated docs with deprecated info for both CKEDITOR.plugins.pastefromword namespace and several PfW config variables. I've also updated @since tags for elements in CKEDITOR.plugins.pastetools as it didn't make any sense to have something added in 4.13.0 with @since 3.1.0 tag. However there is one issue with such approach:

Well, looks kind of strange but that's the way it is, we have introduced deprecated config option (because of duplicating already deprecated one) to have backward compatibility.

Comandeer mentioned this pull request Jul 9, 2019

Unified Paste Handling #3258

Open

jacekbogdanski mentioned this pull request Jul 10, 2019

Paste from google docs support #2472

Closed

2 tasks

msamsel reviewed Jul 10, 2019

View reviewed changes

plugins/pastefromgdocs/plugin.js Outdated Show resolved Hide resolved

Comandeer self-assigned this Jul 12, 2019

Comandeer force-pushed the t/835b branch from 3753c9a to b904e50 Compare July 12, 2019 16:04

Comandeer removed their assignment Jul 14, 2019

f1ames self-requested a review July 16, 2019 09:17

f1ames self-assigned this Jul 16, 2019

f1ames suggested changes Jul 17, 2019

View reviewed changes

CKEditorBot unassigned f1ames Jul 17, 2019

f1ames mentioned this pull request Jul 17, 2019

Paste from Google Docs - documentation and example pages #3274

Closed

Comandeer self-assigned this Aug 5, 2019

jacekbogdanski mentioned this pull request Aug 13, 2019

Paste from Google docs #835

Closed

Comandeer force-pushed the t/835b branch from b9bc5d1 to 53f8509 Compare August 13, 2019 13:22

Comandeer requested a review from f1ames August 14, 2019 11:59

CKEditorBot unassigned Comandeer Aug 14, 2019

f1ames self-assigned this Aug 28, 2019

f1ames suggested changes Aug 29, 2019

View reviewed changes

CKEditorBot unassigned f1ames Aug 29, 2019

Comandeer added 14 commits September 13, 2019 19:08

Add additional test for getClipboardData.

884ad52

Add info about supported browsers.

c99c1d8

Add handling of legacy config variables.

c45dfcc

Add liststyle plugin.

66ae1a6

Add tests for nested lists.

2193005

Update pfwTools.

03435de

Force only pastetools plugin in unit test.

42b185b

Update fixtures.

10b0e99

Make quotes optional in recognising GDocs content.

4099cb1

Add unit test for detecting GDocs content.

879137c

Fix incorrect API docs.

0fe2d7f

Remove logic connected with allow-word flag from pastefromgdocs.

c7c2d31

Remove PfW from test.

4cd305c

Update API docs with deprecation info.

7959df2

Comandeer force-pushed the t/835b branch from 032f4a4 to 2a33018 Compare September 13, 2019 17:09

Update @since in docs.

2e7cfed

Comandeer force-pushed the t/835b branch from 2a33018 to 2e7cfed Compare September 13, 2019 17:15

f1ames added 2 commits September 16, 2019 14:50

Docs adjustments.

23af9a5

Changelog entry. [skip ci]

9a0ef0d

f1ames assigned f1ames and unassigned Comandeer Sep 16, 2019

f1ames self-requested a review September 16, 2019 13:00

f1ames approved these changes Sep 16, 2019

View reviewed changes

CKEditorBot unassigned f1ames Sep 16, 2019

f1ames merged commit 6b570a6 into major Sep 16, 2019

CKEditorBot deleted the t/835b branch September 16, 2019 13:03

f1ames mentioned this pull request Sep 17, 2019

Unstable PFW test related to filters loading #3424

Open

f1ames mentioned this pull request Dec 5, 2019

Some of "plugins/forms" unit tests fails on IE8 #3700

Closed

Comandeer mentioned this pull request Jan 28, 2020

Paste from Libre Office changes the behaviour of paste filter #3809

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Paste Tools and Paste from GDocs #3257

Introduce Paste Tools and Paste from GDocs #3257

Comandeer commented Jul 9, 2019

Comandeer commented Jul 10, 2019

Comandeer commented Jul 12, 2019

Comandeer commented Jul 13, 2019

Comandeer commented Jul 14, 2019

f1ames left a comment

f1ames Jul 16, 2019

f1ames Jul 16, 2019

f1ames Jul 16, 2019

f1ames Jul 16, 2019

Comandeer Aug 5, 2019

f1ames Jul 17, 2019

Comandeer Aug 5, 2019

f1ames commented Jul 17, 2019

msamsel commented Aug 1, 2019

Comandeer commented Aug 5, 2019

Comandeer commented Aug 9, 2019

Comandeer commented Aug 9, 2019

Comandeer commented Aug 11, 2019

Comandeer commented Aug 13, 2019

f1ames left a comment

f1ames Aug 28, 2019

f1ames Aug 28, 2019

f1ames Aug 28, 2019

Comandeer Aug 30, 2019

Comandeer Sep 1, 2019

Comandeer commented Sep 13, 2019

f1ames left a comment

Introduce Paste Tools and Paste from GDocs #3257

Introduce Paste Tools and Paste from GDocs #3257

Conversation

Comandeer commented Jul 9, 2019

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What is the proposed changelog entry for this pull request?

What changes did you make?

Comandeer commented Jul 10, 2019

Comandeer commented Jul 12, 2019

Comandeer commented Jul 13, 2019

Comandeer commented Jul 14, 2019

f1ames left a comment

Choose a reason for hiding this comment

Code

Tests

Docs

UI

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

f1ames commented Jul 17, 2019

msamsel commented Aug 1, 2019

Comandeer commented Aug 5, 2019

Comandeer commented Aug 9, 2019

Comandeer commented Aug 9, 2019

Comandeer commented Aug 11, 2019

Comandeer commented Aug 13, 2019

f1ames left a comment

Choose a reason for hiding this comment

Unit tests

Manual tests

Docs

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

Comandeer commented Sep 13, 2019

f1ames left a comment

Choose a reason for hiding this comment