Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#10146 closed Bug (fixed)

Empty lines are removed in enter mode BR

Reported by: Jakub Ś Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.1.2
Component: General Version: 4.0 Beta
Keywords: Cc: mattleff@…, Chris Wells

Description (last modified by Jakub Ś)

To reproduce:

  1. Open enterkey sample and set enter mode to BR
  2. in WYSIWYG press new page command and then press Enter key few times. You will get:
    <br />
    <br />
    <br />
    
  3. Toggle between WYSIWYG and Source

Result: Every time you switch one BR gets removed until there is nothing in editor. This is in CKEditor 4 beta

  1. From CKEditor 4.0 you will get something like below when switching to source.
    <br />
    <br />
    <br />
    &nbsp;
    
  2. Every time you switch to source remove the &nbsp;. Only then br tag gets remove (one at a time)

Problem can be reproduced in all browsers from CKEditor 4 beta. It doesn't occur in 3.x branch.

Change History (34)

comment:1 Changed 6 years ago by Jakub Ś

Description: modified (diff)
Status: newconfirmed

comment:2 Changed 6 years ago by Jakub Ś

#10145 was marked as duplicate.

comment:3 Changed 6 years ago by Matthew Leffler

Cc: mattleff@… added

comment:4 Changed 6 years ago by Jakub Ś

Description: modified (diff)

comment:5 Changed 6 years ago by Jakub Ś

#10246 was marked as duplicate.

TC:
Paste below:

    <br /> 
    <br /> 
<br /> 
    <br /> 
    <p>This is a paragraph of text.</p> 
    <br /> 
    <br /> 
<br /> 
    <br /> 
    <p>Second paragraph of text.</p>
<br /> 
    <br /> 
    <p>Second paragraph of text.</p>

You get

<br />
<br />
<br />
&nbsp;
<p>This is a paragraph of text.</p>
<br />
<br />
<br />
&nbsp;
<p>Second paragraph of text.</p>
&nbsp;

<p>Second paragraph of text.</p>

This particular TC can be reproduced from CKEditor 4.0 but ths is because there is something behind br tag. If there was nothing result would be the same as in 4.0 beta.

comment:6 Changed 6 years ago by Jeremy Jannotta

Hi, is there an ETA on fixing this issue? It's actually a significant blocker for us upgrading to v4 (from 3.5.1), and would love to have a fix, or at least a workaround, so we can upgrade as soon as possible.

comment:7 Changed 6 years ago by Jakub Ś

Currently there is neither ETA nor workaround for this issue. I will however ask my colleagues about this.

The only let’s say good news is that after first change &nbsp; seem to block further 'br' removal.

UPDATE: Currently there is no eta for this ticket.

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

comment:8 Changed 6 years ago by Piotrek Koszuliński

I bisected the repo until the beginning of v4, but it is reproducible even then, so I'm not able to find a commit which has broken this.

comment:9 Changed 6 years ago by Jeremy Jannotta

Thanks for looking into this guys.

If we had to as a short-term workaround, can you think of a way to preserve the line breaks after they are input, such as on a 'dataReady' event, or using htmlDataProcessor, or the like?

Also, to clarify, even though the results look mostly the same, we're seeing all the br tags removed in this scenario, not just the last, so:

<br>
<br>
<p>This is a paragraph of text.</p>

ends up like

&nbsp;
<p>This is a paragraph of text.</p>

comment:10 Changed 6 years ago by Jakub Ś

I think there is a workaround for this issue. Its kind of hackish but result is rather consistent.

var editor = CKEDITOR.replace( 'editor1', {
	
				enterMode : CKEDITOR.ENTER_BR
			} );
			
			editor.on('pluginsLoaded' , function( evt ){ //it must be done before "instanceReady" to make the tag blocking in effect
						var counter = 0;
						evt.editor.dataProcessor.dataFilter.addRules({
							elements :{
								br : function( element ) {
									if(element.next.value == '&nbsp;' || element.next.name == 'br'){										
										counter++;
										return;										
									}
									
									if(counter <= 2){
										var helper = new CKEDITOR.htmlParser.text('&nbsp;');									
										helper.insertAfter(element);
									}
									counter = 0;			
								}
							}
						});
					});

The above code

//for:
<br>
<br>
<p>This is a paragraph of text.</p>
//and:
<br>
<p>This is a paragraph of text.</p>

will result in:
<br />
&nbsp;
<p>This is a paragraph of text.</p>


and for:
<br>
<br>
<br>
<br>
<p>This is a paragraph of text.</p>

will result in:

<br>
<br>
<br>
&nbsp;
<p>This is a paragraph of text.</p>

comment:11 in reply to:  10 Changed 6 years ago by Jeremy Jannotta

Replying to j.swiderski:

Thanks! This helps a lot, and and gets us so much closer.

I noticed a couple of things:

  • Changing counter <= 2 to counter <= 1 , fixes an issue with 3 BR tags resulting in 3 BRs and a NBSP (vs 2 BRS and a NBSP).
  • Have been playing around with this code, no luck so far, to find a way to preserve a single line break (with this addition, one BR results in BR and a NBSP, this two line breaks). Not as big a deal, but would like to preserve as much of the original formatting as possible. One BR resulting in one NBSP would be ok in this case.

I think there is a workaround for this issue. Its kind of hackish but result is rather consistent.

var editor = CKEDITOR.replace( 'editor1', {
	
				enterMode : CKEDITOR.ENTER_BR
			} );
			
			editor.on('pluginsLoaded' , function( evt ){ //it must be done before "instanceReady" to make the tag blocking in effect
						var counter = 0;
						evt.editor.dataProcessor.dataFilter.addRules({
							elements :{
								br : function( element ) {
									if(element.next.value == '&nbsp;' || element.next.name == 'br'){										
										counter++;
										return;										
									}
									
									if(counter <= 2){
										var helper = new CKEDITOR.htmlParser.text('&nbsp;');									
										helper.insertAfter(element);
									}
									counter = 0;			
								}
							}
						});
					});

The above code

//for:
<br>
<br>
<p>This is a paragraph of text.</p>
//and:
<br>
<p>This is a paragraph of text.</p>

will result in:
<br />
&nbsp;
<p>This is a paragraph of text.</p>


and for:
<br>
<br>
<br>
<br>
<p>This is a paragraph of text.</p>

will result in:

<br>
<br>
<br>
&nbsp;
<p>This is a paragraph of text.</p>

comment:12 Changed 6 years ago by Matthew Leffler

@j.swiderski, Can we get this targeted to a milestone? This is an annoying bug for us. :)

comment:13 in reply to:  12 Changed 6 years ago by Jeremy Jannotta

I second that, getting this on the roadmap would be great. Despite the workaround we have in place, which feels more like a tenuous band-aid, it's still not preserving the original formatting which is intended.

Replying to mattleff:

@j.swiderski, Can we get this targeted to a milestone? This is an annoying bug for us. :)

comment:14 Changed 6 years ago by Jakub Ś

I have informed my supervisor about this issue. I haven't received information about any milestone but now our senior developers know about this issue so there is a chance they will have it fixed in one of future releases.

comment:15 Changed 6 years ago by Chris Wells

Cc: Chris Wells added

Just adding myself to the CC here as this issue is keeping me from updating. @j.swiderski and I are trying to determine whether my submission #10381 is a duplicate of this one.

comment:16 Changed 6 years ago by Jakub Ś

#10381 was marked as duplicate.

Workaround used here doesn't quite work for #10381 example. Perhaps it will be possible to update this workaround.

comment:17 Changed 6 years ago by Chris Wells

Below is an updated workaround that handles my issue (#10381) as well. Unfortunately I couldn't find a way to improve on the previous code or combine it with mine better. I may be able to spend some free time on this later, but for now I've maxed out my work time for it (:

editor.on('pluginsLoaded' , function( evt ){ //it must be done before "instanceReady" to make the tag blocking in effect
  var previousNext = false;
  evt.editor.dataProcessor.htmlFilter.addRules({
    elements :{
      $ : function( element ) {
        if (element.previous || element.next) {
          if (previousNext && previousNext.value == '&nbsp;' && element.previous && element.previous.name =='br') {
            var helper = new CKEDITOR.htmlParser.text('<br>');                  
            helper.insertBefore(element);
            previousNext.remove();
          }
          previousNext = element.next;
        }
      }
    }
  });
  
  var counter = 0;
  evt.editor.dataProcessor.dataFilter.addRules({
    elements :{
      br : function( element ) {
        if(element.next && (element.next.value == '&nbsp;' || element.next.name == 'br')){                    
          counter++;
          return;                    
        }
            
        if(counter <= 1){
          var helper = new CKEDITOR.htmlParser.text('&nbsp;');                  
          helper.insertAfter(element);
        }
        
        counter = 0;      
      }
    }
  });
});

With this code...

<table>
	<tbody>
		<tr>
			<td>Table cell contents</td>
		</tr>
	</tbody>
</table>
<br>
<p>P contents</p>

will result in...

<table>
	<tbody>
		<tr>
			<td>Table cell contents</td>
		</tr>
	</tbody>
</table>
<br />
&nbsp;
<p>P contents</p>
Last edited 6 years ago by Chris Wells (previous) (diff)

comment:18 Changed 6 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.1.2

Let's try to have it in the 4.1.2.

comment:19 Changed 6 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:20 Changed 6 years ago by Olek Nowodziński

At the moment the following can be confirmed in ENTER_BR:

  • <br /> tags disappear one-by-one when switching WYSIWYG -> source -> WYSIWYG-> etc. only in Firefox. This can be easily fixed by disabling this replace in ENTER_BR.
  • Additionally, in all browsers, the last <br /> is recognized as a bogus and cleanBogus() removes it when htmlDataProcessor outputs data (toDataFormat). brFilter() appends &nbsp; in this place so for this code in WYSIWYG:
    foo<br /><br /><br />
    
    editor.getData() returns:
    foo<br /><br />\uA0
    

comment:21 in reply to:  20 Changed 6 years ago by Frederico Caldeira Knabben

Replying to a.nowodzinski:

  • <br /> tags disappear one-by-one when switching WYSIWYG -> source -> WYSIWYG-> etc. only in Firefox. This can be easily fixed by disabling this replace in ENTER_BR.

Yes, that's a bug.

  • Additionally, in all browsers, the last <br /> is recognized as a bogus and cleanBogus() removes it when htmlDataProcessor outputs data (toDataFormat). brFilter() appends &nbsp; in this place so for this code in WYSIWYG:
    foo<br /><br /><br />
    
    editor.getData() returns:
    foo<br /><br />\uA0
    

That's a feature and it's there to guarantee consistent rendering in all browsers. On editing the &nbsp; is reverted to <br>.

comment:22 Changed 6 years ago by Olek Nowodziński

Status: assignedreview

Created branches t/10146 in dev and tests-v4.

  • Proposed a simple fix for unnecessary bogus removal in Firefox.
  • Created TT for the ticket.
  • Created MT for the ticket.

comment:23 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

The mt is not needed. Ok, for the rest. It doesn't seem to bring regressions.

comment:24 Changed 6 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged fix into master, dev (​git:89d740763d), tests (c6ce5c800db385).

comment:25 in reply to:  9 Changed 6 years ago by Chris Wells

Hello folks, pardon my ignorance, but what happens with the additional bugs that were pointed out in the comments of this ticket? I have tested with the changes and see the improvement in Firefox, however my original ticket (#10381) that was marked as a duplicate of this one is not affected, nor is the scenario from jpj109's comment (from #10246, which was also marked as a duplicate of this issue).

comment:26 Changed 6 years ago by Jakub Ś

@cwells I have checked these issues you describe. In this case you would need to use web-tools to check editor behaviour :).

In WYSIWYG this is being displayed as br while in Source mode as &nbsp;. Current fix uses such transformations br->&nbsp;->br.

The only problem I have noticed is with:

<br>
<br>
<p>This is a paragraph of text.</p>

result for it is the same as for

<br>
<p>This is a paragraph of text.</p>

Both are changed in source mode into

&nbsp;
<p>This is a paragraph of text.</p>

Which means that for two br's one is permanently removed. Other br quantities (1,3,4...) are handled correctly.

comment:27 in reply to:  26 Changed 6 years ago by Chris Wells

Replying to j.swiderski:

@cwells I have checked these issues you describe. In this case you would need to use web-tools to check editor behaviour :).

Thanks for sticking with me on this. Could you elaborate on what you mean by using web-tools to check editor behavior? I'm sorry to be so adamant about this issue, it just happens that it affects a lot of places in our website that have <table>, <hX> and <br> elements that just won't play nice together in ckeditor4.

comment:28 Changed 6 years ago by Jakub Ś

I have meant tools like Firebug, Chrome web-tools or IE dev-tools.
The point is, you see in Source mode &nbsp; and in WYSIWYG br.

If you get data from editor e.g. with getData method you will see it contains BR. Please try your code with http://nightly.ckeditor.com/13-05-29-13-05/full/samples/api.html. You will see br is alerted.

comment:29 in reply to:  28 Changed 6 years ago by Chris Wells

Replying to j.swiderski:

I have meant tools like Firebug, Chrome web-tools or IE dev-tools.
The point is, you see in Source mode &nbsp; and in WYSIWYG br.

If you get data from editor e.g. with getData method you will see it contains BR. Please try your code with http://nightly.ckeditor.com/13-05-29-13-05/full/samples/api.html. You will see br is alerted.

Ok, understood. In my testing, however, I am not getting the <br> in getData. With Chrome's developer tools, here is what I get using a local copy of ckeditor-dev-master/plugins/enterkey/samples/enterkey.html:

I enter the following code in source mode:

<table>
	<tbody>
		<tr>
			<td>Table cell contents</td>
		</tr>
	</tbody>
</table>
<br>
<p>P contents</p>

and then change to WYSIWYG and run CKEDITOR.instances.editor1.getData() in console...

This is the result in ENTER_P mode:

<table>
	<tbody>
		<tr>
			<td>Table cell contents</td>
		</tr>
	</tbody>
</table>

<p>&nbsp;</p>

<p>P contents</p>

<p>&nbsp;</p>

This is the result in ENTER_BR mode:

<table>
	<tbody>
		<tr>
			<td>Table cell contents</td>
		</tr>
	</tbody>
</table>
&nbsp;

<p>P contents</p>

comment:30 Changed 6 years ago by Jakub Ś

Problem with enter mode P has already been described here - http://dev.ckeditor.com/ticket/10431.

About Enter mode BR – I’m sorry for my last comment. Actually what you see in source mode is rather the same thing you will get with getData method.
The behaviour should rather be the opposite - &nbsp; in source and br in WYSIWYG.

I will address this ticket to my colleague as now I simply don't know why it works that way. Perhaps there is no other way then presented in workaround and last BR has to changed to &nbsp;

comment:31 Changed 6 years ago by Jeremy Jannotta

Thanks for all your work looking into this, but I was wondering if there is any progress yet on solving my original test case?.

Release 4.1.2 does not fix it, and as commented in http://dev.ckeditor.com/ticket/10146#comment:26 , changing two <br>'s to a single &nbsp; is still a major issue at the very least.

Because this issue still exists, I vote we reopen this ticket/create a new ticket, as this is still a big problem.

comment:32 Changed 6 years ago by Jakub Ś

Replying to comment:31:
Original issue is http://dev.ckeditor.com/ticket/10146#comment:9 and yes it hasn't been fixed in IE, Opera and Webkit (only FF seems to work fine).

comment:33 Changed 6 years ago by Jakub Ś

#11308 was marked as duplicate.

This issue is continued in #11392.

comment:34 Changed 3 years ago by Jakub Ś

var editor = CKEDITOR.replace( 'editor1', {
	enterMode : CKEDITOR.ENTER_BR
});	

editor.on( 'pluginsLoaded', function( evt ){
	evt.editor.dataProcessor.dataFilter.addRules({
		elements :{
			br : function( element ) {			
				//if next element is BR or <!--cke_br_comment-->, ignore it.
				if( element && element.next && ( element.next.name == 'br' || element.next.value == 'cke_br_comment' ) ){
					return;
				}else {
					var comment = new CKEDITOR.htmlParser.comment( 'cke_br_comment' );
					comment.insertAfter( element );	
				}
			}
		}
	});
				
	evt.editor.dataProcessor.dataFilter.addRules({
		elements :{
			li : function( element ) {
				if( element && element.next && element.next.name == 'br' ){
					var nextElem = element.next;		
					while( nextElem.next && nextElem.next.name == 'br' ){
						nextElem = nextElem.next;
					}
					var comment = new CKEDITOR.htmlParser.comment( 'cke_br_comment' );
					comment.insertAfter( nextElem );	
				}
			}
		}
	}, 1 );//priority defaults to 10
				
	evt.editor.dataProcessor.htmlFilter.addRules({
		comment : function( value, node ) {
			if( value.indexOf('cke_br_comment') >= 0 ) {
				return false;
			}
		}
	});
				
});

Above is the workaround which works for all test cases mentioned here except one:

<br>
<br>
<p>This is a paragraph of text.</p>

For some reason br br element is always changed to br element

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