Opened 10 years ago
Closed 10 years ago
#12168 closed New Feature (fixed)
Improve dataTransfer object
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 )
Part of #11437.
setData and getData for custom data types should work on every browser. dataTransfer object should also have isDrag property.
Change History (12)
comment:1 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 10 years ago by
Owner: | set to Piotr Jasiun |
---|---|
Status: | new → assigned |
comment:3 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:4 Changed 10 years ago by
comment:5 Changed 10 years ago by
Status: | assigned → review |
---|
comment:6 Changed 10 years ago by
Status: | review → review_failed |
---|
textToHtml
- Name
transformPlainTextToHtml
(the same in the method's doc). BTW. Docs should be written in 3rd person - i.e. 'transforms' instead of 'transform'. - This method bases its behaviour on two parameters - selection's position and enter mode. I see now that it does not make much sense in editable class, because e.g. it's impossible to use it for selection placed in other place than where selection is currently placed. Therefore, please move it to tools and make the 2nd arguments options object (with properties: enterMode, inPre).
- Where are tests for this method?
- Name
- I think that our
dataTransfer
interface should suppress native errors. The getData method should usetry-catch
and returnnull
or empty string (depending on how it works natively) if error was thrown on IE. Specifically to avoid this: https://github.com/cksource/ckeditor-dev/blob/89063ac575cf110d9b0a6387f3f983ce7a9e33b2/plugins/clipboard/plugin.js#L1351. On the other hand try-catch is already used here: https://github.com/cksource/ckeditor-dev/blob/89063ac575cf110d9b0a6387f3f983ce7a9e33b2/plugins/clipboard/plugin.js#L1876 so it may be just a bug. - https://github.com/cksource/ckeditor-dev/blob/89063ac575cf110d9b0a6387f3f983ce7a9e33b2/plugins/clipboard/plugin.js#L1358 - don't set it on the object if you are going to change it few lines below. When reading this I wanted to comment that - "wait, traveler, you forgot to transform plain text to HTML". Only after few whiles I noticed what happens below. Use a local variable while still processing it.
- https://github.com/cksource/ckeditor-dev/blob/89063ac575cf110d9b0a6387f3f983ce7a9e33b2/plugins/clipboard/plugin.js#L1369 - fix indentation.
- https://github.com/cksource/ckeditor-dev/blob/89063ac575cf110d9b0a6387f3f983ce7a9e33b2/plugins/clipboard/plugin.js#L1821 - empty line missing.
- https://github.com/cksource/ckeditor-dev/blob/89063ac575cf110d9b0a6387f3f983ce7a9e33b2/plugins/clipboard/plugin.js#L1736 - extract this method from the constructor.
- https://github.com/cksource/ckeditor-dev/blob/89063ac575cf110d9b0a6387f3f983ce7a9e33b2/plugins/clipboard/plugin.js#L1736 - why private?
- https://github.com/cksource/ckeditor-dev/blob/89063ac575cf110d9b0a6387f3f983ce7a9e33b2/plugins/clipboard/plugin.js#L1897 - here and in other places don't use variable
t
. Usetype
from the first moment, because you don't needt
after normalising it. - https://github.com/cksource/ckeditor-dev/blob/89063ac575cf110d9b0a6387f3f983ce7a9e33b2/plugins/clipboard/plugin.js#L1904 - method could return true/false depending on whether data was set. It could be a useful feedback.
- Split setData getData tests into multiple, properly titled, test cases.
- https://github.com/cksource/ckeditor-dev/blob/89063ac575cf110d9b0a6387f3f983ce7a9e33b2/tests/plugins/clipboard/datatransfer.js#L289 - avoid setting such HTML, because it's incorrect and will be auto paragraphed. This leads to tests being less clear and of course bugs.
comment:7 Changed 10 years ago by
Status: | review_failed → review |
---|
Changes done and pushed.
transformPlainTextToHtml
needs only enterMode as an option so I decided no to create option object for one option. In the future if we will have more options we can check the type of the parameter.
dataTransfer.setData
will always work, even if native method will not, so it is pointless to return result.
comment:9 Changed 10 years ago by
Status: | review → review_failed |
---|
- You forgot to remove editable#textToHtml method.
- I see that the code which decides which enter mode should be used while transforming text o HTML is repeated twice - in clipboard plugin and in editable. This means that we need editable#transformPlainTextToHtml which will do this and which will use tools.transformPlainTextToHtml.
- You forgot the @since tags.
- https://github.com/cksource/ckeditor-dev/blob/de348e9ded089a5f8111d127d71eed7096c3bd40/core/tools.js#L382 - don't mention paste position, because you don't know context in which this method will be used - it'd a general purpose transform function.
- https://github.com/cksource/ckeditor-dev/blob/de348e9ded089a5f8111d127d71eed7096c3bd40/core/tools.js#L383 - "Transformed" indicates that this is the HTML that has been transformed, while, in real, it was generated.
- https://github.com/cksource/ckeditor-dev/blob/de348e9ded089a5f8111d127d71eed7096c3bd40/core/tools.js#L383 - start comment from the upper case.
- https://github.com/cksource/ckeditor-dev/blob/de348e9ded089a5f8111d127d71eed7096c3bd40/core/tools.js#L467 - unnecessary semicolon. Actually, there are 5 linter errors.
- tools#getUniqueId does not have tests. Please write at least one that will verify e.g. type of returned value and its length.
If any of these points conflicts with code created in branches based we can extract it to a separate ticket and fix afterwards, because none is critical.
comment:10 Changed 10 years ago by
Status: | review_failed → review |
---|
Changes done and pushed to t/12168.
comment:11 Changed 10 years ago by
Status: | review → review_passed |
---|
comment:12 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Closed with hash git:cb28e9c.
dataTransfer
may have no native object,insertText
to transform text to html instead of simplehtmlEncode
.Now dataVaule and type are set, based on dataTransfer data, just before firing paste. It will be moved to the
paste
listener in #12173, but now paste event is not firing ifdataValue
is empty so we need to set it earlier.Changes in t/12168.