Opened 11 years ago

Closed 10 years ago

#5531 closed Bug (fixed)

All IDs and classes should all have the 'cke_' prefix

Reported by: Sa'ar Zac Elias Owned by: Sa'ar Zac Elias
Priority: Normal Milestone: CKEditor 3.4
Component: General Version: 3.0
Keywords: Cc:

Description

While I was reading the code I've noticed that not all IDs (e.g. in dialogs) and classes (e.g. in the colordialog plugin) uses the 'cke_' prefix, some of them are quite common IDs such as 'picker'. This can lead to collision and make the integration problematic.

Attachments (3)

5531.patch (5.9 KB) - added by Sa'ar Zac Elias 10 years ago.
5531_2.patch (7.7 KB) - added by Sa'ar Zac Elias 10 years ago.
5531_3.patch (10.8 KB) - added by Sa'ar Zac Elias 10 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 11 years ago by Garry Yao

Keywords: Confirmed added
Milestone: CKEditor 3.4

comment:2 Changed 11 years ago by Sa'ar Zac Elias

Has a relation with #5138, so maybe both bugs will be handled in one patch.
Maybe a generic function should be created to get IDs with the 'cke_' prefix and at the same time get the IDs in the same convention (e.g. all letters lowercase or capitalize).

comment:3 Changed 11 years ago by Sa'ar Zac Elias

Another thing that should be changed, the dialog UI generates IDs like '91_uiElement'.

comment:4 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Version: SVN (CKEditor)3.0

#5324 has been marked as dup

comment:5 Changed 10 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

comment:6 Changed 10 years ago by Sa'ar Zac Elias

Owner: changed from Tobiasz Cudnik to Sa'ar Zac Elias

Changed 10 years ago by Sa'ar Zac Elias

Attachment: 5531.patch added

comment:7 Changed 10 years ago by Sa'ar Zac Elias

Status: assignedreview

Changed 10 years ago by Sa'ar Zac Elias

Attachment: 5531_2.patch added

comment:8 Changed 10 years ago by Sa'ar Zac Elias

The second patch covers also the smiley and color dialogs.

comment:9 Changed 10 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed
  • I still can see several improper ids in the dialogs.
  • We can see that most of the ids are build with the same formula: 'cke_' + CKEDITOR.tools.getNextNumber(). It makes sense introducing a new CKEDITOR.tools.getNextId() function which makes the code simpler, shorter, and guarantees the results are uniform.

Also, just to be sure, let's target it to the 3.4.x branch.

Changed 10 years ago by Sa'ar Zac Elias

Attachment: 5531_3.patch added

comment:10 Changed 10 years ago by Sa'ar Zac Elias

Keywords: Confirmed removed
Status: review_failedreview

I used a tool script to list all elements without the 'cke_' prefix this time.

comment:11 Changed 10 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

That's cool :)

Later we can also consider removing all those suffixes, as they don't bring anything useful, other than making the code bigger.

comment:12 Changed 10 years ago by Sa'ar Zac Elias

Resolution: fixed
Status: review_passedclosed

Fixed with [5758].

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