Opened 14 years ago

Closed 13 years ago

#5045 closed Bug (fixed)

uiColor creates wrong css-code when editor-id contains period(.) when using multiple editors

Reported by: Nick Owned by: Alfonso Martínez de Lizarrondo
Priority: Normal Milestone: CKEditor 3.4.2
Component: General Version: 3.1
Keywords: Cc:

Description

Similar to ticket #4546 If you have mutiple editors in a page (using replace textarea by code) using uiColor and the textarea id contains a character needing escaping (.:) only the last editor will have the required colour.

Attachments (6)

uicolor_test.txt (2.3 KB) - added by Nick 14 years ago.
5045.patch (1.2 KB) - added by Alfonso Martínez de Lizarrondo 14 years ago.
Proposed patch
5045_2.patch (3.7 KB) - added by Garry Yao 14 years ago.
5045_3.patch (2.4 KB) - added by Alfonso Martínez de Lizarrondo 14 years ago.
Updated patch
5045_4.patch (2.7 KB) - added by Alfonso Martínez de Lizarrondo 14 years ago.
Revised patch
5045_5.patch (2.7 KB) - added by Alfonso Martínez de Lizarrondo 13 years ago.
Updated patch

Download all attachments as: .zip

Change History (20)

Changed 14 years ago by Nick

Attachment: uicolor_test.txt added

comment:1 in reply to:  description Changed 14 years ago by Nick

Replying to nmiller:

Similar to ticket #4546 If you have mutiple editors in a page (using replace textarea by code) using uiColor and the textarea id contains a character needing escaping (.:) only the last editor will have the required colour.

NOTE: Only a problem in the IE browser (tested with IE 8 on Windows)

comment:2 Changed 14 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.3

Changed 14 years ago by Alfonso Martínez de Lizarrondo

Attachment: 5045.patch added

Proposed patch

comment:3 Changed 14 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added

The problem is that when the new rules are appended to the stylesheet, the current content is returned by IE unescaped, so when it's reparsed it throws it away as invalid rules. The patch puts back again the escapes to fix the problem.

Anyway, this works only for Standards mode. Since 3.2 it seems that this feature is broken for IE in quirks.

Another approach that could fix both problems would be to generate "clean" ids for the editor elements, removing any of those special characters and avoiding the issue altogether.

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

Owner: set to Alfonso Martínez de Lizarrondo
Status: newassigned

Changed 14 years ago by Garry Yao

Attachment: 5045_2.patch added

comment:5 Changed 14 years ago by Garry Yao

Keywords: Confirmed added
Milestone: CKEditor 3.3CKEditor 3.4

I would vote for the second approach for cleanness, anyway, we could handle it in the next release.

Changed 14 years ago by Alfonso Martínez de Lizarrondo

Attachment: 5045_3.patch added

Updated patch

comment:6 Changed 14 years ago by Alfonso Martínez de Lizarrondo

I've based the new patch on your proposal, but I've simplified it as I didn't see the need to change the getNextNumber function.

comment:7 Changed 14 years ago by Garry Yao

Keywords: Review- added; Review? removed

My point is that if we're introducing editor::id we should make it somehow meaningful, instead of a random id, does that make sense?

comment:8 Changed 14 years ago by Frederico Caldeira Knabben

Documentation for CKEDITOR.editor.id: A unique random number assigned to each editor instance in the page.

That's ok... there is no need to make it sequential, or logic, as long as it's unique. In this way it can be useful as an API feature, and I think others will also appreciate it.

There is no need to prefix it though. It can be simply a number.

Additionally, removing "cke_editor_ + name" from the class attribute is wrong at this point, as it's not backwards compatible. So, I wound instead add the id simply.

Changed 14 years ago by Alfonso Martínez de Lizarrondo

Attachment: 5045_4.patch added

Revised patch

comment:9 Changed 14 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; Review- removed

The reason to add the prefix for the id was that an all numbers class name isn't valid IIRC, so instead of putting the prefix in different places, it seemed simpler to do it at the initialization.

comment:10 Changed 14 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4CKEditor 3.5

comment:11 Changed 13 years ago by Tobiasz Cudnik

Keywords: Confirmed removed
Status: reviewreview_failed

I'm on Frederico's side here, that we should keep editor::id as simple and clean as possible and we need only a number.

We already keep redundant strings in various places as prefixes (eg "cke_") without a problem, so we can here.

Changed 13 years ago by Alfonso Martínez de Lizarrondo

Attachment: 5045_5.patch added

Updated patch

comment:12 Changed 13 years ago by Alfonso Martínez de Lizarrondo

Status: review_failedreview

The new patch uses just tools.getNextId() so it's valid and it doesn't add anything extra.

comment:13 Changed 13 years ago by Garry Yao

Status: reviewreview_passed

tools::escapeCssSelector need to be removed before committing.

comment:14 Changed 13 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: review_passedclosed

Fixed with [5956]

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy