Opened 5 years ago

Closed 5 years ago

#12341 closed Bug (fixed)

Fixes after #12173 review.

Reported by: Piotr Jasiun Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.5.0 Beta
Component: General Version:
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

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):

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.

Change History (17)

comment:1 Changed 5 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: newassigned

comment:2 Changed 5 years ago by Piotr Jasiun

Description: modified (diff)

comment:3 Changed 5 years ago by Piotr Jasiun

Description: modified (diff)

comment:4 Changed 5 years ago by Piotrek Koszuliński

cc

comment:5 Changed 5 years ago by Piotr Jasiun

Description: modified (diff)

comment:6 Changed 5 years ago by Piotr Jasiun

Description: modified (diff)

comment:7 Changed 5 years ago by Piotr Jasiun

Description: modified (diff)

comment:8 Changed 5 years ago by 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.

comment:9 Changed 5 years ago by 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.

comment:10 Changed 5 years ago by Piotr Jasiun

Status: assignedreview

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

comment:11 in reply to:  8 Changed 5 years ago by 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.

comment:12 Changed 5 years ago by Piotrek Koszuliński

Description: modified (diff)
Status: reviewreview_failed

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.

comment:13 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

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

comment:14 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_failed

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.

Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)

comment:15 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

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

Last edited 5 years ago by Piotr Jasiun (previous) (diff)

comment:16 Changed 5 years ago by Piotr Jasiun

Rebased on the newest major.

comment:17 Changed 5 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

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

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