Implement logic for handling bogus BR nodes.

[Martin Kou]

The bogus BR logic is needed in many dialog and style-related commands (e.g. table, list, enter key, etc.)

[Martin Kou]

Issues fixed in the patch:

1. BRs at the end of blocks are replaced by non-breaking spaces as in v2.
2. BRs added inside PRE blocks are replaced by \r\n.
3. Fixed incorrect indenting and whitespace truncation for PRE blocks.

There's no need to port AppendBogusBR and CreateBogusBR since the BR handling logic does not depend on special attributes like type=_moz at all.

[Frederico Caldeira Knabben]

I've thoroughly though about this issue since the start of the V3 coding. Actually, we always had problems with unneeded <br> and &nbsp;.

The solution here should be quite simply. It's enough to understand that <br>, if placed at the end of a block, is used exclusively to set the "line" height. It guarantees that a line has some content, even if invisible and not selectable.

So, if we are sure the block already has content, we can safely discard the <br>. Otherwise, we just leave it as is.

The proposed patch, other than taking a different approach (with &nbsp;), is also making the <br> cleanup inside the DOM, which is wrong. A getData() call should never change the DOM. If changes are to be done, they must go in the code that generates the output.

[Frederico Caldeira Knabben]

I'm be attaching a new patch here. It handles the same issues proposed by Martin's patch, but it acts mainly in the CKEDITOR.htmlParser.fragment class. We can safely assume that the fragment class will always return a cleaned up piece of HTML.

This patch is much simpler, and should also perform much better, as it's part of the parsing pass.

I've included some tests in it. It would be nice to have a larger set of HTML snippets to enlarge the test cases.

[Martin Kou]

The patch does not work when there are multiple empty paragraphs in Firefox.

Also, it does not give consistent results when the user presses Shift-Enter to create an empty new line at the end of paragraph.

So Review-.

[Martin Kou]

#2956 is related to this ticket.

[Garry Yao]

Replying to martinkou:

The patch does not work when there are multiple empty paragraphs in Firefox.

Also, it does not give consistent results when the user presses Shift-Enter to create an empty new line at the end of paragraph.

So Review-.

I've tried with a proposed patch based on Fred's patch which should pass Martin's TCs.

[Garry Yao]

As discussed with Martin, Fred's approach of removing the invisible padding block 'br' mostly works well for the current situation, making either 'cke_bogus' or '_moz_dirty' transparent for the source code sweeping.

Attaching a sample page to demonstrate the idea, as well as updated the patch to current trunk, with the following TC adjustments:

1. The processing result with the following content:
   

   <div><p>A<br><br></p></div>
   

   should become:

   <div><p>A<br></p></div>
   

2. The processing result with the following content:
   

   <div><p><br><br>A</p></div>
   

   should remain the same:

   <div><p><br>A</p></div>
   

[Garry Yao]

Sample file for testing padding bock BR

[Martin Kou]

I've just discussed this ticket with Fred, and I was asked to take ownership of this ticket.

[Frederico Caldeira Knabben]

I've noticed the additions to the filter.js file, and I think we don't really need them. Everything could be handled by the onElement filtering, making the patch a bit simpler.

The changes on fragment.js look good.

[Martin Kou]

Added 9 test cases in the new patch.

[Martin Kou]

Fixed a minor error in the comments.

[Garry Yao]

As discussed with Martin, this input is not working properly in FF:

<p>
 text<br /></p>

it's expected to be the following when switch to wysiwyg mode and switch back:

<p>
 text<br />
&nbsp;</p>

[Martin Kou]

Your test case is actually correct.

The raw HTML input is usually meant for HTML code already existing outside of the editor. e.g. A CKEditor plugin in Drupal. So the

<p>text<br /></p>

code you have will have come from a Drupal page being edited.

And in this case, the <br /> makes no difference. Because outside of browser editing mode, a <br /> at the tail of a block is never visible. No matter if it's IE or Firefox. So it is correct for the editor to trim it to

<p>text</p>

and display that in WYSIWYG mode instead.

There's a small bug in the previous patch about this case, actually. Previously, the <br> trimming logic would be executed for non-IE browsers only. But in this case, the <br /> should be trimmed even for IE. So in my new patch, the trimming logic is run like this:

1. If we're converting from WYSIWYG mode data to output/Source mode data, then the <br> would be trimmed only for non-IE browsers. The reason is that tail <br> nodes are visible in IE editing mode.
2. If we're converting from raw HTML data to WYSIWYG data, then the <br> would always be trimmed for any browser. The reason is that the tail <br> is always invisible outside of editing mode in any browser.

[Martin Kou]

A bit of clarification... "Your test case is actually correct." was meant to say the behavior of 2886_5.patch, which trims the <br /> away from the HTML input, is correct. Adding an &nbsp; to the tail is wrong.

What's wrong 2886_5.patch is that it's not doing the trimming in IE.

[Garry Yao]

Things begin to work perfectly, and it's good to see now everything went into filter, just spotted two minor issues:

1. table/ui/ol is not necessarily to be inside blockLikeTags;
2. The 'extendBlockFor...' is registering to corresponding elements, this may conflicting with any other filter rules regarding a specific element ( though we don't have now ), do you think we can register to the generic entry rules.element.$?

[Martin Kou]

Fixed with [3660].

Click here for more info about our SVN system.

[Garry Yao]

When constructing parser nodes:

block.children.push( new CKEDITOR.htmlParser.text( '\xa0' ) );

we should also update their parent for newly create text, now there's a lot of hang up text nodes resulted.

[Garry Yao]

Fixed with [3665].