#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)
Change History (19)
Changed 9 years ago by
Attachment: | nestedWidgetsUndoRedo.gif added |
---|
Changed 9 years ago by
Attachment: | nesteWidgetsUndoRedobootstrapwidgets.gif added |
---|
comment:1 Changed 9 years ago by
Keywords: | Blink Webkit added; undo/redo blur exception removed |
---|---|
Status: | new → confirmed |
Changed 9 years ago by
Attachment: | 2015-05-29_1548.swf added |
---|
Changed 9 years ago by
Attachment: | 2015-05-29_1549.swf added |
---|
comment:2 Changed 9 years ago by
Owner: | set to Szymon Cofalik |
---|---|
Status: | confirmed → assigned |
comment:3 Changed 9 years ago by
Status: | assigned → review |
---|
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' } }
comment:4 Changed 9 years ago by
What if i use http://docs.ckeditor.com/#!/api/CKEDITOR.plugins.widget-method-initEditable somewhen after the original widget initialisation?
comment:5 Changed 9 years ago by
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.
comment:7 Changed 9 years ago by
Fix and tests look nice, therfore I got some comments.
- 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.
- 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]
- Function [findCorrentEditable] should have at least one test event if its internal one. See perhaps this.
- 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.
comment:8 Changed 9 years ago by
I've pushed branch:t/13334
- Added a test.
- 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.
- I've put findCorrectEditable into Widget.prototype and added a test.
- Splited manual test for nested widgets between already existing manual tests for widgets.
comment:9 Changed 9 years ago by
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.
comment:10 follow-up: 11 Changed 9 years ago by
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
.
comment:11 Changed 9 years ago by
Status: | review → review_failed |
---|
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.
comment:12 Changed 9 years ago by
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
comment:13 Changed 9 years ago by
Status: | review_failed → review |
---|
comment:14 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
I updated function name in comments. Fixed with git:a3d9e76.
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:
Result: error will be thrown.
TC2
To reproduce:
Result: error will be thrown.
Error is:
Message: Cannot read property 'blur' of null
line: 238
URI: ckeditor-dev/core/focusmanager.js