#4387 closed Bug (fixed)
Right clicking in Kama skin can lead to a javascript error
| Reported by: | fringley | 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)
Change History (13)
Changed 7 years ago by fringley
comment:1 Changed 7 years ago by tobiasz.cudnik
- Owner set to tobiasz.cudnik
- Status changed from new to assigned
comment:2 Changed 7 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 7 years ago by tobiasz.cudnik
- Keywords Pending added; Confirmed removed
comment:4 Changed 7 years ago by fringley
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 7 years ago by tobiasz.cudnik
comment:5 Changed 7 years ago by tobiasz.cudnik
- Keywords Confirmed Review? added; Pending removed
Changed array prototype explains the issue.
Attaching prepared patch.
comment:6 Changed 7 years ago by fringley
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 7 years ago by tobiasz.cudnik
comment:7 Changed 7 years ago by garry.yao
- Keywords Review+ added; Review? removed
comment:8 Changed 7 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 7 years ago by tobiasz.cudnik
- Resolution set to fixed
- Status changed from assigned to closed
Fixed with [4211].
comment:10 Changed 7 years ago by fringley
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.
Patch