Opened 8 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

  1. 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).
  1. Using Internet Explorer, apply style Subtitle to one of the new lines and style Special Character to the another line.
  1. Click away somewhere and click in the lines where you applied the styles.
  1. 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 8 years ago by Jakub Ś

Keywords: IE added
Status: newconfirmed
Version: 4.5.44.0

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.

{ name: 'Italic Title',	element: 'h2', styles: { 'font-style': 'italic', 'color': '#aaa' } },
{ name: 'Subtitle', element: 'h3', styles: { 'color': '#aaa', 'font-style': 'bold' } },
{ name: 'Subtitle2', element: 'h3', styles: { 'color': 'rgb(170, 170, 170)', 'font-style': 'italic' } },
{ name: 'Subtitle3', element: 'h3', styles: { 'font-style': 'italic' } },

I was able to reproduce this issue from CKEditor 4.0.

comment:2 Changed 8 years ago by kkrzton

Owner: set to kkrzton
Status: confirmedassigned

comment:3 Changed 8 years ago by kkrzton

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 8 years ago by kkrzton

Status: assignedreview

Changes in t/14252 - while comparing css styles, the normalized (HEX) color representation is used.

comment:5 Changed 8 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.10

comment:6 Changed 8 years ago by Tomasz Jakut

Status: reviewreview_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 kkrzton

Status: review_failedreview

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 Tomasz Jakut

Status: reviewreview_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 kkrzton

Status: review_failedreview

Changes in t/14252.

I modified CKEDITOR.tools.normalizeHex to always lowercase the color representation.

comment:10 Changed 8 years ago by Tomasz Jakut

Resolution: fixed
Status: reviewclosed
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