Opened 10 years ago

Closed 10 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 10 years ago.
4886_2.patch (912 bytes) - added by Frederico Caldeira Knabben 10 years ago.
4886_3.patch (2.3 KB) - added by Frederico Caldeira Knabben 10 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 10 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.3

Changed 10 years ago by Garry Yao

Attachment: 4886.patch added

comment:2 Changed 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 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 10 years ago by Frederico Caldeira Knabben

Attachment: 4886_2.patch added

comment:12 Changed 10 years ago by Frederico Caldeira Knabben

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

comment:13 Changed 10 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 10 years ago by Frederico Caldeira Knabben

Attachment: 4886_3.patch added

comment:14 Changed 10 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 10 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 10 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 10 years ago by Frederico Caldeira Knabben

Keywords: Review? removed

Investigating...

comment:18 Changed 10 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 10 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 10 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [5103].

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