Ticket #3152 (closed Bug: fixed)

Opened 6 years ago

Last modified 5 years ago

basicstyle override tag not working

Reported by: garry.yao Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.0
Component: General Version: SVN (FCKeditor) - Retired
Keywords: Confirmed Review+ Cc:

Description

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

Attachments

3152.patch (9.6 KB) - added by garry.yao 5 years ago.
3152_2.patch (9.5 KB) - added by garry.yao 5 years ago.
3152_3.patch (9.5 KB) - added by garry.yao 5 years ago.
3152_4.patch (385 bytes) - added by garry.yao 5 years ago.

Change History

comment:1 Changed 6 years ago by fredck

  • Keywords Confirmed added

comment:2 Changed 5 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from new to assigned

Changed 5 years ago by garry.yao

comment:3 Changed 5 years ago by garry.yao

  • Keywords Review? added

comment:4 follow-up: ↓ 5 Changed 5 years ago by fredck

  • Keywords Review- added; Review? removed

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.

Changed 5 years ago by garry.yao

comment:5 in reply to: ↑ 4 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

Replying to fredck: Fixing the above mentioned issues.

comment:6 follow-up: ↓ 7 Changed 5 years ago by fredck

  • Keywords Review- added; Review? removed

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.

Changed 5 years ago by garry.yao

comment:7 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

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 )

comment:8 in reply to: ↑ 7 Changed 5 years ago by 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.

comment:9 Changed 5 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:10 Changed 5 years ago by garry.yao

  • Status changed from assigned to closed
  • Resolution set to fixed

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

Changed 5 years ago by garry.yao

comment:11 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review+ removed
  • Status changed from closed to reopened
  • Resolution fixed deleted

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

comment:12 Changed 5 years ago by martinkou

  • Keywords Review+ added; Review? removed

comment:13 Changed 5 years ago by garry.yao

  • Status changed from reopened to closed
  • Resolution set to fixed

Fixed with [3379].

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy