basicstyle override tag not working

[Garry Yao]

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

[Frederico Caldeira Knabben]

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.

[Frederico Caldeira Knabben]

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].