Opened 10 years ago

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

Status: newconfirmed

comment:2 Changed 10 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 10 years ago by Olek Nowodziński

Also WFM.

comment:4 Changed 10 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 10 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

comment:6 Changed 10 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 10 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 10 years ago by Garry Yao (previous) (diff)

comment:8 Changed 10 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 10 years ago by Piotrek Koszuliński (previous) (diff)

comment:9 Changed 10 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 10 years ago by Piotrek Koszuliński (previous) (diff)

comment:10 Changed 10 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 10 years ago by Garry Yao

Component: GeneralCore : Selection
Status: reviewreview_passed

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

Resolution: fixed
Status: review_passedclosed
Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy