Opened 11 years ago
Closed 10 years ago
#11460 closed New Feature (fixed)
Handling dropping content in editor
Reported by: | Piotrek Koszuliński | Owned by: | Piotr Jasiun |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.5.0 Beta |
Component: | General | Version: | |
Keywords: | Cc: |
Description (last modified by )
Part of #11437.
Spec (status: proposal)
- Editor#drop event has to be introduced. It has to be able to carry the same data as a native drop event, so e.g. multiple files as some blobs if spec assumes that (http://www.w3.org/TR/2010/WD-html5-20101019/dnd.html#dnd should be checked to see what's possible). So the first step of normalisation has to be done before firing editor#drop. Alternatively, a wrapped native event may be carried, in which normalisation would be done on demand in its getter methods. The decision has to be made depending on browsers' implementations compatibility. I guess that differences may be so significant that simple wrapper would not make sense. In both cases a class representing new data type has to be introduced, so in fact the final solution may be something between simple wrapper and initial normalisation.
- After drop was fired, if it wasn't cancelled, editor#paste has to be fired so editor#paste will be the single entry point for content inserted by user.
- Editor#paste accepts only HTML strings and it seems to be very unsafe to change this. So if non-HTML content has been dropped, it has to be translated to HTML string. For example - dropped image should be translated to '<img src="..." alt="">'. The source will be a reference to a blob carried as additional drop event data (files list). Editor#paste can be fired only if #drop returned a value of HTML type. See point 6.
- A general idea may be that drop event data is a value-type pair (+ additional value-types pairs for additional files, etc. if that will be required - see 1.) and the translation from non-HTML type to HTML string will mean changing value to HTML string and type to 'html'.
- The default translation should be done by a #drop listener with high priority (>10) or after #drop. It should be done only if #drop does not yet carry a data in HTML type. This way we'll allow overriding default translation behaviour, by attaching #drop listener performing it earlier. Also, in my opinion it will be better if default translation was done by a #drop event listener too, because it will allow to change something after it happened.
- We still need to analyse where to put what part of this feature. For example it would be nice if we could add basic dnd support to all standard presets and make the full support with files support optional. The basic part should be able to work without the optional part.
- Uploading files has to be handled by #paste (because files can be pasted and dropped and #paste is the single entry point). Ticket: #11461.
- Widgets dnd will have to be aligned to changes made in this ticket. Ticket: #11219.
Attachments (12)
Change History (51)
comment:1 Changed 11 years ago by
Description: | modified (diff) |
---|---|
Status: | new → confirmed |
comment:2 Changed 11 years ago by
Owner: | set to Piotr Jasiun |
---|---|
Status: | confirmed → assigned |
comment:3 follow-up: 4 Changed 11 years ago by
comment:4 Changed 11 years ago by
Replying to Reinmar:
During today’s discussion with PJ we raised a number of topics which need further discussion or actions or just should be emphasised.
- Remark: Paste event may contain DataTransfer object and files list similarly to drop event. Even if it doesn’t contain them currently, it may in the future and we should take that into consideration. This makes both events very similar.
That's a good point.
- Task: We need full research on current implementations of drop and paste events
Full table of what data (including types) are carried for what type of insertions (both - dropping and pasting) on which browsers is needed. It will help to make decisions regarding API and testing. It may become a compatibility table after the job.
Scenarios (dropping/pasting):
- HTML content,
- HTML content with images from outside of editable area,
- one image from system file browser,
- many images from system file browser,
- part of image (data) copied (and dropped if that’s possible) e.g. from image editor (e.g. MS Paint),
- content of MS Word document with images,
- text file from system file browser,
- content of text file.
Research is avaible in #11526.
Focus should be put on what data types are available in drop/paste event and what files are available in files property. Sample used for testing should be attached in this ticket.
- Task: We need a short excerpt from both specifications (clipboard and dnd). In previous point I mentioned a compatibility table which will clarify how the situation looks now. But it may change in the future, so specs should be checked.
Well... the problem now is that specification is far from reality. Event in introduction they are writing about attributes and solutions not supported in some or any browser (like dropzone
or custom data type). So probably one day in the future browsers will be closer to the specification but it is far future. The main problem is (of course) IE. Anyway W3C APIs are described in #11526.
- Question: do we need “drop file here” area? File should be dropped under the cursor, just like inline widget is dropped. It works this way when I drop image into TextEdit - I see a caret and image is inserted at that position. It works differently in Gmail, but image is an attachment there, so it’s a different case. MS Word should be checked.
In Gmail if you drop image it appear inside a content, not as an attachment (we tested it with Linux, Windows and Mac). If you D&D file in different type then it goes to the attachment. In MS Word it works like text D&D. Anyway this is more UX think so I will move this discussion to mail and involve Anna and Sebastian into discussion.
From the technical point of view I can not get files details until file is dropped. In dropover
event I can get only information that some "Files" are over droparea, so I cannot show “drop file here” information only for supported file types. What I can is to change the message just after user drop files from "drop file here" to "this file type is not supported" and hide droparea after some seconds.
- Question: isn’t my proposal for drop event being forwarded to paste ridiculous? As I mentioned in first point paste is very similar to drop. This means that both should be handled similarly, but definitely not that drop should fire paste. For example, pasting file is (or may be - see point 3.) the same as dropping it - there may be no HTML representation available, so transformation may have to be performed - the same as for file drop. Would we transform files twice or would we do it only during paste event? Both options are hard to justify, so let’s start from beginning.
We need a single event for data insertion - that’s definitely a truth. But our mistake was to accept paste as it. Paste is a paste, drop is a drop - they are similar, but different actions. We need a new event to handle them, therefore events chain should rather look like that:
- paste -> afterPaste -> insertData
- drop -> (afterDrop) -> insertData
Shared transformations would be done on insertData and insertData would also finalise insertion by calling editor.insertXXX. Action specific transformations may still be done specifically for past or drop. For example, on some browser we’ll still use pastebin, so we’ll need “HTMLified text normalisation” - this will be done on paste only. However, “plain text to HTML transformation” which will cover pasting and dropping plain text may be done in insertData unless HTML content is already available in event’s data.
The downside is that such changes will break backward compatibility at some point. It may turn out that we’ll be able to keep most of current paste event’s behaviour, but some things will change definitely. I don’t see other option though, because trying to keep current code intact will result in a ridiculous solution. So - good design is first, but backward compatibility is important and should be kept where possible.
Another downside is that we may need to touch a lot of clipboard code now. Theoretically, my idea could be implemented without touching paste event at all (the only difference is that afterPaste has to fire insertData), but making it compatible with drop event may require some modifications, especially in order to keep good code organisation.
The biggest problem, in my opinion, is that we have two different data structures now.
- on the one hand we have our old paste event which has only one dataValue in one type (html/text/auto),
- on the other we have rich
DataTransfer
which can contain multiple types of data and files in the same time.
And there are two thinks we can do with if this:
- create some addition step of data transformation when the
DataTransfer
will be transformed to the html (ex. dropped image file will be read and change toimg
element) and such html will be pushed to the current paste event, - modify current paste event so it contains all of the date (files and multiple data types), it this case we should keep old properties for backward compatibility.
Currently DataTransfer
does not contain a lot information in paste event, it works more with drop
then paste
. Also we are going to work on CKEditor 5 soon so it might be a solution to focus on drop event now and support rich paste
event in CKE5.
comment:5 Changed 11 years ago by
After the #11526 research I think that drop handling should work this way:
- when user drag contains inside editor mark it with some ID (we can find dragged content using selection on drag) and put this ID into dataTransfer,
- when drop event occurred check if it is you ID;
- if not: read html or text (if html is not available) and put in drop position,
- if it is our ID: read html for our ID, remove dragger content and put html in the drop position.
In both situation we need to cancel a drop event. There are two thing we will lose with such solution. When user drag content from the outside of the editor:
- on IE, where we have no html in
dataTransfer
object, content will be dropped as plain text, - we will lose native drag behavior so in every case drag content will not be removed.
comment:6 Changed 11 years ago by
Based on PJ's research made in #11526 we discussed today the possible design of this feature. We agreed on most of points, although I feel that it's so complex and tricky that we need to have a "running spec" in which we'll be keeping track of all decisions.
Arguments and decisions
- Because of time limits and advanced age of 4.x branch, we cannot redesign entire input processing. Such steps can be done in future major release. We pointed out that it's unlikely that it will happen in CKEditor 5.x, because solution we're going to introduce in 4.4 will be satisfactory.
- IE 11 is as bad as IE 8 in terms of offered APIs and their completeness. Therefore, situation won't change significantly in couple of years.
- Current behaviour of
editor#paste
event must be preserved in as many details as possible. - There should be a single entry point.
- If it's possible
editor#paste
should not be fired byeditor#drop
, what means that there would have to be a new event (editor#inputData
). - IEs do not allow to get HTML from
drop
andpaste
events. We still need pastebin for IEs, because we have to support pasting HTML from multiple sources. In case of drag&drop, we need to choose between preventing default drop (what would allow to handle it in a custom way) and being able to drag&drop HTML from outside editor into editor. We'll be able to handle drag&drop within editor (and even all editors on a page - although there's problem with things like filtering widgets in case of differently configured instances) by overriding carried data (only text format is available) by a reference to stored dragged HTML. Therefore, I think that we may accept the limitation that HTML is plain textified when drag&dropped from outside of editor. - Drop and paste data cannot be read asynchronously (after timeout). Data from pastebin is read after short timeout after native
paste
... We've got a conflict of interests here. - The additional data items that
editor#paste
andeditor#drop
events will carry should be facades to nativeDataTransfer
objects. Theoretically we could cache data retrieved from native objects, but: a) it doesn't seem right, b) it's asynchronous what complicates the thing, c) we'll completely block possibility to use some File APIs (e.g. the ability to upload a blob rather than base64 encoded string to a server and couple of others like progress listener), d) it may be unnecessarily heavy. Therefore, to avoid such situation our data facade has to keep reference to native object. - Because of two previous points we cannot use pastebin and offer items data at the same time. On problematic browsers like IE we can try to discover if
paste
may contain HTML (ifgetData('Text')
returns something) and in this case use pastebin; if not, then we can skip pastebin and offer files data. Since, as explained in one of previous points, we may accept that only text can be dropped from outside of editor, that discovery has to be done for pasting only. - If we wouldn't use pastebin at all on Blink/Webkit/Gecko (what's tempting because of bugs it causes and its complexity), we'll need to solve two problems:
- Webkit&Blink pastes/drops awful HTML overloaded with inline styles. The only possible way to handle this is by stripping all styles. This could be configurable, because in basic/standard presets ACF will strip all of them too, but by default such filter would have to enabled. There's also a light in the tunnel - perhaps we could override clipboard's data on copy/cut from editor with our, specially marked HTML string and then we wouldn't have to do a styles attribute filtering when copy/cut&paste was done within editor.
- There's no HTML version of dropped and pasted plain text. Currently we unify "HTMLified text" versions pasted by browsers into pastebin. Every browser generate different HTML, so we've got a hardcoded filters for discovery and transformations. We'll be able to get rid of most of it (leave only parts for IE, which surprisingly produces best HTML) and write simple "HTMLification processor" turning plain text into HTML.
- However, we may still use pastebin if no files are pasted (similar discovery as in IE case), so problems explained above would not exist. Unfortunately, they will still exist for dropping plain text and HTML, so we have to solve them anyway.
- Editor should add class to editable on
dragover
, so developers can add special styles to mark such editable.
Design
Data flow
- Step one - data capturing
- The goal is to pass files list with data transfer facedes, HTML data string, and type to
editor#paste
andeditor#drop
. - There's a decision made whether we need to use pastebin or there are some files so we can't.
- We retrieve data stored on
dragstart
(IE case). - We remove dragged content from editor.
- If there was a plain text pasted/dropped we HTMLify it.
- All data - HTML data string, type (
'auto'
by default,'text'
if it was plain text,'html'
if there was no content other than files) and files list are passed toeditor#drop
oreditor#paste
.
- The goal is to pass files list with data transfer facedes, HTML data string, and type to
- Step two - data processing
- Includes: type discovery (text, html), plain text htmlification/normalization, html textification (
forcePasteAsPlainText
), etc. - We need this step during
editor#paste
andeditor#drop
, so we'll not need to change currenteditor#paste
behaviour. - Methods used currently by
editor#paste
listeners have to be made public unless this part ofeditor#drop
is implemented in the clipboard plugin.
- Includes: type discovery (text, html), plain text htmlification/normalization, html textification (
- Step three - files processing
- Can be done in
editor#inputData
, because it's a single entry point. Although, since the rest of processing happen oneditor#drop
andeditor#paste
we may consider processing files on them too. - During this step listeners check files list and if they can handle any of pasted/dropped files, they add HTML generated for that content to the HTML data string. For example "image handler" will add
<img data-cke-file-id="rand" alt="" src="CKEDITOR.tools.transparentImageData">
and will replacesrc
attribute with data URI retrieved (asynchronously!) from file, based ondata-cke-file-id
if it survived ACF and other filters. There has to be a way to hook file upload to that too. File upload might start at this point, so it will have access to data transfer facades. - It may be convenient to be able to mark that file was already "handled", so other listeners will not duplicate it.
- It's unclear yet how image handler and uploader could communicate with image2, because replacing its source is not enough - data has to be updated. However, problem would not exist if that handled would be added by image2 itself.
- Can be done in
- Step four - single entry point
editor#inputData
is fired synchronously after execution ofeditor#drop
andeditor#paste
with all their data (files lists, HTML data, type and custom properties).- It's fired in its 1000-prior listener, so
editor#paste
oreditor#drop
can be fired by other code and the chain will be finished.
- Step five - data insertion
- If HTML data string is not empty, insertion is performed synchronously after
editor#inputData
(in its 1000-prior listener, soeditor#inputData
can be fired by other code and the chain will be finished). editor#afterInputData
is fired after insertion. Additionallyeditor#afterPaste
is fired. Currentlyeditor#afterPaste
is fired asynchronously "so all other listeners for 'paste' will be fired first". It doesn't make sense, but on the other hand changing this would be hard, because synchronously aftereditor#paste
we e.g. fireafterCommandExec
.editor#afterPaste
andeditor#beforePaste
should be marked as deprecated. First is replaced byeditor#afterInputData
, second is only a problem for us and most likely is useless.
- If HTML data string is not empty, insertion is performed synchronously after
- Step six - files upload
- Since at this point
- After files data have been collected and HTML was inserted, file uploading begins. It may also begin earlier, when files are available, but it seems to be more correct to start uploading after insertion. However, in this case we need to find a way to cache files that might be uploaded, so their content (blob and dara URI) are still available here.
- Elements referencing to those files are still marked with
data-cke-file-id
attribute. - Uploading file may take more time, so at every step we need to verify that these elements still exist (simply, by using
document.find()
instead of caching them). - Elements may be duplicated before uploading completes, so
data-cke-file-id
has to be preserved and situation when more than one element with this attribute exist has to be considered. - File id should be correlated with editor instance - when content was pasted into another editor, attribute must be removed and new upload thread has to be launched. The reason is that editors may have different configuration regarding files handling and uploading, so we cannot assume that it should be the same URI.
UI
- We may consider styling editable when class marking editable on dragover is present.
- We need a convenient place to communicate with user about uploading process. We thought about "toasts" - small notification boxes displayed in the bottom right corner of editable. Toasts system may use floatpanels to render and should have API to add notification and close it. Notification's content needs to be an HTML. We may implement simple version of toasts at the beginning (e.g. skip such tricky cases like keeping them in the viewport, when editable isn't fully visible and more options regarding positions). The behaviour (on hover, close, etc.) may be precised later and is not crucial now. In the future we could also use special toasts instead of alerts (there is a ticket about stopping using alerts).
- We are not considering displaying inline information about uploading progress because of undomanager, dirty flag and other complications.
Code separation
It's not yet clear how do we want to distribute code between plugins.
- There may be developers which don't want even basic drop support (I don't think so, but there are e.g. touch devices where I'm unsure about drag&drop support). I would keep basic drop support in clipboard plugin because they'll share a lot of code anyway.
- Where do we want to keep file handlers? We'll have at least one - for image, but we may have also for other files (which generates links). I think that they should be kept in image, image2 and link plugins. See also one of points in step three of data flow regarding communication with image2 widget.
- Files uploading is a separate plugin.
- Toasts are separate plugin.
- What's left?
comment:7 Changed 11 years ago by
About clipboard facade and timeout. In fact it is not possible to read from clipboardData
, dataTransfer
or items
(new API, Chrome). But we can easily save, for future usage, file references as files
array or item.getAsFile()
and read files contents whenever we need. I tested it and works fine in all cases (paste and drop in Chrome, FF and IE).
BTW I love when browsers use common API:
files = evt.dataTransfer ? evt.dataTransfer.files : // Drop evt.clipboardData && evt.clipboardData.items ? [ evt.clipboardData.items[0].getAsFile() ] : // Paste Chrome evt.clipboardData ? evt.clipboardData.files : // Paste Firefox window.clipboardData ? window.clipboardData.files : // Paste IE undefined;
Changed 11 years ago by
Attachment: | keep-files.html added |
---|
comment:8 Changed 11 years ago by
There a problem with drag position on IE, because:
- on drag selection sometimes is in correct place but sometimes in totally wrong position or not exists at all,
range.moveToPoint
method throwsUnspecified error
when you are not directly on contents (eg. cursor is next to the header); if I call it onmouseover
I get more errors than positions and the last good position is usually far from expectation,- it is not possible to change native drop content so I can not change it to some hidden
span
and change after native drop.
My idea to solve this problem is to use elementFromPoint
method which works pretty well even if cursor in next to the element. So if moveToPoint
throws error I can get nearlest element using elementFromPoint
and set the cousor at the begging or end of the element (based on cursor position).
Also we will need to fix dragged selection before we remove it, because if user select part of the table and drag and drop it, script will remove part of cells what causes crash on IE 11.
comment:9 Changed 11 years ago by
Summary: | The editor#drop event - handling dropping content in editor → Handling dropping content in editor |
---|
comment:10 Changed 11 years ago by
I implemented basic D&D and check how it works in browsers. There are numerous errors with getting drop position in browsers:
FF: wrong drop position when I drop at the end of the paragraph.FF: "TypeError: startNode is null" when I drop at the end of the paragraph content from the same line.IE8 & IE11: "unspecified error" (e.g. when I drop content next to the "Broadcasting and quotes" header.).IE8 & IE11: "index size error" (specific case not yet investigated).IE8: drag content does not disappears sometimes.IE8: random crash.IE8: D&D in the same paragraph does not work.
- Chrome: everything is fine.
As long as we can fix most of this issue with simple workarounds, "unspecified error" on IE is the most popular error and the one we cannot easily avoid (for the reasons mentioned in the previous post).
So my proposal is to use custom code, based on document.elementFromPoint()
and range.getClientRects()
(which works surprisingly good in IE). I created prove of concept and it works on IE11-IE9 far better then native code. It is based on the standard range
so it does not work on IE8 yet. I tested range.getClientRects()
on IE8 and it works, so custom solution should work on IE8 as well.
To show you how both solution works there are them:
Of course the final version would be a mixed so it will use native methods when they works and custom when the do not. This is just a prove of concept. What do you think about it?
comment:11 Changed 11 years ago by
First of all we must have a really clean implementation for retrieving range from native sources on drop and retrieving selected HTML on dragstart. Otherwise we're not able to evaluate these parts. The current proposal in branch:t/11460-native contains already too much integration code which may cause issues and it makes testing of these separate parts harder.
For example I see that at some point you set selection after drop, but before retrieving the range at drop position. Such operation may have unpredictable influence on DOM and browsers' internal states. Then you changed the order of actions, but the situation is still far from being clear. You retrieve range, then play with selection and then try to use this range - that can be unpredictable.
Therefore, what we need is:
- Retrieving HTML on dragstart. The sample could log retrieved HTML on console.
- Remembering dragged selection. Since the standard bookmarks are more reliable, I would check if we couldn't use them instead of bookmarks2. Of course inserting spans may influence locating drop, but we'll check this in a next step. First we have to find out if we can make bookmarks on drag start and restore them on drop. Please make two versions of this test case - one using
createBookmarks()
and one usingcreateBookmarks2()
. After restoring ranges from these bookmarks usedeleteContents()
to see if all this worked. - Retrieving drop position. Again - just retrieving, without previous steps. Insert some visible pointer (e.g. a letter) into that range. Check both methods - using the
getRangeAtDropPosition
borrowed from widget plugin and simpleeditor.getSelection()
.
The easiest way will be to create separate test files for all this cases. You don't need to push branches, just attach here sample HTML files.
PS. Remember that if you have found a bug in an existing method which you need to use, we have to fix that method first. You cannot proceed with a broken part of your implementation because we won't be able to evaluate later the entire component. Therefore, report tickets for such bugs, create test cases and notify me or Olek. Such ticket will be considered a blocker so we'll try to find a solution ASAP.
Changed 11 years ago by
Attachment: | keepDragSelectionWithBookmarks.html added |
---|
Keep drag selection with bookmark.
Changed 11 years ago by
Attachment: | keepDragSelectionWithBookmarks2.html added |
---|
Keep drag selection with bookmark2.
Changed 11 years ago by
Attachment: | keepDragSelectionWithRanges.html added |
---|
Keep drag selection with ranges.
Changed 11 years ago by
Attachment: | readHtmlOnDragstart.html added |
---|
Read HTML on 'dragstart' and write it to the console.
Changed 11 years ago by
Attachment: | getDropPositionUsingGetSelection.html added |
---|
Get drop position using native selection.
Changed 11 years ago by
Attachment: | getDropPositionUsingWidgetFunction.html added |
---|
Get drop position using function borrowed from widgets.
Changed 11 years ago by
Attachment: | getDropPositionUsingCustomSrcipt.html added |
---|
Get drop position using custom script based on elementFromPoint and getClientRects
Changed 11 years ago by
Attachment: | getDropPositionUsingGetSelection-IE8-bugs.webm added |
---|
Changed 11 years ago by
Attachment: | getDropPositionUsingWidgetFunction-IE8-UnspecifiedError.webm added |
---|
comment:12 Changed 11 years ago by
Description: | modified (diff) |
---|
I created set of samples for problems mentioned above.
- Retrieving HTML on dragstart.
- readHtmlOnDragstart.html. Works pretty well. Of course there are some edge cases (like fake selection) but these could be fixed later. I don't see any blocking issues here.
- Remembering dragged selection.
- keepDragSelectionWithBookmarks.html. Does not work on Chrome. When you add span on dragstart D&D stop working. Also there would be a problem when user drag element and do not drop it, we need to remove
span
s then. - keepDragSelectionWithBookmarks2.html. Works pretty fine, but needs more tests.
- keepDragSelectionWithRanges.html. Works as well. Since document elements does not change during D&D, range seems to be better way to keep drag selection than
bookmark2
.
- keepDragSelectionWithBookmarks.html. Does not work on Chrome. When you add span on dragstart D&D stop working. Also there would be a problem when user drag element and do not drop it, we need to remove
Both
bookmark2
andrange
needs to be changed to thebookmark
after drop, because we need to create abookmark
for a drop position which can destroybookmark2
orrange
.
- Retrieving drop position.
- getDropPositionUsingGetSelection.html.
- On Chrome selection is on the drag, not drop position (but on Chrome we can retrieve drop position easily using different methods).
- On IE 11 it works fine, selection is on the drop position.
- On IE 8 it is buggy. Usually selection is in the good position but I found two bugs in a couple of minutes.
- getDropPositionUsingWidgetFunction.html. It is also buggy on IE. The problem is an "unspecified error" from "moveToPosition" (screencast).
- getDropPositionUsingCustomSrcipt.html. Works surprisingly fine on IE9-11 (it is not ported to the IE8 yet).
- getDropPositionUsingGetSelection.html.
comment:13 follow-up: 14 Changed 11 years ago by
Good job with the samples. They are really useful.
- Retrieving HTML on dragstart.
- IE8 says
undefined
when dragging image or table (control selection). Works fine on IE11, I didn't check the rest. - All browsers return only text when dragging part of selected link, when native dnd inserts dragged text as a link.
- Chrome returns only text when dragging entirely selected link. Other browsers returns link.
- In my opinion we need to fix our
range#cloneContents()
so it does work well in this case. This method is not used in ANY place in editor, so we can tune it a little bit.
- IE8 says
- Remembering dragged selection.
- I'm ok with keeping reference to range. On
drop
, ifrange.select()
throws error we should stop entire process, because it meant that something violated DOM. Better to do nothing then.
- I'm ok with keeping reference to range. On
- Retrieving drop position.
- GetSelection indeed doesn't work - that's because we prevent default action. Dead end.
MoveToPoint
indeed throws errors. Unfortunately it does it on every IE, but luckily I found only two cases, although pretty often.- Custom script doesn't work for me. I recorded customRangeFromPointBugs.webm which shows that this method isn't any better than
moveToPoint
. Actually, it is worse, it's long even though IE8 is still not supported there, so it's not a good choice. - Therefore I've got a different proposal. Since moveToPoint mostly fails when dropping between blocks of text, why not looking for a range up and down? With a step e.g. 2px look up, if moveToPoint failed, look down, if failed again, look up 4px, then down, etc, up to e.g. 30px. This way we'll find a position in the nearest block, so exactly what IE visually showed us. We may not find anything useful on the right side of anchor (e.g. in the "Broadcasting and quotest" header) - in this case we can back up with
elementFromPoint
and place selection at the end of it (remember about bogus <br> in IE11). That of course may still not be enough to handle all cases, but I think that it will be good enough in general. - We may also check other method - if
moveToPoint
failed we can let IE does its native drop, then get selection, and most often we'll find dropped content inside it. However, this may cause blinking and may be unreliable as well. So if previous option is going to work I'll be for it.
Changed 11 years ago by
Attachment: | customRangeFromPointBugs.webm added |
---|
comment:14 Changed 11 years ago by
Replying to Reinmar:
Good job with the samples. They are really useful.
- Retrieving HTML on dragstart.
- IE8 says
undefined
when dragging image or table (control selection). Works fine on IE11, I didn't check the rest.- All browsers return only text when dragging part of selected link, when native dnd inserts dragged text as a link.
- Chrome returns only text when dragging entirely selected link. Other browsers returns link.
- In my opinion we need to fix our
range#cloneContents()
so it does work well in this case. This method is not used in ANY place in editor, so we can tune it a little bit.
I'm not sure if we can "fix" range#cloneContents()
or we need to write it from scratch. What cloneContents()
basically do is calling execContentsAction
what is pretty complex function used also by some other functions. This function was designed that way it can change range, so it might happen. And if we would need to write new range.cloneContents
method just to use it in selection.getSelectedHTML
maybe we should focus on getSelectedHTML
. If it really need range.cloneContents
we should write it but this is not a method we should focus on.
- Remembering dragged selection.
- I'm ok with keeping reference to range. On
drop
, ifrange.select()
throws error we should stop entire process, because it meant that something violated DOM. Better to do nothing then.- Retrieving drop position.
- GetSelection indeed doesn't work - that's because we prevent default action. Dead end.
MoveToPoint
indeed throws errors. Unfortunately it does it on every IE, but luckily I found only two cases, although pretty often.- Custom script doesn't work for me. I recorded customRangeFromPointBugs.webm which shows that this method isn't any better than
moveToPoint
. Actually, it is worse, it's long even though IE8 is still not supported there, so it's not a good choice.- Therefore I've got a different proposal. Since moveToPoint mostly fails when dropping between blocks of text, why not looking for a range up and down? With a step e.g. 2px look up, if moveToPoint failed, look down, if failed again, look up 4px, then down, etc, up to e.g. 30px. This way we'll find a position in the nearest block, so exactly what IE visually showed us. We may not find anything useful on the right side of anchor (e.g. in the "Broadcasting and quotest" header) - in this case we can back up with
elementFromPoint
and place selection at the end of it (remember about bogus <br> in IE11). That of course may still not be enough to handle all cases, but I think that it will be good enough in general.- We may also check other method - if
moveToPoint
failed we can let IE does its native drop, then get selection, and most often we'll find dropped content inside it. However, this may cause blinking and may be unreliable as well. So if previous option is going to work I'll be for it.
Sounds good, let's try some mixed solution.
comment:15 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:16 follow-up: 17 Changed 11 years ago by
I'm not sure if we can "fix" range#cloneContents() or we need to write it from scratch. What cloneContents() basically do is calling execContentsAction what is pretty complex function used also by some other functions. This function was designed that way it can change range, so it might happen. And if we would need to write new range.cloneContents method just to use it in selection.getSelectedHTML maybe we should focus on getSelectedHTML. If it really need range.cloneContents we should write it but this is not a method we should focus on.
I'm actually curious how cloneContents works currently. There's a small chance that it was intended to be used in such cases - it's hard to tell now. But even if not, implementing it from scratch is acceptable. Note that implementing getSelectedHtml requires having something similar to cloneContents (even in readHtmlOnDragstart.html you used cloneContents, although a native one).
Changed 11 years ago by
Attachment: | getDropPositionUsingMixedSolution.html added |
---|
Final candidate for getRangeAtDropPosition
comment:17 Changed 11 years ago by
Replying to Reinmar:
I'm not sure if we can "fix" range#cloneContents() or we need to write it from scratch. What cloneContents() basically do is calling execContentsAction what is pretty complex function used also by some other functions. This function was designed that way it can change range, so it might happen. And if we would need to write new range.cloneContents method just to use it in selection.getSelectedHTML maybe we should focus on getSelectedHTML. If it really need range.cloneContents we should write it but this is not a method we should focus on.
I'm actually curious how cloneContents works currently. There's a small chance that it was intended to be used in such cases - it's hard to tell now. But even if not, implementing it from scratch is acceptable. Note that implementing getSelectedHtml requires having something similar to cloneContents (even in readHtmlOnDragstart.html you used cloneContents, although a native one).
I created a ticket for this #11636.
comment:18 Changed 11 years ago by
Milestone: | CKEditor 4.4 → CKEditor 4.5 |
---|
comment:19 follow-up: 20 Changed 11 years ago by
The problem with this ticket for now is that we have two (or more) ranges: for drag (might be multiple) and for drop. Both of then should be modified but when we modify one we can lost access to the second one (offset could be no more right), even if the modification is creating a bookmark.
For this reason we need to figure out which range in "lower" in the document (greater offset) and create bookmark for this range first. The problem is that it is not trivial which range is "lower" ex. one range could be in a text node, but the second one at the end of paragraph. We need to handle all of the cases.
comment:20 Changed 11 years ago by
Replying to pjasiun:
The problem with this ticket for now is that we have two (or more) ranges...
But what's the exact TC and what branch it can be reproduced?
comment:21 Changed 11 years ago by
I managed to fix all of the known bugs with custom D&D inside CKEditor and now I'm testing D&D data from the outside. I realize that there is a problem because the way I get a drop position on IE9+ (editor.getSelection().getRanges()[ 0 ]
) does not work as I expected in such situation. On the other hand pure JS:
document.body.ondrop = function ( evt ) { window.focus(); document.selection.createRange().text = "!!!" return false; }
works perfectly fine on native content editable. I will debug why CKEditors methods do not work properly.
After this I have to fix selection after drop on IE8 and custom D&D will be ready for the alfa testing (if I will not find any new important bugs).
comment:22 Changed 11 years ago by
I fixed all issues mentioned above and do not found any new. Now the ticket need some clean up and tests (both manual and unit).
comment:23 Changed 11 years ago by
Status: | assigned → review |
---|
I've cleaned code, added tests (both unit and manual), code comments, support for cross-editor D&D and fixed some more issues so the ticket is now ready for review, but it should be a special review.
Apart from code review we need to do manual test phase and check is everything is fine on every supported browser. I created mt/11460.html
for this. This is the best moment to fix issues because there is no abstract clipboard object over D&D.
Also there are some known issues which are not part of this ticket:
- because of #11636 dragged content is not correct in some cases (e.g. when you drag part of the link),
- drag position needs clean up after D&D (e.g. remove empty paragraphs, fix table),
- drop position needs clean up after D&D (e.g. add spaces before/after dropped content, apply parents styles, break paragraph when one paragraph is dropped at the end to the other paragraph),
- in the external D&D: Chrome add plenty of addition tags.
Changes in t/11460 and corresponding test branch.
comment:24 Changed 10 years ago by
Status: | review → assigned |
---|
comment:25 Changed 10 years ago by
During testing phase our great testers found bunch on bugs to fix. Full list of found bug is available here:
https://docs.google.com/document/d/1hG4H0r21MXNkRd3amDEOBPygJe3ehBAXFWAal2DptGQ/edit?usp=sharing
Bugs which will be fixed or checked as a part of this ticket are:
error when drop in the same text node where drag selection ends (drag selection start in different node) (Bug 1, Bug 13),- IE9: D&D somewhere text with anchor, D&D this anchor again, you will get error (Bug 5),
- IE9, Opera: content: "abcd", drag "ab" after "cd" ("cdab"), drag "da" after "b", error (Bug 8).
cke-1398780468602 after drop into the textarea (Bug 11),IE11: drop external html to inline does not work (Bug 23),- IE8: editor is not focused after dropping from external source. Cursor stays in the source editor when dragging between editors (Bug 25).
IE8: Select "Apollo 1" (bolded) and drop in "was" -> error. Generalized - drop after the dragged text. (Bug 26).- IE8: Select second list item and drop it at the beginning of first -> error. Dragging from a list causes errors all the time (Bug 27).
red tests on OSX and on Opera- IE8: Image disappears during D&D. (Bug 28).
comment:26 Changed 10 years ago by
Status: | assigned → review |
---|
Fixing the issue with the ID in the textarea I realize that code became too complicated and needs refactoring to make it readable.
I created dataTransfer
facade object and move there part of the logic. Now this object contains only methods needed at this stage. I the future getData
and setData
methods will be modified to support custom data types on all browsers and sourceRanges
will be probably moved somewhere else. But this is the part of the different ticket.
Now I will work on bug fixing and on tests to create some more and fix red tests on OSX.
The architecture of the solution is ready for the review.
comment:28 Changed 10 years ago by
Status: | review → review_failed |
---|
seleciton.js
getSelectedHtml
- not checking, because that's a temporary method.
tools.js
refreshCursor
- Better name is needed - e.g.
fixGeckoDisappearingCursorBug
- Code sample should use
if ()
. The same in real code. - Method description is poor - it has no value for developer. Links to tickets and problematic cases should be included.
- In docs it's "Firefox" not "FF".
- #5560 is mentioned in a comment, but it's tottally unrelated. It should be #5660.
- Are you sure that we need the same fix for drag and drop? These seem to be unrelated cases. The issue with cursor should be mentioned in comment or in ticket (and linked from comment), so we could verify that. Documentation for why code does something is many times more important than documentation for what this code does.
- BTW. It would be very good to have this repored on bugzilla, but I remember that it was a very fragile case.
- Better name is needed - e.g.
clipboard plugin
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L1440 - mixed indentation.
- It would be good to separate code for copy&paste and dnd. E.g.
addListenersToEditable
could be split. CKEDITOR.plugins.clipboard.dnd
- don't usednd
in public APIs, this is a internal abbreviation. I would name itdragData
.CKEDITOR.plugins.clipboard.dnd = undefined
- set tonull
.- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L549-L563 - this logic should be extracted to
plugins.clipboard.catchDragEvent( evt )
- similarly this https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L538 - https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L579 - what happens inside this condition should be extracted to separate functions -
handleInternalDrop
, etc. Note that if such complex function will become public (e.g. underplugins.clipboard
) you'll be able to test them separately. - https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L619 - use
CKEDITOR.NODE_*
. - https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L619-L621 - do not align code with spaces.
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L631-L632 - these properties are read only. And what about endOffset and container?
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L618-L633 - if you extract this to a public method you could test it. The same will the rest of the code... it's absurdly complex and have no unit tests.
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L1508-L1509 - do not split line in the middle of object property access.
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L1533 - you can simplify this function if you first check whether startContainer is a text node.
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L1557 - tab after
var
. - https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L1569 - it should be public because it contains public class
dataTransfer.
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L1579-L1586 - we keep params' descriptions in the same lines and do not add empty lines.
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L1603 - wrong quotes.
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L1635 - ???
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L1651 - instead of accessing prototype dozen of times override it. Code size matters.
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L1720-L1769 - move all this to the constructor or right after it. You don't need to specify
@member
in such case. - https://github.com/cksource/ckeditor-dev/blob/3601cdd/plugins/clipboard/plugin.js#L585 - I'm rather against handling multi-range selections. In many cases we care only about first range and in this case we should too. This will simplify this already very complex code.
tests
- https://github.com/cksource/ckeditor-dev/blob/3601cdd/tests/plugins/clipboard/drop.js#L9 - mixed indentation.
- As for tests I've got a general remark that we cannot have such tests. That will be a root of problems and they will be unmaintainable. Instead of trying to mock entire drop event you should mock method that gets range from drop event - it should return a range that you want it to return. This code is simply too complex on both ends - inside clipboard and in tests. It's a mistake to write functional tests for UI, especially that we have to test this manually anyway.
comment:29 Changed 10 years ago by
Status: | review_failed → assigned |
---|
comment:30 Changed 10 years ago by
I checked how D&D works on Safari@Mac and I met some issue:
- on the external drop it is not possible to get html, this is because of this bug: https://bugs.webkit.org/show_bug.cgi?id=19893
- there is no
drop
event if I do not set any text data ondragstart
(even if I set some custom data) https://github.com/cksource/ckeditor-dev/blob/t/11460/plugins/clipboard/plugin.js#L542 - there are different mouse positions on the screen on Mac and on PC so tests need to be rewritten and they should not be based on these positions.
comment:31 Changed 10 years ago by
Status: | assigned → review |
---|
Changes:
- applied changes suggested during last review,
- fixed 2 more issue,
- improved tests and created some more,
- reorganized code,
- added some comments and documentation,
- checked tests with build version.
Code is ready to the final review.
comment:33 Changed 10 years ago by
First part of review (mostly code style and naming):
- reset/initDataTransfer are related to drag and drop exclusively. Name of these methods have to mark that - i.e. resetDragDataTransfer. Alternatively we can move them to sub namespace, but I think that it will be better to make decision regarding that later, when we'll see how many pate/dnd methods we have in clipboard namespace. For now add only the "drag" word.
- Existence of addDropListenersToEditable function is confusing, because it's used once, 2 lines above.
- Cache
CKEDITOR.plugins.clipboard
under some shorter name (simplyclipboard
will be ok), because it's used plenty of times. Additionally, defineCKEDITOR.plugins.clipboard
with all its methods, not as empty object. fixIESplittedNodes
->fixIESplitNodesAfterDrop
.- Some doc strs have empty line at the end.
- We use tags in doc strs after text, not before (incorrect @private position).
clipboard.dragData
anddragRange
are not documented.fixIESplittedNodes
is used once so don't cache its name.- You execute
rangeBefore( dragRange, dropRange )
twice - execute it once, cache result and the logic will be less confusing. BTW. you can remove empty lines between these twoif()
anddropBookmark
initialisation to show that this is all linked. - Please mark following places with
@todo <explanation>
markers:firePasteWithDataTransfer
definition,deleteContents()
andgetSelectedHtml()
calls, so we won't miss them. - Maybe
rangeBefore
->isRangeBefore
? - Params documentation for private methods is missing.
comment:34 Changed 10 years ago by
Status: | review → review_failed |
---|
Tests review:
- datatransfer.js
- Call
resetDataTransfer
fromtearDown
orsetUp
. bot.setHtmlWithSelection( '[<b>foo</b>]' );
- this isn't a real selection. Selection will be anchored inside inline tag.- I miss some assertion messages or better test cases names (rather them) which will clarify what is being tested.
- Call
- drop.js
- I made fixes by myself. Many HTML comparison could be simplified with assert.isInnerHtmlMatching and undo tests needed some strong corrections. It was possible (and important to fix) to avoid using timeouts for standard drop cases - waiting for events is much better.
comment:35 Changed 10 years ago by
Status: | review_failed → review |
---|
I improved tests, docs and names. Code is ready for the next review.
comment:36 Changed 10 years ago by
I pushed two commits fixing some minor things.
I found one problem that must be fixed - when running tests from dashboard, one dnd test is red on IE9.
And one problem that may be fixed later - on Safari nothing is dropped when dnding from editor to textarea.
comment:37 Changed 10 years ago by
Status: | review → review_passed |
---|
Problems fixed. R:). I leave you the merge. Congrats!
comment:38 Changed 10 years ago by
1622 lines added, 44 deleted, 67 commits, 38 comments, 42 tests... and done. :)
comment:39 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
During today’s discussion with PJ we raised a number of topics which need further discussion or actions or just should be emphasised.