Opened 8 years ago

Closed 7 years ago

#4886 closed Bug (fixed)

Extra line break inside of created links

Reported by: Wiktor Walc Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.2
Component: General Version: 3.0
Keywords: IBM Confirmed Review+ Cc: jkavanag@…

Description

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>
    

Attachments (3)

4886.patch (2.2 KB) - added by Garry Yao 8 years ago.
4886_2.patch (912 bytes) - added by Frederico Caldeira Knabben 7 years ago.
4886_3.patch (2.3 KB) - added by Frederico Caldeira Knabben 7 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.3

Changed 8 years ago by Garry Yao

Attachment: 4886.patch added

comment:2 Changed 8 years ago by Garry Yao

Milestone: CKEditor 3.3CKEditor 3.1
Owner: set to Garry Yao
Status: newassigned

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.

comment:3 Changed 8 years ago by Garry Yao

Component: GeneralCore : Styles
Keywords: Review? added

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

comment:4 Changed 8 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.1CKEditor 3.2

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.

comment:5 Changed 8 years ago by Joe Kavanagh

Cc: jkavanag@… added
Component: Core : StylesGeneral
Keywords: IBM added

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

comment:6 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

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.

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

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

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

comment:10 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

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

comment:11 Changed 7 years ago by Frederico Caldeira Knabben

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

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.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 4886_2.patch added

comment:12 Changed 7 years ago by Frederico Caldeira Knabben

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

comment:13 Changed 7 years ago by Garry Yao

Keywords: Review- added; Review? removed

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.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 4886_3.patch added

comment:14 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

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.

comment:15 Changed 7 years ago by Garry Yao

Keywords: Review- added; Review? removed

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.

comment:16 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed

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.

comment:17 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review? removed

Investigating...

comment:18 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review? added

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

comment:19 Changed 7 years ago by Garry Yao

Keywords: Review+ added; Review? removed

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

comment:20 Changed 7 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [5103].

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