Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#13334 closed Bug (fixed)

Nested widget undo/redo bug

Reported by: Radoslav Petkov Owned by: Szymon Cofalik
Priority: Normal Milestone: CKEditor 4.5.0
Component: UI : Widgets Version: 4.5.0 Beta
Keywords: Blink Webkit Cc:

Description

I have found a bug in the nesting widget functionality after using undo/redo feature.

The same bug can be reproduced using this plugin http://ckeditor.com/addon/widgetbootstrap

Attachments (4)

nestedWidgetsUndoRedo.gif (454.2 KB) - added by Radoslav Petkov 4 years ago.
nesteWidgetsUndoRedobootstrapwidgets.gif (656.8 KB) - added by Radoslav Petkov 4 years ago.
2015-05-29_1548.swf (812.6 KB) - added by Jakub Ś 4 years ago.
2015-05-29_1549.swf (798.2 KB) - added by Jakub Ś 4 years ago.

Change History (19)

Changed 4 years ago by Radoslav Petkov

Attachment: nestedWidgetsUndoRedo.gif added

Changed 4 years ago by Radoslav Petkov

comment:1 Changed 4 years ago by Jakub Ś

Keywords: Blink Webkit added; undo/redo blur exception removed
Status: newconfirmed

I have been able to reproduce this issue with bootstrap plugin and attached widget2 plugin. I have been able to reproduce this issue only in Blink and Webkit.

TC1
To reproduce:

  1. Insert widget, then widget inside a widget and then widget inside nested widget. You should have hierarchy of 3 widgets.
  2. Start pressing undo.

Result: error will be thrown.

TC2
To reproduce:

  1. Insert widget, then widget inside a widget
  2. Switch to Source, wysiwyg, Source

Result: error will be thrown.

Error is:
Message: Cannot read property 'blur' of null
line: 238
URI: ckeditor-dev/core/focusmanager.js

Changed 4 years ago by Jakub Ś

Attachment: 2015-05-29_1548.swf added

Changed 4 years ago by Jakub Ś

Attachment: 2015-05-29_1549.swf added

comment:2 Changed 4 years ago by Szymon Cofalik

Owner: set to Szymon Cofalik
Status: confirmedassigned

comment:3 Changed 4 years ago by Szymon Cofalik

Status: assignedreview

Fixed on branch:t/13334

The origin of those bugs is flawed widgets initialization. Before fixing, when looking for editables, first matching element was chosen. Sometimes it was an editable element from other widget. In this case, two widgets had same element in Widget.editables. This caused multiple errors when one of those widgets got removed because the other one had wrong references.

Now we look only for elements that are not inside other widgets.

Unfortunately the error will still occur if editables are defined in wrong order. Editables have to be defined in same order as they are in DOM.

Good:

template: '<div class="testwidget"><div class="col1"></div>'+
	'<div class="col2"></div></div>',
editables: {
	col1: { selector: '.col1' },
	col2: { selector: '.col2' }
}

Bad:

template: '<div class="testwidget"><div class="col1"></div>'+
	'<div class="col2"></div></div>',
editables: {
	col2: { selector: '.col2' },
	col1: { selector: '.col1' }
}
Last edited 4 years ago by Szymon Cofalik (previous) (diff)

comment:4 Changed 4 years ago by Radoslav Petkov

What if i use http://docs.ckeditor.com/#!/api/CKEDITOR.plugins.widget-method-initEditable somewhen after the original widget initialisation?

comment:5 Changed 4 years ago by Szymon Cofalik

Since nested widgets are initialized during http://docs.ckeditor.com/#!/api/CKEDITOR.plugins.widget-method-initEditable the method should still find correct editables. As long as editables definitions and elements in DOM are in same order and you initialize only editables that are defined, everything should be fine.

To test this behavior I used widgetbootstrap plugin and edited two column widget adding extra editable without changing template.

col3: {
	selector: '.col-3',
	allowedContent: allowedWidget
}

Then I created the widget and, through scripts, I added <div class="col-3 col-md-6"></div> to div.row.two-col and then initialized editable widget.initEditable('col3', {selector: '.col-3'}); where widget is the created widget. Undo/redo and source/wysiwyg worked fine, even if the widget was nested in other widget.

If this is not what you asked for, please provide in-depth description.

Last edited 4 years ago by Szymon Cofalik (previous) (diff)

comment:6 Changed 4 years ago by Szymon Cofalik

Probably fixes also #13218

Last edited 4 years ago by Szymon Cofalik (previous) (diff)

comment:7 Changed 4 years ago by Artur Delura

Fix and tests look nice, therfore I got some comments.

  1. What should happend when node itself is passed as a guard in node.getParents? It should return just one element I guess. There should be test for it.
  2. I see that getParents method can return result in a flipped order. I wonder whether it should only switch result array or maybe also guard checking should start from the top?
body -- > div.guard -- > p -- > span

span.getParents( true, guard ); // It should be [div.quard, p, span] or [body, div.quard]
  1. Function [findCorrentEditable] should have at least one test event if its internal one. See perhaps this.
  2. Manual test should have one strict scerario whixh will tell how to reproduce the issue. Expected result should be provided as well. During the development process it's easy to remember and reproduce what was wrong. After a while or when others run manual test, they will never know what was wrong before.
Last edited 4 years ago by Artur Delura (previous) (diff)

comment:8 Changed 4 years ago by Szymon Cofalik

I've pushed branch:t/13334

  1. Added a test.
  2. Current implementation has two advantages:
  • it was easy to implement,
  • if you want to get path from body to div.guard you could do just guard.getParents() so there is no need for the implementation you have proposed.
  1. I've put findCorrectEditable into Widget.prototype and added a test.
  2. Splited manual test for nested widgets between already existing manual tests for widgets.

comment:9 Changed 4 years ago by Artur Delura

I rebased and pushed some changes to branch:t/13334.

According to findCorrectEditable I don't think that method name is correct. First of all "correct" is unnecessary, because we always suppose to have correct result. Also editable doesn't suit me because function features doesn't confine to find editable but to all elements with selector provided. I suggest to change it's name to findOne or findFirst.

Perhas "findOneNotNested" might be okay. Also local variable name won't inappropriate then.

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

comment:10 Changed 4 years ago by Piotr Jasiun

Some code style issues:

editable is always about the element with contentEditable attribute. Since this function can return any element, AFAICS, it should be just an element.

comment:11 in reply to:  10 Changed 4 years ago by Artur Delura

Status: reviewreview_failed

Replying to pjasiun:

Fixed this already.

comment:12 Changed 4 years ago by Szymon Cofalik

Thanks for insightful review.

@a.delura: changed name to findOneNotNested, changed local var name
@pjasiun: added underscore, comment already fixed by @a.delura

Pushed to branch:t/13334

Last edited 4 years ago by Szymon Cofalik (previous) (diff)

comment:13 Changed 4 years ago by Szymon Cofalik

Status: review_failedreview

comment:14 Changed 4 years ago by Artur Delura

Resolution: fixed
Status: reviewclosed

I updated function name in comments. Fixed with git:a3d9e76.

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

Milestone: CKEditor 4.5.0

This fix was included in 4.5.0.

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