Opened 2 years ago

Last modified 2 years ago

#14845 confirmed Bug

Using justify in BR mode doesn't remove trailing BR's in IE thus resulting in new line

Reported by: Jakub Ś Owned by:
Priority: Nice to have (we want to work on it) Milestone:
Component: General Version: 4.0
Keywords: Support IE10 IE9 IE8 Cc: jayhamiltoniv@…

Description

Steps to reproduce

  1. Configure CKEditor so that it uses (you can also use allowedContent:true for simplicity)
    var editor = CKEDITOR.replace( 'editor1', {
    	enterMode : CKEDITOR.ENTER_BR,	
    	allowedContent : '*{*}'
    });
    
  2. Switch to source mode and paste below HTML:
    <div >
    	<div >
    		<div style="font-family: tahoma,arial; font-size: 9pt;">
    			<div  style="padding: 4px;">
    				<div  >
    					<span style="border: 0pt rgb(0, 0, 0); text-align: left; color: rgb(0, 0, 0); padding-top: 0px; padding-bottom: 0px; font-style: normal; font-weight: normal; text-decoration: none; margin-top: 0px; margin-bottom: 0px;">
    						<span style="padding-top: 0px; padding-bottom: 0px; margin-top: 0px; margin-bottom: 0px;">
    							<span class="ddspellcheck" id="scayt3">jkhgkjh</span>
    						</span>
    						<br>
    						<span style="padding-top: 0px; padding-bottom: 0px; margin-top: 0px; margin-bottom: 0px;">
    							<span class="ddspellcheck" id="scayt2">gkjhgkjh</span>
    						</span>
    						<br>
    						<span style="padding-top: 0px; padding-bottom: 0px; margin-top: 0px; margin-bottom: 0px;">
    							<span class="ddspellcheck" id="scayt1">gkjhlgfkhj</span>
    						</span>
    					</span>
    					<br>
    				</div>
    			</div>
    		</div>
    	</div>
    </div>
    
  3. Switch to wysiwyg, click inside middle line and click justify center.

Expected result

Middle line gets centered.

Actual result

Middle line gets centred but extra line is added. This is happening because BR in first span (which gets wrapped into div) doesn't get removed like in modern browsers.

<div>
	<div>
		<div style="font-family: tahoma,arial; font-size: 9pt;">
			<div style="padding: 4px;">
				<div>
					<span style="border: 0pt rgb(0, 0, 0); text-align: left; color: rgb(0, 0, 0); padding-top: 0px; padding-bottom: 0px; font-style: normal; font-weight: normal; text-decoration: none; margin-top: 0px; margin-bottom: 0px;">
						<span style="padding-top: 0px; padding-bottom: 0px; margin-top: 0px; margin-bottom: 0px;">
							<span class="ddspellcheck" id="scayt3">jkhgkjh</span> 
						</span>
					</span>
					<br /> <!-- This doesn't get reoved -->
					&nbsp;
				</div>
				<div style="text-align: center;">
					<span style="padding-top: 0px; padding-bottom: 0px; margin-top: 0px; margin-bottom: 0px;">
						<span class="ddspellcheck" id="scayt2">gkjhgkjh</span> 
					</span>
				</div>

				<div>
					<span style="border: 0pt rgb(0, 0, 0); text-align: left; color: rgb(0, 0, 0); padding-top: 0px; padding-bottom: 0px; font-style: normal; font-weight: normal; text-decoration: none; margin-top: 0px; margin-bottom: 0px;">
						<span style="padding-top: 0px; padding-bottom: 0px; margin-top: 0px; margin-bottom: 0px;">
							<span class="ddspellcheck" id="scayt1">gkjhlgfkhj</span> 
						</span> 
					</span>
				</div>
			</div>
		</div>
	</div>
</div>

Other details (browser, OS, CKEditor version, installed plugins)

Problem can be reproduced from CKEditor 4.0 in IE10 and below

Change History (8)

comment:1 Changed 2 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 2 years ago by Deepak Abburi

