basicstyle override tag not working

[garry.yao]

Specifically the overrides of both <b> and <i> are not working.

[fredck]

After applying the patch, the basic style detection stoped working. It's enough to load a sample and place the caret inside the bold words. The bold button will not get highlighted.

Other than that:

• On "checkElementRemovable":
  • The "element.getName()" check has been removed, but the code that followed it must be executed just in the case of element name match. So, that code must now go inside an "if" block, just like in V2.
  • The function must "return false" at it's very last line.
  • Actually, it's enough to precisely replicate the V2 code.
• We have changed the way utility functions are being defined in the style plugin code. So, just like getAttributesForComparison, getOverrides should be a private function, not anymore a propert into the style class.
• In removeFromElement, the added loop on "attributes" is a duplicate of the code already present a few lines above in the same function.
• There are several unrelated (and wrong) coding style changes on the "for" blocks.

[garry.yao]

Replying to fredck: Fixing the above mentioned issues.

[fredck]

Still a new issues:

• The cache trick at line 936 has no effect. It simply changes the pointer of arguments.callee to another function, but will not (fortunately) change future calls for getOverrides. I've said fortunately, because getOverrides is not a private function, used by several different styles. The right solution is caching it in the parameter "style" object.

• If you look at the colobutton plugin, "colorButton_foreStyle" defines a style that has an override for the <font> element, containing the "color" attribute (null means "any value" there). It means that if you have the following:

<font color="red">Test</font>

Doing a "select all" and applying a new color, the <font> tag should disappear. What we have instead is the following:

<span style="color: rgb(0, 0, 255);"><font color="red">Test</font></span>

• As you will have a chance to review your changes, please fix the coding style at line 679.

[garry.yao]

Replying to fredck:

Still a new issues:

• The cache trick at line 936 has no effect.

Yes, it's a big mistake.

• If you look at the colobutton plugin, "colorButton_foreStyle" defines a style that has an override for the <font> element, containing the "color" attribute (null means "any value" there). It means that if you have the following:

<font color="red">Test</font>

Doing a "select all" and applying a new color, the <font> tag should disappear. What we have instead is the following:

<span style="color: rgb(0, 0, 255);"><font color="red">Test</font></span>

I found the issued was caused by a deeper hiding bug from #3087, which should not be fixed at this ticket, verified by the following codes:

CKEDITOR.tools.isArray( new CKEDITOR.style( CKEDITOR.instances.editor1.config.colorButton_foreStyle.overrides )._.definition.overrides )

[garry.yao]

Replying to garry.yao:

Replying to fredck:

I found the issued was caused by a deeper hiding bug from #3087, which should not be fixed at this ticket, verified by the following codes:

CKEDITOR.tools.isArray( new CKEDITOR.style( CKEDITOR.instances.editor1.config.colorButton_foreStyle.overrides )._.definition.overrides )

I should have make myself clear that the CKEDITOR.tools.clone is used by L89 of _source\plugins\styles\plugin.js when constructing CKEDITOR.style object.

[garry.yao]

Fixed with [3376]. Click here for more info about our SVN system.

[garry.yao]

I've found another logic flaw with function 'checkElementRemoveable'.

[garry.yao]

Fixed with [3379].