Ticket #2886 (closed Task: fixed)

Opened 6 years ago

Last modified 5 years ago

Implement logic for handling bogus BR nodes.

Reported by: martinkou Owned by: martinkou
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

2886.patch (4.2 KB) - added by martinkou 6 years ago.
2886_2.patch (9.0 KB) - added by fredck 6 years ago.
2886_ref.patch (8.5 KB) - added by garry.yao 6 years ago.
sample.html (1.3 KB) - added by garry.yao 6 years ago.
Sample file for testing padding bock BR
2886_3.patch (10.5 KB) - added by martinkou 5 years ago.
2886_4.patch (7.8 KB) - added by martinkou 5 years ago.
2886_5.patch (7.8 KB) - added by martinkou 5 years ago.
2886_6.patch (7.1 KB) - added by martinkou 5 years ago.
2886_7.patch (7.5 KB) - added by martinkou 5 years ago.
2886_8.patch (896 bytes) - added by garry.yao 5 years ago.

Change History

comment:1 Changed 6 years ago by martinkou

  • Status changed from new to assigned
  • Owner set to martinkou
  • Type changed from Bug to Task

Changed 6 years ago by martinkou

comment:2 Changed 6 years ago by martinkou

  • 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 6 years ago by fredck

  • Owner changed from martinkou to fredck
  • Status changed from assigned to new
  • Keywords Review- added; Review? removed

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 6 years ago by fredck

comment:4 Changed 6 years ago by fredck

  • Keywords Review? added; Review- removed
  • Status changed from new to assigned

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 follow-up: ↓ 8 Changed 6 years ago by martinkou

  • 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 6 years ago by martinkou

#2956 is related to this ticket.

comment:7 Changed 6 years ago by garry.yao

  • Keywords IBM added

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

comment:9 Changed 6 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 6 years ago by garry.yao

Sample file for testing padding bock BR

comment:10 Changed 5 years ago by fredck

  • Keywords 3.0RC added

comment:11 Changed 5 years ago by martinkou

  • Owner changed from fredck to martinkou
  • Status changed from assigned to new

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

comment:12 Changed 5 years ago by martinkou

  • Keywords Review? added; Review- removed

Changed 5 years ago by martinkou

comment:13 Changed 5 years ago by fredck

  • 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 5 years ago by martinkou

comment:14 Changed 5 years ago by martinkou

  • Keywords Review? added; Review- removed

Added 9 test cases in the new patch.

Changed 5 years ago by martinkou

comment:15 Changed 5 years ago by martinkou

Fixed a minor error in the comments.

comment:16 Changed 5 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 5 years ago by martinkou

  • 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 5 years ago by martinkou

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 5 years ago by martinkou

comment:19 Changed 5 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 5 years ago by martinkou

comment:20 Changed 5 years ago by martinkou

  • Keywords Review? added; Review- removed

comment:21 Changed 5 years ago by garry.yao

  • Keywords Review+ added; Review? removed

comment:22 Changed 5 years ago by martinkou

  • Status changed from new to closed
  • Resolution set to fixed

Fixed with [3660].

Click here for more info about our SVN system.

comment:23 Changed 5 years ago by garry.yao

  • Status changed from closed to reopened
  • Resolution fixed deleted

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 5 years ago by garry.yao

comment:24 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review+ removed

comment:25 Changed 5 years ago by martinkou

  • Keywords Review+ added; Review? removed

comment:26 Changed 5 years ago by garry.yao

  • Status changed from reopened to closed
  • Resolution set to fixed

Fixed with [3665].

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