Hi, I have a fix for the issue in core/dom/iterator.js file. I hope it help the engineer who works on the issue. if we replace the following code

if ( removePreviousBr ) {
				var previousSibling = block.getPrevious();
				if ( previousSibling && previousSibling.type == CKEDITOR.NODE_ELEMENT ) {
					if ( previousSibling.getName() == 'br' )
						previousSibling.remove();
					else if ( previousSibling.getLast() && previousSibling.getLast().$.nodeName.toLowerCase() == 'br' )
						previousSibling.getLast().remove();
				}
			}

			if ( removeLastBr ) {
			var lastChild = block.getLast();
	if ( lastChild && lastChild.type == CKEDITOR.NODE_ELEMENT && lastChild.getName() == 'br' ) {
					// Remove br filler on browser which do not need it.
if ( !CKEDITOR.env.needsBrFiller || lastChild.getPrevious( bookmarkGuard ) || lastChild.getNext( bookmarkGuard ) )
						lastChild.remove();
				}
			}

with the following

if ( removePreviousBr ) {
				var previousSibling = block.getPrevious();
				if ( previousSibling && previousSibling.type == CKEDITOR.NODE_ELEMENT ) {
				    if (previousSibling.getName() == 'br')
				        previousSibling.remove();

				    else {
				        var prevSib = previousSibling;
				        while (prevSib.$.childNodes.length) {
				            if (prevSib.getLast() && prevSib.getLast().$.nodeName.toLowerCase() == 'br') {
				                prevSib.getLast().remove();
				            }
				            prevSib = prevSib.getLast();

				        }
				    }
					  
			}
		}

			if ( removeLastBr ) {
			    var lastChild = block.getLast();
				if ( lastChild && lastChild.type == CKEDITOR.NODE_ELEMENT && lastChild.getName() == 'br' ) {
				    //Take care not to remove the block expanding <br> in non-IE browsers.
if ( CKEDITOR.env.ie || lastChild.getPrevious( bookmarkGuard ) || lastChild.getNext( bookmarkGuard ) )
				        lastChild.remove();
				}
	else if (lastChild.getLast() && lastChild.getLast().getName() == 'br')
				{
				    lastChild.getLast().remove();
				}
else if (lastChild.getLast().getLast() && lastChild.getLast().getLast().getName() == 'br') {
				    lastChild.getLast().getLast().remove();
				}
			}

it will fix the issue. But i am not sure about the other effects of that code change.

Thanks, Deepak

Last edited 2 years ago by Jakub Ś (previous) (diff)

comment:3 Changed 2 years ago by Jay

Cc: jayhamiltoniv@… added

comment:4 Changed 2 years ago by Tade0

Thank you for your contribution. I'm seeing that this works well as a solution to this specific edge case.

Chances are that the problem lies somewhere deeper, though, so we need to look for a more general solution - if one exists.

Investigating this now.

comment:5 Changed 2 years ago by Deepak Abburi

Hi ,

what i see is if we have the things in the following format the issue happens.

<div>
    <span>
        text1<br>
        text2<br>
        text3<br>
     </span>
</div>

So the issue may be dealing with splitting the blocks where we have this type of pattern. When we split the things in this case we are not removing the old br in the markup. the extra line will be there depending on span with BR sometimes we may get extra line before where we justifying and some times we may get extra br after the line where we are justifying so it really depends on pattern. when we try iterator.getnextparagraph() the issue happens so may be when we split things we can go ahead remove the additional br at end of different child positions.On my debugging i have seen br in different child position depending on depending on spans inside div. so may be removal of br from text while we split block may fix the issue. But i am not sure on that.

I just want give this because may be this is a simple pattern which will help the situation more clear for you while you are trying to fix the things.

Thanks, Deepak

comment:6 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.0CKEditor 4.6.1

Unfortunately we won't be able to squeeze it into 4.6.0, retargeting for 4.6.1 though.

comment:7 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2

Moving to 4.6.2 minor release, as 4.6.1 is mostly about polishing 4.6.0.

comment:8 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.2
Priority: NormalNice to have (we want to work on it)
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy