Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#10689 closed Bug (fixed)

Save toolbar button saves only first instance

Reported by: gifad Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.2.1
Component: General Version: 4.2
Keywords: Cc:

Description

If multiple ckeditor instances are present in the form, only the first instance is saved (others are posted in their original state);

New to 4.2 (4.1.3 ok)(also present in latest github)

Firefox, webkit, no javascript error

reproduced in duplicating "editor1" textarea => "editor2" in ckeditor/samples/replacebyclass.html

also obvious (and critical) in drupal, which uses a first instance as "summary'

Attachments (2)

10689.html (1.9 KB) - added by Piotrek Koszuliński 6 years ago.
mt-10689.html (2.9 KB) - added by Piotr Jasiun 6 years ago.
Manual Test

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by Jakub Ś

Status: newconfirmed

I was able to reproduce this in all browsers from CKE 4.2 just like @gifad has described.

comment:2 Changed 6 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.2.1

comment:3 Changed 6 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:4 Changed 6 years ago by Jakub Ś

#10696 was marked as duplicate.

comment:5 Changed 6 years ago by Piotr Jasiun

Status: assignedreview

comment:6 Changed 6 years ago by Piotr Jasiun

No tests. They would be too time consuming.

comment:7 Changed 6 years ago by Piotr Jasiun

One more commit with comment how (why!) it was actually working.

git:6ab7614

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

Status: reviewreview_failed

For future reference - why this was an incorrect duck typing:

    if ( !form.$.submit.nodeName && !form.$.submit.length )

This works correctly for the first editor - it checks whether submit property of a form is an element or HTML collection (of inputs with name=submit). However, it fails for the second one because overridden submit property now has a length property, because function has it too (it means the number of arguments listed in fn def). So the second editor cannot override form.$submit.

However, the if ( !form.$.submit.nodeName ) as proposed by PJ is not enough. Only IE8 allows to override form.submit being an element or HTML collection, and... it throws an error then. So we need more precise duck typing.

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

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

PS. I don't like the fact that save plugin works by an accident, but to fix this we would have to double the effort, so this topic should not be included in this ticket. Let's report a ticket for it after closing this one (because something may change in the meantime).

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

Changed 6 years ago by Piotrek Koszuliński

Attachment: 10689.html added

comment:10 Changed 6 years ago by Piotr Jasiun

Status: review_failedreview

Now I'm checking if form.$.submit is function using duck type. I cannot use typeof because for IE8 type of function is object.

And I also don't like magic in this part of code. I think we should use save event (instead of submit) to cancel submition when user press save button. I created ticket for it: #10748.

comment:11 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_passed

Changed 6 years ago by Piotr Jasiun

Attachment: mt-10689.html added

Manual Test

comment:12 Changed 6 years ago by Piotr Jasiun

Resolution: fixed
Status: review_passedclosed

comment:13 Changed 6 years ago by Piotr Jasiun

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