Ticket #4886 (closed Bug: fixed)

Opened 5 years ago

Last modified 5 years ago

Extra line break inside of created links

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

4886.patch (2.2 KB) - added by garry.yao 5 years ago.
4886_2.patch (912 bytes) - added by fredck 5 years ago.
4886_3.patch (2.3 KB) - added by fredck 5 years ago.

Change History

comment:1 Changed 5 years ago by fredck

  • Milestone set to CKEditor 3.3

Changed 5 years ago by garry.yao

comment:2 Changed 5 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from new to assigned
  • Milestone changed from CKEditor 3.3 to CKEditor 3.1

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

  • Keywords Review? added
  • Component changed from General to Core : Styles

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

comment:4 Changed 5 years ago by fredck

  • Milestone changed from CKEditor 3.1 to CKEditor 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 5 years ago by JoeK

  • Cc jkavanag@… added
  • Keywords IBM added
  • Component changed from Core : Styles to General

[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 follow-up: ↓ 7 Changed 5 years ago by alfonsoml

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

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 follow-up: ↓ 9 Changed 5 years ago by alfonsoml

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

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

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

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

comment:12 Changed 5 years ago by fredck

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

comment:13 Changed 5 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 5 years ago by fredck

comment:14 Changed 5 years ago by fredck

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

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

  • Keywords Review? removed

Investigating...

comment:18 Changed 5 years ago by fredck

  • Keywords Review? added

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

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

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

Fixed with [5103].

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