Ticket #3309: 3309_3.patch

_source/plugins/styles/plugin.js

@@ -510 +510 @@
                  */
                 range.enlarge( CKEDITOR.ENLARGE_ELEMENT );
 
-                var bookmark = range.createBookmark( true ),
-                        startNode = range.document.getById( bookmark.startNode );
+                var bookmark = range.createBookmark(),
+                        startNode = bookmark.startNode;
 
                 if ( range.collapsed )
                 {
-                        /*
-                         * If the range is collapsed, try to remove the style from all ancestor
-                         * elements, until a block boundary is reached.
-                         */
-                        var startPath = new CKEDITOR.dom.elementPath( startNode.getParent() );
-                        for ( var i = 0, element ; i < startPath.elements.length && ( element = startPath.elements[i] ) ; i++ )
-                        {
+                        
+                        var startPath = new CKEDITOR.dom.elementPath( startNode.getParent() ),
+                                // The topmost element in elementspatch which we should jump out of.
+                                boundaryElement;
+
+
+                        for ( var i = 0, element ; i < startPath.elements.length
+                                        && ( element = startPath.elements[i] ) ; i++ )
+                        {
+                                /*
+                                 * 1. If it's collaped inside text nodes, try to remove the style from the whole element.
+                                 *
+                                 * 2. Otherwise if it's collapsed on element boundaries, moving the selection
+                                 *  outside the styles instead of removing the whole tag,
+                                 *  also make sure other inner styles were well preserverd.(#3309)
+                                 */
                                 if ( element == startPath.block || element == startPath.blockLimit )
                                         break;
 
                                 if ( this.checkElementRemovable( element ) )
                                 {
-                                        /*
-                                         * Before removing the style node, there may be a sibling to the style node
-                                         * that's exactly the same to the one to be removed. To the user, it makes
-                                         * no difference that they're separate entities in the DOM tree. So, merge
-                                         * them before removal.
-                                         */
-                                        mergeSiblings( element );
-                                        removeFromElement( this, element );
-                                }
-                        }
-                }
-                else
+                                        var endOfElement = range.checkBoundaryOfElement( element, CKEDITOR.END ),
+                                                        startOfElement = !endOfElement && range.checkBoundaryOfElement( element, CKEDITOR.START );
+                                        if ( startOfElement || endOfElement )
+                                        {
+                                                boundaryElement = element;
+                                                boundaryElement.match = startOfElement ? 'start' : 'end';
+                                        }
+                                        else
+                                        {
+                                                /*
+                                                 * Before removing the style node, there may be a sibling to the style node
+                                                 * that's exactly the same to the one to be removed. To the user, it makes
+                                                 * no difference that they're separate entities in the DOM tree. So, merge
+                                                 * them before removal.
+                                                 */
+                                                mergeSiblings( element );
+                                                removeFromElement( this, element );
+
+                                        }
+                                }
+                        }
+
+                        // Re-create the style tree after/before the boundary element,
+                        // the replication start from bookmark start node to define the
+                        // new range.
+                        if ( boundaryElement )
+                        {
+                                var clonedElement = startNode;
+                                for ( var i = 0 ;; i++ )
+                                {
+                                        var newElement = startPath.elements[ i ];
+                                        if ( newElement.equals( boundaryElement ) )
+                                                break;
+                                        // Avoid copying any removable style element.
+                                        else if( newElement.match )
+                                                continue;
+                                        else
+                                                newElement = newElement.clone();
+                                        newElement.append( clonedElement );
+                                        clonedElement = newElement;
+                                }
+                                clonedElement[ boundaryElement.match == 'start' ?
+                                                        'insertBefore' : 'insertAfter' ]( boundaryElement );
+                        }
+                }
+                else
                 {
                         /*
                          * Now our range isn't collapsed. Lets walk from the start node to the end

_source/core/dom/range.js

@@ -296 +296 @@
                 };
         }
 
+        // Evaluator for CKEDITOR.dom.element::checkBoundaryOfElement, reject any
+        // text node and non-empty elements unless it's being bookmark text.
+        function elementBoundaryEval( node )
+        {
+                // Reject any text node unless it's being bookmark.
+                return node.type != CKEDITOR.NODE_TEXT
+                       && node.getName() in CKEDITOR.dtd.$removeEmpty
+                           || node.getParent().hasAttribute( '_fck_bookmark' );
+        };
+
         CKEDITOR.dom.range.prototype =
         {
                 clone : function()
@@ -1467 +1477 @@
                         };
                 },
 
+                /**
+                 * Check whether current range is on the inner edge of the specified element.
+                 * @param {Number} checkType ( CKEDITOR.START | CKEDITOR.END ) The checking side.
+                 * @param {CKEDITOR.dom.element} element The target element to check.
+                 */
+                checkBoundaryOfElement : function( element, checkType )
+                {
+                        var walkerRange = this.clone();
+                        // Expand the range to element boundary.
+                        walkerRange[ checkType == CKEDITOR.START ?
+                         'setStartAt' : 'setEndAt' ]
+                         ( element, checkType == CKEDITOR.START ?
+                           CKEDITOR.POSITION_AFTER_START
+                           : CKEDITOR.POSITION_BEFORE_END );
+
+                        var walker = new CKEDITOR.dom.walker( walkerRange ),
+                         retval = false;
+                        walker.evaluator = elementBoundaryEval;
+                        return walker[ checkType == CKEDITOR.START ?
+                                'checkBackward' : 'checkForward' ]();
+                },
                 // Calls to this function may produce changes to the DOM. The range may
                 // be updated to reflect such changes.
                 checkStartOfBlock : function()
@@ -1593 +1624 @@
 CKEDITOR.ENLARGE_ELEMENT = 1;
 CKEDITOR.ENLARGE_BLOCK_CONTENTS = 2;
 CKEDITOR.ENLARGE_LIST_ITEM_CONTENTS = 3;
+
+/**
+ * Check boundary types.
+ * @see CKEDITOR.dom.range::checkBoundaryOfElement
+ */
+CKEDITOR.START = 1;
+CKEDITOR.END = 2;
+CKEDITOR.STARTEND = 3;

_source/tests/plugins/styles/styles.html

@@ -336 +336 @@
 
                 test_ticket_2040 : function()
                 {
-                        doc.getById( '_P1' ).setHtml( 'This is some <strong>sample text<\/strong>. You are using <a href="http://www.fckeditor.net/">CKEditor<\/a>.' );
+                        doc.getById( '_P1' ).setHtml( 'This is some <strong>sample text<\/strong>. You are using <a href="http://www.fckeditor.net/">ckeditor<\/a>.' );
 
                         var range = new CKEDITOR.dom.range( doc );
                         range.setStart( doc.getById( '_P1' ), 1 );
@@ -345 +345 @@
                         var style = new CKEDITOR.style( { element : 'i' } );
                         style.applyToRange( range );
 
-                        assert.areSame( 'this is some <strong><i>sample</i> text<\/strong>. you are using <a href="http://www.fckeditor.net/">CKEditor<\/a>.', getInnerHtml( '_P1' ) );
+                        assert.areSame( 'this is some <strong><i>sample</i> text<\/strong>. you are using <a href="http://www.fckeditor.net/">ckeditor<\/a>.', getInnerHtml( '_P1' ) );
                 },
 
                 test_checkElementRemovable1 : function()
@@ -486 +486 @@
                         assert.areSame( '<p><i title="z">text</i></p><i title="z">outter</i>', getInnerHtml( element ) );
                 },
 
