Nested widget undo/redo bug

[Radoslav Petkov]

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

[Jakub Ś]

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

[Szymon Cofalik]

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' }
}

[Radoslav Petkov]

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

[Szymon Cofalik]

The method should find proper elements. Nested widgets are initialized during ​http://docs.ckeditor.com/#!/api/CKEDITOR.plugins.widget-method-initEditable . 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.

[Szymon Cofalik]

Probably fixes also #13218

[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]

3. Function [findCorrentEditable] should have at least one test event if its internal one. See perhaps ​this.
4. 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.

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

3. I've put findCorrectEditable into Widget.prototype and added a test.
4. Splited manual test for nested widgets between already existing manual tests for widgets.

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

[Piotr Jasiun]

Some code style issues:

• ​https://github.com/cksource/ckeditor-dev/blob/t/13334/plugins/widget/plugin.js#L1262 private function should start with underscore,
• ​https://github.com/cksource/ckeditor-dev/blob/t/13334/plugins/widget/plugin.js#L1272 we do not use comments like this. The comment should be above the line.

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

[Artur Delura]

Replying to pjasiun:

• ​https://github.com/cksource/ckeditor-dev/blob/t/13334/plugins/widget/plugin.js#L1272 we do not use comments like this. The comment should be above the line.

Fixed this already.

[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

[Artur Delura]

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

[Piotrek Koszuliński]

This fix was included in 4.5.0.