Opened 4 years ago

Closed 3 years ago

#13129 closed Bug (fixed)

Block widget is not focus after drag and drop and undo

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

Description

  1. Open http://tests.ckeditor.dev:1030/tests/plugins/widget/manual/block
  2. Select any widget.
  3. Move it using drag and drop.
  4. Undo.

Result: widget is not selected (it should be) and selection is lost.

Bug introduced by #13042.

Note that in #13042 this assertion were removed because they failed:

They should be restored after the fix or appropriate assertion should be added instead.

Change History (9)

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

Milestone: CKEditor 4.5.0
Status: newconfirmed

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

Milestone: CKEditor 4.5.0CKEditor 4.5.1

comment:3 Changed 3 years ago by Szymon Cofalik

Owner: set to Szymon Cofalik
Status: confirmedassigned

comment:4 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

Fixed on branch:t/13129.

I deleted a line of code which should be cleaned in #13042. It is not needed in current implementation of D&D and even introduces bugs. After deleting it, widget is selected after undoing. Tested on all browsers manually.

I also reverted
https://github.com/ckeditor/ckeditor-dev/blob/7342d1806e7534ed14a4ec39d8d5d7b08fd61203/tests/plugins/widget/dnd.js#L388

However
https://github.com/ckeditor/ckeditor-dev/blob/7342d1806e7534ed14a4ec39d8d5d7b08fd61203/tests/plugins/widget/dnd.js#L380
does not really make any sense to me - why we would like to check it there? I moved it before firing the event. This line was there from the beginning of the test.

I also added an extra assert to test if widget is still selected after undo.

comment:5 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_failed
  • Messages in 1 and 2 contradict the actual assertions.
  • Assertion
    assert.isTrue( !!editor.getSelection().isFake, ... )
    
    is weak because it will pass no matter which element is selected. What if something else gets selected? Use
    assert.areSame( widget, editor.widgets.focused, 'widget is focused' );
    
    instead. You may need getWidgetById again because undo will demolish editor.widgets and the reference will point to a non–existing widget.

comment:6 Changed 3 years ago by Szymon Cofalik

Status: review_failedassigned

comment:7 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

Pushed fixes to branch:t/13129

comment:8 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_passed

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

Resolution: fixed
Status: review_passedclosed

Merged into master in git:507dd19.

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