Ticket #5045 (closed Bug: fixed)
uiColor creates wrong css-code when editor-id contains period(.) when using multiple editors
| Reported by: | nmiller | Owned by: | alfonsoml |
|---|---|---|---|
| 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
Change History
comment:1 in reply to: ↑ description Changed 3 years ago by nmiller
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:3 Changed 3 years ago by alfonsoml
- 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 3 years ago by alfonsoml
- Owner set to alfonsoml
- Status changed from new to assigned
comment:5 Changed 3 years ago by garry.yao
- Keywords Confirmed added
- Milestone changed from CKEditor 3.3 to CKEditor 3.4
I would vote for the second approach for cleanness, anyway, we could handle it in the next release.
comment:6 Changed 3 years ago by alfonsoml
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 3 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 3 years ago by fredck
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.
comment:9 Changed 3 years ago by alfonsoml
- 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:11 Changed 3 years ago by tobiasz.cudnik
- Status changed from review to review_failed
- Keywords Confirmed removed
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.
comment:12 Changed 3 years ago by alfonsoml
- Status changed from review_failed to review
The new patch uses just tools.getNextId() so it's valid and it doesn't add anything extra.
comment:13 Changed 3 years ago by garry.yao
- Status changed from review to review_passed
tools::escapeCssSelector need to be removed before committing.
comment:14 Changed 3 years ago by alfonsoml
- Status changed from review_passed to closed
- Resolution set to fixed
Fixed with [5956]
