Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#13465 closed Bug (fixed)

Error is thrown and widget is lost when DnD if it's the only content of editor

Reported by: Piotrek Koszuliński Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.5.3
Component: General Version: 4.5.0
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

  1. Open the placeholder sample.
  2. Clear all contents.
  3. Insert one placeholder.
  4. Try to DnD it.

An error is thrown and the placeholder disappears.

Reproduced on Chrome and FF.

Change History (34)

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

Description: modified (diff)
Status: newconfirmed

comment:2 Changed 9 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:3 Changed 9 years ago by Artur Delura

It might be a duplicate of #13453.

comment:4 Changed 9 years ago by Szymon Cofalik

Owner: changed from Artur Delura to Szymon Cofalik

comment:5 Changed 9 years ago by Artur Delura

Owner: changed from Szymon Cofalik to Artur Delura

This is mine :D

comment:6 Changed 9 years ago by Artur Delura

So the problem is directly in extractHtmlFromRange method. If we got following content in the editable and selection:

<p>[<span>foo</span>]<span data-cke-bookmark="1">bar</span></p>

So range is anchored in <p> element with offsets 0 and 1. If we call editable.extractHtmlFromRange( range, 1), we end up with removed bookmark span.

The funny fact is that element is removed only when it has data-cke-bookmark attribute. If we change it to something different everything works well.

Update 1 As far as I can see extract method creates a bookmark which adjoin our bookmark span, so at the end it's removed accidentally.

Update 2 here we whole paragraph is removed.

Last edited 9 years ago by Artur Delura (previous) (diff)

comment:7 Changed 9 years ago by Artur Delura

So problem is in check[Start|End]OfBlock in range. It checks whether start or end block is empty. Method checks also bookmark containers and treat it as an empty ones. In use case listed above, there is only one bookmark within <p> element.

The problem is that this bookmark was created previously and shouldnt be taken into consideration.

Process look as follow:

  1. D&D occurs so two bookmark are being created - drag and drop.
  2. Then extractHtmlFromRange is called and within this function another bookmark is created.
  3. While extracting - bookmarks are also removed.

But should be removed only this ones which was created in extractHtmlFromRange method. In our use case drop bookmark is removed as well.

check[Start|End]OfBlock method is not that clever to distinguish bookmarks which should be removed from that ones which shouldn't be.

Last edited 9 years ago by Artur Delura (previous) (diff)

comment:8 Changed 9 years ago by Artur Delura

I commented out bookmark checking and run all tests and everything pass. So the question is for what purpose we remove bookmarks?

comment:9 Changed 9 years ago by Artur Delura

Status: assignedreview

Changes in branch:t/13465. Solution is simple, extractHtmlFromRange doesn't remove bookmarks. We got yet another one possible solutions here, but I'm not sure. We could call method extractHtmlFromRange without second parameter which is responsible for removing empty block.

comment:10 Changed 9 years ago by Piotrek Koszuliński

Status: reviewreview_failed

I pushed one additional test (manual one) to branch:t/13465.

The idea what behaviour must be changed was good. Unfortunately R-, because it's extractHtmlFromRanger what should be fixed, not checkEnd/StartOfBlock. A change like the one proposed could blow up half of the editor, because you don't know where that method is used.

comment:11 Changed 9 years ago by Tade0

Owner: changed from Artur Delura to Tade0
Status: review_failedassigned

comment:12 Changed 9 years ago by Tade0

I cherry picked to a new branch the commits that create the tests and will be adding my work on top of that.

comment:13 Changed 9 years ago by Tade0

I tested dragging the widget before itself and by the time https://github.com/cksource/ckeditor-dev/blob/t/13465/core/editable.js#L747 is reached the bookmark is already gone.

Given this changing the code so that the empty block isn't removed solves only the problem with dragging the widget after itself.

Searching now for a solution that handles dragging both before and after.

comment:14 Changed 9 years ago by Tade0

I found an interesting quirk. This is how the DOM may look at https://github.com/cksource/ckeditor-dev/blob/t/13465/core/editable.js#L723 when dragging the widget before itself:

<p>

<span data-cke-bookmark="1" style="display: none;">&nbsp;</span>

<span data-cke-bookmark="1" style="display: none;">&nbsp;</span>

<span tabindex="-1" contenteditable="false" data-cke-widget-wrapper="1" data-cke-filter="off" class="cke_widget_wrapper cke_widget_inline cke_widget_focused cke_widget_selected" data-cke-display-name="placeholder" data-cke-widget-id="0">

<span data-cke-bookmark="1" id="cke_bm_29C" style="display: none;">&nbsp;</span>

<span class="cke_placeholder cke_widget_element" data-cke-widget-keep-attr="0" data-widget="placeholder" data-cke-widget-data="%7B%22name%22%3A%22foobar%22%2C%22classes%22%3Anull%7D">foobar?</span>

<span class="cke_reset cke_widget_drag_handler_container" style="top: -12px; left: 0px; display: block; background: url(http://tests.ckeditor.dev:1030/apps/ckeditor/plugins/widget/images/handle.png) rgba(220, 220, 220, 0.498039);">

<img class="cke_reset cke_widget_drag_handler" data-cke-widget-drag-handler="1" src="data:image/gif;base64,R0lGODlhAQABAPABAP///wAAACH5BAEKAAAALAAAAAABAAEAAAICRAEAOw==" width="15" title="Click and drag to move" height="15" draggable="true">

</span>

</span>

<span data-cke-bookmark="1" style="display: none;">&nbsp;</span>

</p>

Note the bookmark inside the widget wrapper.

comment:15 Changed 9 years ago by Tade0

In conclusion this seems to be two separate problems:

  1. Sometimes the drop bookmark lands in a block that is considered "empty" and both those elements are removed.
  2. In other situations the bookmark lands inside the widget (like in comment:14).

It's possible to solve 1. by preserving the children of the removed block.

  1. requires further investigation since normally it should be impossible to drop anything inside an element, that has contenteditable="false".

comment:16 Changed 9 years ago by Tade0

This line: https://github.com/cksource/ckeditor-dev/blob/t/13465/plugins/clipboard/plugin.js#L1740 causes problem 2.

It's a native method, so we have little control over where it places the drop range.

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

So I think we have to simply abort if something is wrong. As soon as possible we should verify the ranges - that e.g. drop range is outside of drag range.

comment:18 Changed 9 years ago by Piotrek Koszuliński

PS. Turns out that #13453 is about dropRange being found inside dragRange. Therefore, let's focus here on the first problem - extractHtmlFromRange removing blocks with bookmarks.

comment:19 Changed 9 years ago by Tade0

Status: assignedreview

Solved problem 1. by checking if the block that is to be removed contains bookmarks (or, actually, <span> elements).

Changes pushed to branch:t/13465a.

comment:20 Changed 9 years ago by Piotrek Koszuliński

Status: reviewreview_failed

I rebased branch:t/13465a and pushed two more commits there.

There are three problems:

  1. Some tests fail in http://tests.ckeditor.dev:1030/tests/core/editable/getextracthtmlfromrange - fixed in the second commit.
  2. You can use path.block.getElementsByTag() - fixed in the second commit.
  3. The bookmarks check must be more precise (must look for data-cke-bookmark) - added test which shows this.

Some hints:

  • element.data( 'cke-bookmar' )
  • CKEDITOR.dom.walker.bookmark
  • use walker or iterate over the returned spans. The latter will be simpler.

comment:21 Changed 9 years ago by Tade0

Status: review_failedassigned

comment:22 Changed 9 years ago by Tade0

I went with iterating over the spans eventually.

Tests are green on Chrome, Firefox, IE11, Edge and IE8.

Squashed my commit with yours, changes pushed to branch:t/13465b so as to not remove your changes completely from the history.

comment:23 Changed 9 years ago by Tade0

Status: assignedreview

comment:24 Changed 9 years ago by Piotrek Koszuliński

Status: reviewreview_failed
Version: 4.5.0

Looks quite ok. The last thing will be extracting that loop into a separate private function so it's called only when all other conditions are met and does not bloat its current surrounding.

comment:25 Changed 9 years ago by Tade0

Status: review_failedreview

Changes pushed to branch:t/13465b.

comment:26 Changed 9 years ago by Olek Nowodziński

Status: reviewreview_failed

I rebased branch:t/13465b on latest master and pushed a commit to save a few bytes in core.

Still, an error is thrown once dropped the widget before itself in IE11 (http://tests.ckeditor.dev:1030/tests/tickets/13465/1).

PS. Same on Edge.

Last edited 9 years ago by Olek Nowodziński (previous) (diff)

comment:27 Changed 9 years ago by Tade0

Have you checked whether it's because of the problem described in comment:14 and #13453?

comment:28 in reply to:  27 Changed 9 years ago by Olek Nowodziński

Replying to Tade0:

Have you checked whether it's because of the problem described in comment:14 and #13453?

Nope :)

comment:29 Changed 9 years ago by Piotrek Koszuliński

Nope, it isn't caused by #13453 or nope, you haven't checked?

comment:30 in reply to:  29 ; Changed 9 years ago by Olek Nowodziński

Replying to Reinmar:

Nope, it isn't caused by #13453 or nope, you haven't checked?

Haven't checked.

comment:31 Changed 9 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.2CKEditor 4.5.3

comment:32 in reply to:  30 Changed 9 years ago by Tade0

Status: review_failedreview

Replying to a.nowodzinski:

Haven't checked.

Could you verify that the error message is caused by the problem from #13453?

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

Resolution: fixed
Status: reviewclosed

Fixed on master with git:b32d85d. The only issue I could find now is the ability to drop content outside of body in IE11 (or something similar). I'll report a separate ticket for this.

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

Reported the followup - #13640.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy