Extra line break inside of created links

[Wiktor Walc]

When creating links, CKEditor adds <br /> inside of the <a> tag. Confirmed in FF, Safari and Opera.

Steps to reproduce

• go to ​http://ckeditor.com/demo
• press "Ctrl + A" and then "Del" to delete all contents
• click on the "Link" button, type some url, like ckeditor.com
• result:

  <p>
  	<a href="http://ckeditor.com">http://ckeditor.com<br />
  	</a></p>
  

[Garry Yao]

Re-target it to 3.1 since 'Link' command isn't the only victim of this bug, one more serious symptom below:

Reproducing Procedures

1. Open any sample page, click 'New Page' to clear up content;
2. Type one word 'text' then 'Shift Enter' to create one line-break but don't type anything;
3. Go back selecting the word 'text' by double clicking it;
4. Click on 'Bold' command on it, then click on next line;
5. Start typing something on that line.

• Actual Result: The Bold style from last line applied on it.
• Expected Result: The second line shouldn't have any style.

[Garry Yao]

Ticket Test added at :
​http://ckeditor.t/tt/4886/1.html.

[Frederico Caldeira Knabben]

We've already started the testing phase of the 3.1. That's not the moment to add further tickets to it. Only regressions are to be considered.

[Joe Kavanagh]

[FF] Line breaks are also getting added to paragraphs.

1. Open any sample page, click 'New Page' to clear up content.
2. Type one word, 'text', then press Enter.
3. Repeat step 2 above two more times to create three paragraphs in total.
4. On the second paragraph select 'text' by double clicking it, and delete it.
5. Press Enter, then press Backspace.
6. Start typing something on that line.

• Actual Result: There is a leading empty line in the second paragraph caused by an inserted line break.
• Expected Result: The paragraphs should be evenly spaced. There should be no line break at the start of the second paragraph.

[Alfonso Martínez de Lizarrondo]

The patch seems to work fine, except for the case of comment 5, but I'm not sure that it's related to this issue.

Anyway, I don't understand why the lineContent is defined with 2 parameters if it's called only from one place.
If it's better leave it that way, the purpose of isReject should be explained (it's also missing in invisible that it's also called only once)
And I don't understand the double negation on the return of the function.

[Frederico Caldeira Knabben]

Replying to alfonsoml:

And I don't understand the double negation on the return of the function.

It's there just to make the function return a real Boolean (=== true/false).

[Alfonso Martínez de Lizarrondo]

Yes, I understand what it does.

I meant: I don't know why it's placed there, for me it seems useless: isReject is a boolean (true) and it seems that isLineContent is also a boolean always, so there's no need for that extra check.

[Frederico Caldeira Knabben]

Replying to alfonsoml:

I meant: I don't know why it's placed there, for me it seems useless: isReject is a boolean (true) and it seems that isLineContent is also a boolean always, so there's no need for that extra check.

There is no extra check there, but simply a type conversion as bitwise operations always return Number types:

( true ^ false ) === 1;       // true
( true ^ false ) === true;    // false
!!( true ^ false ) === true;  // true

[Garry Yao]

...except for the case of comment 5, but I'm not sure that it's related to this issue.

For me 5 is talking about the keystroke (Backspace) problem with BR, which desires a new dedicated ticket, while we're concentrate here on the style system.

[Frederico Caldeira Knabben]

The proposed patch is the "lazy way" to have it done. It leaves <br> and empty spaces being added to the ranges to later remove them when effectively applying the style.

The problem must be solved at the it's root instead, by avoiding adding those nodes to the range even before trying to apply the style.

I'll provide a new patch for it.

[Garry Yao]

4886_2.patch has brought the following regression:

Reproducing Procedures

1. Open any sample page, load the editor with the following source and selection:

   [line1<br/><br />]
   

2. Apply bold style and move cursor to the second empty line;
3. Start typing.

• Expected Result: The inserted text should be bold styled.

[Frederico Caldeira Knabben]

So we must go with the way I've thought about it since the beginning. Instead of trying to fix the way styles are applied, we change the way we generate the output. That's the V2 way in fact.

The new patch propose this different approach. It's a quite simple change.

[Garry Yao]

The new patch is not passing the following two TCs:

1. 
Source: <div><br /><p>Test</p></div>
Expected: Unchanged
Actual: <div><p><br />Test</p></div>
Reason: Pending BR is not send on start of blocks.
2.
Source: <p><i><b><br />Test</b></i></p>
Expected: Unchanged
Actual: <p><br /><i><b>Test</b></i></p>
Reason: Pending BRs are isolated from other pending inline elements.

Considering the variability of this approach, it would be great to see some TCs along with the next patch.

[Frederico Caldeira Knabben]

Considering that it's quite difficult to have the case 2 working, and that case 1 is instead a good thing to have fixed that way (avoiding mixing inline and block elements), I would ask to please accept the actual results as good. We may work on them later if required.

[Frederico Caldeira Knabben]

Investigating...

[Frederico Caldeira Knabben]

After some investigation, it looks like the last patch is the best approach for now.

[Garry Yao]

Please open new ticket for the second case after this one get fixed.
You can safely close #4933 at the same time.

[Frederico Caldeira Knabben]

Fixed with [5103].