Opened 6 years ago

Closed 5 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 Piotr Jasiun)

Part of #11437.

Spec (status: proposal)

  1. 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.

  1. 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.

  1. 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.

  1. 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'.

  1. 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.

  1. 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.

  1. Uploading files has to be handled by #paste (because files can be pasted and dropped and #paste is the single entry point). Ticket: #11461.

  1. Widgets dnd will have to be aligned to changes made in this ticket. Ticket: #11219.

Attachments (12)

keep-files.html (1.6 KB) - added by Piotr Jasiun 6 years ago.
keepDragSelectionWithBookmarks.html (6.3 KB) - added by Piotr Jasiun 6 years ago.
Keep drag selection with bookmark.
keepDragSelectionWithBookmarks2.html (6.3 KB) - added by Piotr Jasiun 6 years ago.
Keep drag selection with bookmark2.
keepDragSelectionWithRanges.html (6.3 KB) - added by Piotr Jasiun 6 years ago.
Keep drag selection with ranges.
readHtmlOnDragstart.html (6.1 KB) - added by Piotr Jasiun 6 years ago.
Read HTML on 'dragstart' and write it to the console.
getDropPositionUsingGetSelection.html (5.8 KB) - added by Piotr Jasiun 6 years ago.
Get drop position using native selection.
getDropPositionUsingWidgetFunction.html (7.0 KB) - added by Piotr Jasiun 6 years ago.
Get drop position using function borrowed from widgets.
getDropPositionUsingCustomSrcipt.html (8.1 KB) - added by Piotr Jasiun 6 years ago.
Get drop position using custom script based on elementFromPoint and getClientRects
getDropPositionUsingGetSelection-IE8-bugs.webm (2.3 MB) - added by Piotr Jasiun 6 years ago.
getDropPositionUsingWidgetFunction-IE8-UnspecifiedError.webm (1.1 MB) - added by Piotr Jasiun 6 years ago.
customRangeFromPointBugs.webm (1.9 MB) - added by Piotrek Koszuliński 6 years ago.
getDropPositionUsingMixedSolution.html (17.9 KB) - added by Piotr Jasiun 6 years ago.
Final candidate for getRangeAtDropPosition

Change History (51)

comment:1 Changed 6 years ago by Piotrek Koszuliński

Description: modified (diff)
Status: newconfirmed

comment:2 Changed 6 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:3 Changed 6 years ago by Piotrek Koszuliński

During today’s discussion with PJ we raised a number of topics which need further discussion or actions or just should be emphasised.

  1. 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.
  1. 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.

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.

  1. 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.
  1. 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.
  1. 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.

comment:4 in reply to:  3 Changed 6 years ago by Piotr Jasiun

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.

  1. 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.

  1. 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.

  1. 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.

  1. 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.

  1. 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 to img 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.

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

comment:5 Changed 6 years ago by Piotr Jasiun

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 6 years ago by Piotrek Koszuliński

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

  1. 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.
  2. 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.
  3. Current behaviour of editor#paste event must be preserved in as many details as possible.
  4. There should be a single entry point.
  5. If it's possible editor#paste should not be fired by editor#drop, what means that there would have to be a new event (editor#inputData).
  6. IEs do not allow to get HTML from drop and paste 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.
  7. 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.
  8. The additional data items that editor#paste and editor#drop events will carry should be facades to native DataTransfer 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.
  9. 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 (if getData('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.
  10. 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:
    1. 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.
    2. 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.
  11. 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.
  12. Editor should add class to editable on dragover, so developers can add special styles to mark such editable.

Design

Data flow

  1. Step one - data capturing
    • The goal is to pass files list with data transfer facedes, HTML data string, and type to editor#paste and editor#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) and files list are passed to editor#drop or editor#paste.
  2. Step two - data processing
    • Includes: type discovery (text, html), plain text htmlification/normalization, html textification (forcePasteAsPlainText), MSWord documents normalization, etc.
    • We need this step during editor#paste and editor#drop, so we'll not need to change current editor#paste behaviour.
    • Methods used currently by editor#paste listeners have to be made public unless this part of editor#drop is implemented in the clipboard plugin.
  3. Step three - files processing
    • Can be done in editor#inputData, because it's a single entry point. Although, since the rest of processing happen on editor#drop and editor#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 replace src attribute with data URI retrieved (asynchronously!) from file, based on data-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.
  4. Step four - single entry point
    • editor#inputData is fired synchronously after execution of editor#drop and editor#paste with all their data (files lists, HTML data, type and custom properties).
    • It's fired in its 1000-prior listener, so editor#paste or editor#drop can be fired by other code and the chain will be finished.
  5. Step five - data insertion
    • If HTML data string is not empty, insertion is performed synchronously after editor#inputData (in its 1000-prior listener, so editor#inputData can be fired by other code and the chain will be finished).
    • editor#afterInputData is fired after insertion. Additionally editor#afterPaste is fired. Currently editor#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 after editor#paste we e.g. fire afterCommandExec.
    • editor#afterPaste and editor#beforePaste should be marked as deprecated. First is replaced by editor#afterInputData, second is only a problem for us and most likely is useless.
  6. 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

  1. We may consider styling editable when class marking editable on dragover is present.
  2. 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).
  3. 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?
Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)

comment:7 Changed 6 years ago by Piotr Jasiun

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;
Last edited 6 years ago by Piotr Jasiun (previous) (diff)

Changed 6 years ago by Piotr Jasiun

Attachment: keep-files.html added

comment:8 Changed 6 years ago by Piotr Jasiun

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 throws Unspecified error when you are not directly on contents (eg. cursor is next to the header); if I call it on mouseover 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 6 years ago by Piotr Jasiun

Summary: The editor#drop event - handling dropping content in editorHandling dropping content in editor

comment:10 Changed 6 years ago by Piotr Jasiun

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?

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

comment:11 Changed 6 years ago by Piotrek Koszuliński

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:

  1. Retrieving HTML on dragstart. The sample could log retrieved HTML on console.
  2. 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 using createBookmarks2(). After restoring ranges from these bookmarks use deleteContents() to see if all this worked.
  3. 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 simple editor.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 6 years ago by Piotr Jasiun

Keep drag selection with bookmark.

Changed 6 years ago by Piotr Jasiun

Keep drag selection with bookmark2.

Changed 6 years ago by Piotr Jasiun

Keep drag selection with ranges.

Changed 6 years ago by Piotr Jasiun

Attachment: readHtmlOnDragstart.html added

Read HTML on 'dragstart' and write it to the console.

Changed 6 years ago by Piotr Jasiun

Get drop position using native selection.

Changed 6 years ago by Piotr Jasiun

Get drop position using function borrowed from widgets.

Changed 6 years ago by Piotr Jasiun

Get drop position using custom script based on elementFromPoint and getClientRects

Changed 6 years ago by Piotr Jasiun

comment:12 Changed 6 years ago by Piotr Jasiun

Description: modified (diff)

I created set of samples for problems mentioned above.

  1. 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.
  2. Remembering dragged selection.

Both bookmark2 and range needs to be changed to the bookmark after drop, because we need to create a bookmark for a drop position which can destroy bookmark2 or range.

  1. Retrieving drop position.

comment:13 Changed 6 years ago by Piotrek Koszuliński

Good job with the samples. They are really useful.

  1. 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.
  2. Remembering dragged selection.
    • I'm ok with keeping reference to range. On drop, if range.select() throws error we should stop entire process, because it meant that something violated DOM. Better to do nothing then.
  3. 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.
Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)

Changed 6 years ago by Piotrek Koszuliński

comment:14 in reply to:  13 Changed 6 years ago by Piotr Jasiun

Replying to Reinmar:

Good job with the samples. They are really useful.

  1. 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.

  1. Remembering dragged selection.
    • I'm ok with keeping reference to range. On drop, if range.select() throws error we should stop entire process, because it meant that something violated DOM. Better to do nothing then.
  2. 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 6 years ago by Piotr Jasiun

Description: modified (diff)

comment:16 Changed 6 years ago by Piotrek Koszuliński

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

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

Changed 6 years ago by Piotr Jasiun

Final candidate for getRangeAtDropPosition

comment:17 in reply to:  16 Changed 6 years ago by Piotr Jasiun

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 6 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4CKEditor 4.5

comment:19 Changed 6 years ago by Piotr Jasiun

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 in reply to:  19 Changed 6 years ago by Piotrek Koszuliński

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 6 years ago by Piotr Jasiun

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 6 years ago by Piotr Jasiun

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 6 years ago by Piotr Jasiun

Status: assignedreview

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 6 years ago by Piotr Jasiun

Status: reviewassigned

comment:25 Changed 6 years ago by Piotr Jasiun

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).
Last edited 5 years ago by Piotr Jasiun (previous) (diff)

comment:26 Changed 5 years ago by Piotr Jasiun

Status: assignedreview

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:27 Changed 5 years ago by Piotrek Koszuliński

Rebased branch:t/11460 on latest major.

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

Status: reviewreview_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.

clipboard plugin

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 5 years ago by Piotr Jasiun

Status: review_failedassigned

comment:30 Changed 5 years ago by Piotr Jasiun

I checked how D&D works on Safari@Mac and I met some issue:

comment:31 Changed 5 years ago by Piotr Jasiun

Status: assignedreview

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.

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

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

Force pushed branch:t/11460 rebased on major.

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

First part of review (mostly code style and naming):

  1. 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.
  2. Existence of addDropListenersToEditable function is confusing, because it's used once, 2 lines above.
  3. Cache CKEDITOR.plugins.clipboard under some shorter name (simply clipboard will be ok), because it's used plenty of times. Additionally, define CKEDITOR.plugins.clipboard with all its methods, not as empty object.
  4. fixIESplittedNodes -> fixIESplitNodesAfterDrop.
  5. Some doc strs have empty line at the end.
  6. We use tags in doc strs after text, not before (incorrect @private position).
  7. clipboard.dragData and dragRange are not documented.
  8. fixIESplittedNodes is used once so don't cache its name.
  9. 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 two if() and dropBookmark initialisation to show that this is all linked.
  10. Please mark following places with @todo <explanation> markers: firePasteWithDataTransfer definition, deleteContents() and getSelectedHtml() calls, so we won't miss them.
  11. Maybe rangeBefore -> isRangeBefore?
  12. Params documentation for private methods is missing.

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

Status: reviewreview_failed

Tests review:

  1. datatransfer.js
    • Call resetDataTransfer from tearDown or setUp.
    • 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.
  2. 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 5 years ago by Piotr Jasiun

Status: review_failedreview

I improved tests, docs and names. Code is ready for the next review.

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

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 5 years ago by Piotrek Koszuliński

Status: reviewreview_passed

Problems fixed. R:). I leave you the merge. Congrats!

comment:38 Changed 5 years ago by Piotr Jasiun

git:815db94

1622 lines added, 44 deleted, 67 commits, 38 comments, 42 tests... and done. :)

comment:39 Changed 5 years ago by Piotr Jasiun

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