Opened 9 years ago
Closed 8 years ago
#14252 closed Bug (fixed)
Styles dropdown doesn't always reflect the current style of the text line in Internet Explorer
Reported by: | Lynn Robinson | Owned by: | kkrzton |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.5.10 |
Component: | General | Version: | 4.0 |
Keywords: | IE | Cc: |
Description
Steps to reproduce
- Using the latest nightly build full demo eg. http://nightly.ckeditor.com/15-12-10-07-07/full/samples/ , add some new lines of text in the sample text by hitting enter and typing in some new lines (very short lines that don't wrap).
- Using Internet Explorer, apply style Subtitle to one of the new lines and style Special Character to the another line.
- Click away somewhere and click in the lines where you applied the styles.
- The style name is not displayed in the styles combo/dropdown.
Expected result
The style name should be displayed in the styles combo/dropdown.
Actual result
The style name is not displayed in the styles combo/dropdown.
Other details (browser, OS, CKEditor version, installed plugins)
This bug only happens in IE and not in Firefox or Google Chrome. Some styles (such as Italic Title) do display in the combo box. If you can tell me why some of the styles do appear, it would be nice if you could email me with this information so maybe I can change my style definitions. It doesn't seem to only be a case of whether it is a block vs an inline style (e.g. the Marker style works OK). My own custom styles don't show in the combo in IE and I don't know how to get it to work.
Change History (10)
comment:1 Changed 9 years ago by
Keywords: | IE added |
---|---|
Status: | new → confirmed |
Version: | 4.5.4 → 4.0 |
comment:2 Changed 9 years ago by
Owner: | set to kkrzton |
---|---|
Status: | confirmed → assigned |
comment:3 Changed 9 years ago by
Indeed it is a problem with color attr. The style attribute in IE looks like this
background:#eeeeee;border:1px solid #cccccc;padding:5px 10px;
but when it is accessed by element.getAttribute function the resulted string looks like this
background: rgb(238, 238, 238); padding: 5px 10px; border: 1px solid rgb(204, 204, 204);
When checking if selected element has same styles as one of predefined Styles (compareCssText function which uses parseCssText function to transform style string to object) the color attr values does not match.
Probably color attributes should be normalized (from RGB to HEX notation).
comment:4 Changed 9 years ago by
Status: | assigned → review |
---|
Changes in t/14252 - while comparing css styles, the normalized (HEX) color representation is used.
comment:5 Changed 8 years ago by
Milestone: | → CKEditor 4.5.10 |
---|
comment:6 Changed 8 years ago by
Status: | review → review_failed |
---|
If we are going to change the way we normalize colors, we should do it once, in a most universal way possible. Said that, this fix is not universal enough, e.g. case described in #14541 is still failing on branch:t/14252.
To reproduce, simply add element with rgb()
color, e.g.:
<span style="color: rgb(255, 255, 0);>Yellow</span>
and then check if such style match:
new CKEDITOR.style( { element: 'span', styles: { color: '#f00' } } );
With the current codebase, it fails.
What's more I think that the fix is applied in the wrong place. IMO we should just move the color normalization code outside the `if` statement – it will cause the color normalization to be working always, not only when forcing native normalization.
comment:7 Changed 8 years ago by
Status: | review_failed → review |
---|
Changes in t/14252.
I agree this wasn't the most universal approach. As you pointed out the color value in styles should be always normalized (not only when nativeNormalize flag is set) so I moved this code outside the if statement. I added one test case which you mentioned in your comment above.
However, this test case revealed another issue. The short HEX representation is not normalized so when comparing with full length representation (e.g. #f00 and #ff0000) the test fails. I added a method to normalize short HEX representation.
comment:8 Changed 8 years ago by
Status: | review → review_failed |
---|
I've pushed some minor changes with new test-case to branch:t/14252.
The code looks good, but there is one remaining issue: the new CKEDITOR.tools.normalizeHex
method doesn't normalize upper-case colors, e.g. #FFFF00
/#FF0
is not normalized to #ffff00
.
comment:9 Changed 8 years ago by
Status: | review_failed → review |
---|
comment:10 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Fixed with git:0528e5aaeb104ffd283aeb4ff5efa2fcdb0616e4.
It seems that color is the problem here. In below example (should be put into styles.js) first two styles will show "Styles" label on styles dropdown while the third will show "Subtitle3". It looks like color causes problems when styles need to be matched but also is ignored when there is shorter equivalent.
I was able to reproduce this issue from CKEditor 4.0.