Font style not correct with existed style

[Garry Yao]

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>
     

[Garry Yao]

This one should be resolved after fixing #3091.

[Garry Yao]

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

[Frederico Caldeira Knabben]

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>

2. Switched to WYSIWYG.
3. Double-click on the word "text".
4. 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.

[Garry Yao]

Unit Test Case

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

[Frederico Caldeira Knabben]

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

[Garry Yao]

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

[Martin Kou]

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.

[Garry Yao]

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

[Martin Kou]

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.

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

[Frederico Caldeira Knabben]

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

[Garry Yao]

Fixed with [3532].