Fixes after #12173 review.

[Piotr Jasiun]

Part of #11437. Related to #12173.

• On editable#cut content should be deleted with lower priority (999). Currently it's hard to override cut data, because it's immediately gone.
• initPasteDataTransfer has no documentation.
• In initPasteDataTransfer and initDropDataTransfer you expect first argument to be what? Because docs says it's native event, when it's not.
• I mentioned this during one of previous reviews and I noticed this again - we can't write conditional code based on repeating CKEDITOR.env.ie usage. This makes code hard to read and hard to maintain, because it's unclear what this condition means - why IE, why not other browsers, whether IE12, 13 too or not. You need to create meaningfull variables like dataTransferSetCustomDataSupport and use them. E.g. in pasteDataFromClipboard you created htmlAlwaysInDataTransfer - it's great, just make it global for this plugin and then create more of them.
• CKEDITOR.DATA_TRANSFER_INTERNAL flags should start from 1 (1, 2, 3).

TC1 (Chrome,FF):

• open ​http://ckeditor.dev/plugins/clipboard/dev/dnd.html
• copy text from textarea
• try pasting it into first editor
• nothing happens

TC2 (IE8):

• open ​http://ckeditor.dev/plugins/clipboard/dev/dnd.html
• copy text from textarea
• paste it... it's underlined :D
• it happens only when pasting into the framed editor and it happens also when pasting using button in the toolbar (and allowing clipboard access)
• and that happens because our pastebin doesn't grab data at all, but editor#paste is fired so... we access clipboard directly :|

Because this issue is not related to changes in #12173 (the same problem occurs on master branch) I moved it to the separate ticket: #12348.

[Piotrek Koszuliński]

cc

[Piotr Jasiun]

CKEDITOR.DATA_TRANSFER_INTERNAL flags should start from 1 (1, 2, 4).

I do not get why I should give the third element value "4". It makes sense only if parameter can have multiple values at the same time and in this case transfer operation can not be both internal and external.

[Piotr Jasiun]

I mentioned this during one of previous reviews and I noticed this again - we can't write conditional code based on repeating CKEDITOR.env.ie usage. This makes code hard to read and hard to maintain, because it's unclear what this condition means - why IE, why not other browsers, whether IE12, 13 too or not. You need to create meaningfull variables like dataTransferSetCustomDataSupport and use them. E.g. in pasteDataFromClipboard you created htmlAlwaysInDataTransfer - it's great, just make it global for this plugin and then create more of them.

I agree that sometimes it is better to export condition to the variable, but...

If the set of condition is used once and have a comment explaining it then it does not make sense. I mean something like:

var needsNodesFix = CKEDITOR.env.ie && CKEDITOR.env.version < 10;
if ( needsNodesFix ) {
	clipboard.fixIESplitNodesAfterDrop( dragRange, dropRange );
}

Also when IE handle something differently it is no simplification to rename CKEDITOR.env.ie. I mean it is not dataTransferSetCustomDataSupport because the problem is not just a custom data. IE support only "Text" and "URL" type, in its own way, so the variable should be called dataTransferSupportedInTheIeWay and:

var dataTransferSupportedInTheIeWay = CKEDITOR.env.ie;

does not make anything simpler for me. I agree that maybe one day IE 15 will support dataTransfer in the W3C way, but we can introduce dataTransferSupportedInTheOldIeWay then. Now if we assign CKEDITOR.env.ie to dozens of variables we will make our code more complicated and most probably we will still need to split then when IE will support part of the features because it will support only part.

[Piotr Jasiun]

I fixed the rest of the issues and pushed to the ​t/12341

[Piotrek Koszuliński]

Replying to pjasiun:

CKEDITOR.DATA_TRANSFER_INTERNAL flags should start from 1 (1, 2, 4).

I do not get why I should give the third element value "4". It makes sense only if parameter can have multiple values at the same time and in this case transfer operation can not be both internal and external.

True. My mistake. 1,2,3 is good.

[Piotrek Koszuliński]

Replying to comment:9:

The thing about variables is that they make relations between conditions. If one variable is used in two of them that gives you some information - e.g. that you must change both occurrences. Example from the last ticket I reviewed - you changed clipboardIdDataType in one place but forgot about tests e.g, because it's not used there. There you again check CKEDITOR.env.

What I ask you to do is to look at the code and find patterns. Then create a clipboard's env object that will contain flags which will be used in dev and in tests.

If the meaning of some condition is "because IE does it in its own way" and this is the single place where it happens - leave it. But I'm 100% sure that you will find more patterns, because there are 72 changes involving CKEDITOR.env in major already. But remember to that you're not bound to your narrow field - e.g. a condition used for choosing drop target is used at least few times in many places. So far it was used in widgets only, but it's spread around the code base recently.

Another example:

 if ( CKEDITOR.env.ie ) {
	return new this.dataTransfer( null, sourceEditor );

Are you certain that this is the only place where we exclude/include browsers that don't have full support for data transfer? I can see at least one more in addPasteListenersToEditable.

Look also for occurrences of 'Text' and 'URL'. There are two places where they are used together most likely for the same reason.

You need to remember that we will be maintaining this code for many years. And things that makes some sense now will be a total WTH in couple of months for all of us, including you. And since I found them confusing now, I'm sure that there's a room for improvements.

[Piotr Jasiun]

Agreed. I've introduced isPasteEventFreelyAvailable and isCustomDataTypesSupported properties and getDropTarget method. Branch is ready to review.

[Piotrek Koszuliński]

Great, but the branch ​branch:t/12341 starts on what? It should start on ​branch:t/12172, but I can't see there my commits which I pushed to ​branch:t/12172 nor cksource/t/12172 contains commits that I can see in ​branch:t/12341 and which refer to #12172.

[Piotr Jasiun]

This branch is and was based on #12264 which is based on #12172. I rebased and pushed all 3 of them.

[Piotr Jasiun]

Rebased on the newest major.

[Piotrek Koszuliński]

Merged to major with ​git:e1379b6. I changed name of one of the flags.