+                // Remove inline style when range collapsed at element boundaries,
+                // move out of the removing-style element, with inner style copied.
+                test_ticket_3309 : function()
+                {
+                        var element = doc.getById( '_P1' );
+                        element.setHtml( 'this is some <b><i id="_i1">styles</i></b> text' );
+
+                        // This is some <b><i>styles^</i></b> text
+                        var range = new CKEDITOR.dom.range( doc );
+                        range.setStartAt( doc.getById( '_i1' ), CKEDITOR.POSITION_BEFORE_END );
+
+                        var style = new CKEDITOR.style( { element : 'b' } );
+                        style.removeFromRange( range );
+
+                        assert.areSame( 'this is some <b><i id="_i1">styles</i></b><i></i> text', getInnerHtml( element ) );
+                },
+
+                // No inner style preserved, simply move out of the removing-style element.
+                test_ticket_3309_2 : function()
+                {
+                        var element = doc.getById( '_P1' );
+                        element.setHtml( 'this is some <b id="_b1">styles</b> text' );
+
+                        // This is some <b>styles^</b> text
+                        var range = new CKEDITOR.dom.range( doc );
+                        range.setStartAt( doc.getById( '_b1' ), CKEDITOR.POSITION_BEFORE_END );
+
+                        var style = new CKEDITOR.style( { element : 'b' } );
+                        style.removeFromRange( range );
+                        // This is some <b>styles</b>^ text
+                        assert.areSame( doc.getById( '_b1' ).getParent().$, range.startContainer.$ );
+                        assert.areSame( 2, range.startOffset );
+                        assert.areSame( 'this is some <b id="_b1">styles</b> text', getInnerHtml( element ) );
+                },
+
+                // With style overrides.
+                test_ticket_3309_3 : function()
+                {
+                        var element = doc.getById( '_P1' );
+                        element.setHtml( 'text <strong><bold><span><b><i id="_i1">styles</i></b></span></bold></strong>' );
+
+                        // text <strong><bold><span><b><i id="_i1">^styles</i></b></span></bold></strong>
+                        var range = new CKEDITOR.dom.range( doc );
+                        range.setStartAt( doc.getById( '_i1' ), CKEDITOR.POSITION_AFTER_START );
+
+                        var style = new CKEDITOR.style( { element : 'b' , overrides : [ 'strong', 'bold' ] } );
+                        style.removeFromRange( range );
+
+                        // text <span><i>^</i></span><bold><span><b><i>styles</i></b></span></bold>
+                        assert.areSame( 'text <span><i></i></span><strong><bold><span><b><i id="_i1">styles</i></b></span></bold></strong>', getInnerHtml( element ) );
+                },
                 name : document.title
         };
 })() );
-
+//window.onload = testCase.test_ticket_3309_3;
         //]]>
         </script>
 </head>