Extra BR added after pasting in the first line

[Piotrek Koszuliński]

1. Manually add three text paragraphs within editor;
2. Make selection like following:

   <p>f[oo</p>
   <p>bar</p>
   <p>ba]z</p>
   

3. Copy and paste it, in place

• Actual Result: One extra line space is added after the pasted first line.

[Garry Yao]

The removing the padding BR at the end of pseudo block, must not be handled simply by the "data filter", it should be a coordination between the "html" and "data" filter, the current impl fails the following tc:

1. Load the following source:

   <div>foo^
   <p>bar</p>
   </div>
   

2. Shift-enter to create two BRs
3. Switch to source mode.
4. Switch to wysiwyg mode now.
5. Repeat 3-4.

• Actual: BRs start to loose on each switch.

Besides, we shall have some tests for such a change.

[Piotrek Koszuliński]

I did some tests and I noticed that FF doesn't append <br> if one already is at the end of pseudo block. Thus, I had to fix the code that it removes filler only if it's single one.

Also, I decided to do this in both direction - toHtml and toDataSource. Thanks to this if we'll press shift+enter once in Garry's case (what causes incorrect FF behaviour :|), then we won't see it after switching to source mode. The same after setting: <div>foo<br><p>bar</p></div> and switching from source mode to wysiwyg.

I updated tests in t/9220@tests.

PS. On IE8 when shift+enter pressed once (in Garry's case) strange things start to happen. I'll attach a movie showing how left arrow is looped in paragraph for this HTML: <div>foo<br><p>bar</p></div>. That's not a part of this ticket, but needs new one.

[Garry Yao]

With the current patch, if first paragraph in ticket TC has duo BR at the end, the issue is still reproducible.

[Garry Yao]

I intend to take over this ticket as the padding BR logic has some historical concern that requires some awareness of v3 tickets.

[Garry Yao]

Opened review t/9220b at both dev and test, features bogus handling at the end of pseudo block, changes illustrated as:

• core/editable.js - protected the data as "clip", to avoid loosing whitespace and brs through parsing.
• core/htmldataprocessor.js - the main chunk, it revised the block filter, to consider bogus inside of pseudo-block, the blockFilter is to remove bogus inside of a block, and the brFilter is to append filler after line-break BR.
• core/htmlparser/fragment.js - Required because of the above changes, to trim correctly the whitespace at the end of pseudo-block, raise the robustness of the enter filtering system, it mainly concerns the following sample of HTML emitted by IE8:

  foo
  <b>
  <p>bar</p>
  </b>
  

• plugins/bbcode/plugin.js - Required because of the new assignment of type "fragment" to parser's root node.
• plugins/list/plugin.js - The changes there is to revert the some legacy list filtering logic that is not anymore require, exactly on topic of the pseudo block.
• plugins/wysiwygarea/plugin.js - Revert the change of an additional bogus br at the end of editable, also handled by core now.

[Piotrek Koszuliński]

Reviewing...

[Piotrek Koszuliński]

1. One selection test is failing on FF.
2. There are no tests for issues mentioned in this ticket.
3. I think that some functions should be documented more clearly. E.g.:
   • rtrim / fragment.js
   • blockFilter / htmldataprocessor
   • parse.onText / htmlparser - nbsps as separate nodes case - how and why (in fact it's a good question)
4. I don't like the idea of wrapping content that's going to be inserted in some markers. That's hacking our own code. This idea is returning like a boomerang all the time and I constantly have mixed feelings about it. The real problem is that we're using dataProcessor for other purpose than it was created. Dunno if we've got enough time to rework it now, but IMO we definitely should at least add second mode for insertion data processing.
5. I believe that changes proposed in t/9220b may not be compatible with #9292. In #9292 I avoided preserving ltrimming insertion data, so please review my ticket, because we've got to make a decision what goes first.

[Garry Yao]

1. Still to be checked...
2. htmldataprocessor.html#test bogus nodes (in)output in pseudo block
3. The documentation for rtrim / fragment.js and blockFilter / htmldataprocessor are enough for me, regard NBSP separation in parse.onText / htmlparser, I've committed new change, to remove it from the parser, leaving it to be handled by the processor instead, so it's usage is much clearer now.
4. Unless you can sniff the side effects from the wrapping marker, I see it the only way for impl. when a new filtering system is still in empty talk.
5. The changes are not "conflicting" with #9292, but it's overlapping, so it's enough to just loose the change from #9292 on editable.js

[Garry Yao]

I've checked the selection test failure on Fx, it's related to the change, but it looks like the problem is instead the feature itself, the selection change event on data ready, which doesn't look like a right decision, as theoretically no selection is available at that moment when the DOM is reloaded (on set data), I have no RED after the removal, so I propose a commit to remove it.

[Piotrek Koszuliński]

I pushed imnor code corrections.

R- because of failing tests (possibly came out after rebasing):

• ​http://ckeditor4.t/dt/core/editable/insertlist.html - all browsers
• ​http://ckeditor4.t/dt/core/editable/inserthtml.html - IE8
• ​http://ckeditor4.t/dt/plugins/table/table.html - IE8

Regarding documentation - it's enough for you, because you know the code / you're its author. I, a person who only knows basic concepts and tests, have a lot harder. Especially that this isn't a trivial algorithm or some business logic, which code can be read as a book. It's pretty complicated and not obvious piece of code doing complicated and not obvious things. Without thorough documentation it cannot be understood in other way than just by reading every single line and every single case. That's why I asked about few more words about it :).

[Garry Yao]

Because the review is getting harder, I have to recreate the branch, with the minimal changes targeting the ticket TC, the tests are not changed, but are also squashed for easier review.

Also to make the logic clearer, I've explained future more of the bogus/filler handling logic in code comments.

[Piotrek Koszuliński]

I've force-pushed both branches - rebased dev and added missing spaces and aligned columns in tests.

• 100% green.
• Cases mentioned in this issue passing on Chrome, FF, IE8 and Opera.

R+!

[Garry Yao]

Masterised with:

​https://github.com/ckeditor/ckeditor-dev/commit/4cd29d2257ec480ee19a9751900a879d4eeb27b7