Opened 13 years ago
Closed 13 years ago
#9239 closed Bug (fixed)
template plugin error prune on divarea
| Reported by: | Garry Yao | Owned by: | Piotrek Koszuliński |
|---|---|---|---|
| Priority: | Normal | Milestone: | CKEditor 4.0 |
| Component: | Core : Selection | Version: | 4.0 |
| Keywords: | Cc: |
Description
- Load any divarea sample
- Insert a default template
- Actual: JS error thrown.
Change History (12)
comment:1 Changed 13 years ago by
| Status: | new → confirmed |
|---|
comment:2 Changed 13 years ago by
| Status: | confirmed → pending |
|---|
comment:4 Changed 13 years ago by
| Status: | pending → confirmed |
|---|
Reproducible in IE7-9.
- Load divarea sample
- Clear editor contents. Click new page button or CRTL+A backspace or switch to source delete html and switch to wysiwyg.
- Insert a default template
You will get invalid argument error.
comment:5 Changed 13 years ago by
| Owner: | set to Piotrek Koszuliński |
|---|---|
| Status: | confirmed → assigned |
comment:6 Changed 13 years ago by
| Status: | assigned → review |
|---|
Pushed t/9239.
Fix for #4073 (or some other issue, as I'm not able to find when that patch was really proposed) doesn't seem to be needed still.
comment:7 Changed 13 years ago by
| Owner: | changed from Piotrek Koszuliński to Garry Yao |
|---|
The patch didn't touch the root of the issue, I've pushed t/9239b for a deeper fix, the culprit is that in v4 we unlock selection automatically when focus is returned, while the template dialog has just invalidated the lock selection, in such case a simple silent failure is enough.
The rational behind calling all insertion before closing the dialog, is to ensure that user who listened to the dialog#hide really got the result from all jobs done by the dialog.
comment:8 Changed 13 years ago by
| Status: | review → review_failed |
|---|
I've got mixed feelings about this patch. It of course fixes the issue, but there always pops out security alert in my head when I see code silently accepting incorrect input. Thus, I think that we should still focus on fixing timing on template dialog close. Only if other way can't be find, this patch makes sense.
comment:9 Changed 13 years ago by
| Owner: | changed from Garry Yao to Piotrek Koszuliński |
|---|---|
| Status: | review_failed → review |
I pushed t/9239c.
I still believe that the order of action (dialog.hide after editor.setData) is broken and I would change it anyhow, but I'm proposing ensuring that selection is valid on selection.unlock. It's Garry's fix, but in different place - in which it IMO makes more sense.
comment:10 Changed 13 years ago by
PS. I want to write some tests for this, but let me know what do you think first.
comment:11 Changed 13 years ago by
| Component: | General → Core : Selection |
|---|---|
| Status: | review → review_passed |
comment:12 Changed 13 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | review_passed → closed |
Masterised https://github.com/ckeditor/ckeditor-dev/commit/a9da7a5 + simple tests.
WFM. I couldn't reproduce this issue on http://localhost/cksource/ckeditor-dev/samples/divarea.html