Opened 7 years ago

Closed 7 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

  1. Load any divarea sample
  2. Insert a default template
  • Actual: JS error thrown.

Change History (12)

comment:1 Changed 7 years ago by Garry Yao

Status: newconfirmed

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

Status: confirmedpending

WFM. I couldn't reproduce this issue on http://localhost/cksource/ckeditor-dev/samples/divarea.html

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

Also WFM.

comment:4 Changed 7 years ago by Jakub Ś

Status: pendingconfirmed

Reproducible in IE7-9.

  1. Load divarea sample
  2. Clear editor contents. Click new page button or CRTL+A backspace or switch to source delete html and switch to wysiwyg.
  3. Insert a default template

You will get invalid argument error.

comment:5 Changed 7 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

comment:6 Changed 7 years ago by Piotrek Koszuliński

Status: assignedreview

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 7 years ago by Garry Yao

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.

Last edited 7 years ago by Garry Yao (previous) (diff)

comment:8 Changed 7 years ago by Piotrek Koszuliński

Status: reviewreview_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.

Last edited 7 years ago by Piotrek Koszuliński (previous) (diff)

comment:9 Changed 7 years ago by Piotrek Koszuliński

Owner: changed from Garry Yao to Piotrek Koszuliński
Status: review_failedreview

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.

Last edited 7 years ago by Piotrek Koszuliński (previous) (diff)

comment:10 Changed 7 years ago by Piotrek Koszuliński

PS. I want to write some tests for this, but let me know what do you think first.

comment:11 Changed 7 years ago by Garry Yao

Component: GeneralCore : Selection
Status: reviewreview_passed

comment:12 Changed 7 years ago by Piotrek Koszuliński

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