Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#4387 closed Bug (fixed)

Right clicking in Kama skin can lead to a javascript error

Reported by: Chris Tingley Owned by: Tobiasz Cudnik
Priority: Normal Milestone: CKEditor 3.1
Component: General Version: 3.0
Keywords: Confirmed Review+ Cc:

Description

This in turn would then stop any toolbar buttons working that opened a dialog window.

Bug was chased down to iterating over an array in the Kama skin javascript file (patch attached)

Attachments (3)

ticket_4387.patch (483 bytes) - added by Chris Tingley 10 years ago.
Patch
4387.patch (1.3 KB) - added by Tobiasz Cudnik 10 years ago.
4387_2.patch (2.5 KB) - added by Tobiasz Cudnik 10 years ago.

Download all attachments as: .zip

Change History (13)

Changed 10 years ago by Chris Tingley

Attachment: ticket_4387.patch added

Patch

comment:1 Changed 10 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

comment:2 Changed 10 years ago by Tobiasz Cudnik

Keywords: Confirmed added

What version of safari is affected by this issue ?

It looks strange as it's just a "for in" loop over array. I can't reproduce it on newest safari 4 and safari 3.2.2.

Anyway, thank you for the patch.

comment:3 Changed 10 years ago by Tobiasz Cudnik

Keywords: Pending added; Confirmed removed

comment:4 Changed 10 years ago by Chris Tingley

I had the problem in 4.0.3.

I was a little puzzled by the problem as well, but generally it is bad practice to use for..in loops with arrays as it is not reliable if anything changes the array prototype.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 4387.patch added

comment:5 Changed 10 years ago by Tobiasz Cudnik

Keywords: Confirmed Review? added; Pending removed

Changed array prototype explains the issue.

Attaching prepared patch.

comment:6 Changed 10 years ago by Chris Tingley

The patch I made to our version is from the 3.0 release (which if different to the one in Trunk) which seemed to use the same iteration technique in several places. You may want to double-check anywhere else that uses for..in.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 4387_2.patch added

comment:7 Changed 10 years ago by Garry Yao

Keywords: Review+ added; Review? removed

comment:8 Changed 10 years ago by Tobiasz Cudnik

You're right, there were 3 more.

After deeper checking, there is quite a bunch of for..in loops inside core code. Most of it iterates over objects (i think nobody extends Object.prototype those days ?), but some of them like editor.js L189 are same cases like this one. Doesn't it create any issue for you ?

comment:9 Changed 10 years ago by Tobiasz Cudnik

Resolution: fixed
Status: assignedclosed

Fixed with [4211].

comment:10 Changed 10 years ago by Chris Tingley

As yet, I've not had any more javascript errors occur, but it is possible that in my use I have not exercised those code paths.

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