Opened 8 years ago

Closed 7 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 7 years ago.
5531_2.patch (7.7 KB) - added by Sa'ar Zac Elias 7 years ago.
5531_3.patch (10.8 KB) - added by Sa'ar Zac Elias 7 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by Garry Yao

Keywords: Confirmed added
Milestone: CKEditor 3.4

comment:2 Changed 8 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 8 years ago by Sa'ar Zac Elias

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

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

Version: SVN (CKEditor)3.0

#5324 has been marked as dup

comment:5 Changed 7 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

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

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

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 5531.patch added

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

Status: assignedreview

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 5531_2.patch added

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

The second patch covers also the smiley and color dialogs.

comment:9 Changed 7 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 7 years ago by Sa'ar Zac Elias

Attachment: 5531_3.patch added

comment:10 Changed 7 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 7 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 7 years ago by Sa'ar Zac Elias

Resolution: fixed
Status: review_passedclosed

Fixed with [5758].

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