Opened 9 years ago

Closed 9 years ago

#3240 closed Bug (fixed)

Font style not correct with existed style

Reported by: Garry Yao Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed IBM Review+ Cc:

Description

When inline style is applied, it should always create at the possible outmost, with selection enlarged.

Reproducing Procedures

  1. Open the replace by class example page;
  2. Make the content along with selection as below by double click 'text':
    <b><i>^text^</i></b>
    
  3. Click on 'Font' to open combo and select 'Arial' option;
    • Expected Result:
      <p>
      	<span style="font-family: Arial,Helvetica,sans-serif;"><b><i>text</i></b></span></p>
      
    • Actual Result: An JavaScript Error thrown;
      <p>
      	<b><i><span style="font-family: Arial,Helvetica,sans-serif;">text</span></i></b></p>
      

Attachments (5)

3240.patch (684 bytes) - added by Garry Yao 9 years ago.
test_node_getPosition.patch (2.0 KB) - added by Garry Yao 9 years ago.
Unit Test Case
3240_2.patch (2.6 KB) - added by Garry Yao 9 years ago.
3240_3.patch (6.2 KB) - added by Garry Yao 9 years ago.
3240_4.patch (5.0 KB) - added by Frederico Caldeira Knabben 9 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 9 years ago by Garry Yao

This one should be resolved after fixing #3091.

comment:2 Changed 9 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

#3091 is going to fixed by another approach which doesn't resolve the issue of this bug, so a specific fix should be found.

Changed 9 years ago by Garry Yao

Attachment: 3240.patch added

comment:3 Changed 9 years ago by Garry Yao

Keywords: Review? added

comment:4 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

Confirmed with IE. Ok with Firefox.

The patch has no effect for me though. These is the way I've reproduced it:

  1. Pasted the following into Source view:
<b><i>text</i></b>
  1. Switched to WYSIWYG.
  2. Double-click on the word "text".
  3. Selected a Font style.

The <span> has been created inside <b><i>, instead of outside of it. I've reproduced it in the same way before and after the patch with IE7.

Changed 9 years ago by Garry Yao

Attachment: test_node_getPosition.patch added

Unit Test Case

comment:5 Changed 9 years ago by Garry Yao

I've found the real cause for this problem is a hideous bug from CKEDITOR.dom.node::getPosition within IE, see the attached TC for detail.

Changed 9 years ago by Garry Yao

Attachment: 3240_2.patch added

comment:6 Changed 9 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:7 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

It looks like recent changes have already fixed this one. I'm not able to reproduce it anymore. Can you confirm it?

comment:8 Changed 9 years ago by Garry Yao

Keywords: Review? added; Review- removed

I've confirmed that the problem still exist for IE6 at least, and here's the execution stack which cause the problem:
getPosition() applyInlineStyle applyToRange() applyStyle apply()

comment:9 Changed 9 years ago by Martin Kou

Keywords: IBM added

comment:10 Changed 9 years ago by Martin Kou

Keywords: Review- added; Review? removed

The patch works.

But it seems to me much of the code in getPosition() can be eliminated if getAddress() is used to compare the node positions. Once we've got the index path of two nodes comparing their positions becomes very easy.

Changed 9 years ago by Garry Yao

Attachment: 3240_3.patch added

comment:11 Changed 9 years ago by Garry Yao

Keywords: Review? added; Review- removed

Simplify the CKEDITOR.dom.node::getPosition function.

comment:12 Changed 9 years ago by Martin Kou

Keywords: Review- added; Review? removed
  1. The performance optimization of using native compareDocumentPosition() is a good idea - CKEDITOR.dom.node::getPosition() is used in a DFS-like while loop in the style plugin, so performance is important there.
  2. I can see you copied John Resig's compareDocumentPosition() adaptor for IE here: http://ejohn.org/blog/comparing-document-position/. The values returned are correct.. But please format the code better:
// e.g. If we followed John Resig's indenting... which doesn't necessarily make sense.
            function standard( a, b )
            {
                return a.compareDocumentPosition ?
                    a.compareDocumentPosition( b ) : 
                        a.contains ?
                            ( a != b && a.contains( b ) && CKEDITOR.POSITION_CONTAINS ) +
                                ( a != b && b.contains( a ) && CKEDITOR.POSITION_IS_CONTAINED ) +
                                ( a.sourceIndex >= 0 && b.sourceIndex >= 0 ? 
                                    ( a.sourceIndex < b.sourceIndex && CKEDITOR.POSITION_PRECEDING ) +
                                        ( a.sourceIndex > b.sourceIndex && CKEDITOR.POSITION_FOLLOWING ) :
                                    CKEDITOR.POSITION_DISCONNECTED ) +
                            CKEDITOR.POSITION_IDENTICAL :
                            CKEDITOR.POSITION_IDENTICAL;
            }

So, Review- based on minor code readability issues. The patch is otherwise very good.

comment:13 Changed 9 years ago by Frederico Caldeira Knabben

  • The CKEDITOR.env object should not be extended for such very specific cases. It also impacts in the size of ckeditor_basic.js.
  • There is not need of an complete function replacement here. We just need to have Martin's suggestion applied for those cases where compareDocumentPosition, contains and sourceIndex are not supported.

Changed 9 years ago by Frederico Caldeira Knabben

Attachment: 3240_4.patch added

comment:14 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed
Owner: changed from Garry Yao to Frederico Caldeira Knabben
Status: assignednew

This new patch simply incorporates Garry's proposal with a few enhancements to it.

comment:15 Changed 9 years ago by Garry Yao

Keywords: Review+ added; Review? removed

comment:16 Changed 9 years ago by Garry Yao

Owner: changed from Frederico Caldeira Knabben to Garry Yao
Status: newassigned

Fixed with [3532].

comment:17 Changed 9 years ago by Garry Yao

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