Opened 15 years ago

Closed 15 years ago

#3152 closed Bug (fixed)

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 (4)

3152.patch (9.6 KB) - added by Garry Yao 15 years ago.
3152_2.patch (9.5 KB) - added by Garry Yao 15 years ago.
3152_3.patch (9.5 KB) - added by Garry Yao 15 years ago.
3152_4.patch (385 bytes) - added by Garry Yao 15 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 15 years ago by Frederico Caldeira Knabben

Keywords: Confirmed added

comment:2 Changed 15 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

Changed 15 years ago by Garry Yao

Attachment: 3152.patch added

comment:3 Changed 15 years ago by Garry Yao

Keywords: Review? added

comment:4 Changed 15 years ago by Frederico Caldeira Knabben

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 15 years ago by Garry Yao

Attachment: 3152_2.patch added

comment:5 in reply to:  4 Changed 15 years ago by Garry Yao

Keywords: Review? added; Review- removed

Replying to fredck: Fixing the above mentioned issues.

comment:6 Changed 15 years ago by Frederico Caldeira Knabben

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 15 years ago by Garry Yao

Attachment: 3152_3.patch added

comment:7 in reply to:  6 ; Changed 15 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 15 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 15 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:10 Changed 15 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

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

Changed 15 years ago by Garry Yao

Attachment: 3152_4.patch added

comment:11 Changed 15 years ago by Garry Yao

Keywords: Review? added; Review+ removed
Resolution: fixed
Status: closedreopened

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

comment:12 Changed 15 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:13 Changed 15 years ago by Garry Yao

Resolution: fixed
Status: reopenedclosed

Fixed with [3379].

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