Opened 9 years ago

Closed 8 years ago

#2886 closed Task (fixed)

Implement logic for handling bogus BR nodes.

Reported by: Martin Kou Owned by: Martin Kou
Priority: Normal Milestone: CKEditor 3.0
Component: General Version: SVN (FCKeditor) - Retired
Keywords: IBM 3.0RC Review+ Cc:

Description

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

Attachments (10)

2886.patch (4.2 KB) - added by Martin Kou 9 years ago.
2886_2.patch (9.0 KB) - added by Frederico Caldeira Knabben 9 years ago.
2886_ref.patch (8.5 KB) - added by Garry Yao 8 years ago.
sample.html (1.3 KB) - added by Garry Yao 8 years ago.
Sample file for testing padding bock BR
2886_3.patch (10.5 KB) - added by Martin Kou 8 years ago.
2886_4.patch (7.8 KB) - added by Martin Kou 8 years ago.
2886_5.patch (7.8 KB) - added by Martin Kou 8 years ago.
2886_6.patch (7.1 KB) - added by Martin Kou 8 years ago.
2886_7.patch (7.5 KB) - added by Martin Kou 8 years ago.
2886_8.patch (896 bytes) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (36)

comment:1 Changed 9 years ago by Martin Kou

Owner: set to Martin Kou
Status: newassigned
Type: BugTask

Changed 9 years ago by Martin Kou

Attachment: 2886.patch added

comment:2 Changed 9 years ago by Martin Kou

Keywords: Review? added

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.

comment:3 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
Owner: changed from Martin Kou to Frederico Caldeira Knabben
Status: assignednew

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.

Changed 9 years ago by Frederico Caldeira Knabben

Attachment: 2886_2.patch added

comment:4 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed
Status: newassigned

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.

comment:5 Changed 9 years ago by Martin Kou

Keywords: Review- added; Review? removed

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

comment:6 Changed 8 years ago by Martin Kou

#2956 is related to this ticket.

comment:7 Changed 8 years ago by Garry Yao

Keywords: IBM added

comment:8 in reply to:  5 Changed 8 years ago by 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.

Changed 8 years ago by Garry Yao

Attachment: 2886_ref.patch added

comment:9 Changed 8 years ago by 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>
    

Changed 8 years ago by Garry Yao

Attachment: sample.html added

Sample file for testing padding bock BR

comment:10 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: 3.0RC added

comment:11 Changed 8 years ago by Martin Kou

Owner: changed from Frederico Caldeira Knabben to Martin Kou
Status: assignednew

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

comment:12 Changed 8 years ago by Martin Kou

Keywords: Review? added; Review- removed

Changed 8 years ago by Martin Kou

Attachment: 2886_3.patch added

comment:13 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

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.

Changed 8 years ago by Martin Kou

Attachment: 2886_4.patch added

comment:14 Changed 8 years ago by Martin Kou

Keywords: Review? added; Review- removed

Added 9 test cases in the new patch.

Changed 8 years ago by Martin Kou

Attachment: 2886_5.patch added

comment:15 Changed 8 years ago by Martin Kou

Fixed a minor error in the comments.

comment:16 Changed 8 years ago by Garry Yao

Keywords: Review- added; Review? removed

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>

comment:17 Changed 8 years ago by Martin Kou

Keywords: Review? added; Review- removed

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.

comment:18 Changed 8 years ago by 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.

Changed 8 years ago by Martin Kou

Attachment: 2886_6.patch added

comment:19 Changed 8 years ago by Garry Yao

Keywords: Review- added; Review? removed

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.$?

Changed 8 years ago by Martin Kou

Attachment: 2886_7.patch added

comment:20 Changed 8 years ago by Martin Kou

Keywords: Review? added; Review- removed

comment:21 Changed 8 years ago by Garry Yao

Keywords: Review+ added; Review? removed

comment:22 Changed 8 years ago by Martin Kou

Resolution: fixed
Status: newclosed

Fixed with [3660].

Click here for more info about our SVN system.

comment:23 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: closedreopened

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.

Changed 8 years ago by Garry Yao

Attachment: 2886_8.patch added

comment:24 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review+ removed

comment:25 Changed 8 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:26 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: reopenedclosed

Fixed with [3665].

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