Opened 14 years ago
Closed 14 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)
Change History (23)
comment:1 Changed 14 years ago by
Milestone: | → CKEditor 3.3 |
---|
Changed 14 years ago by
Attachment: | 4886.patch added |
---|
comment:2 Changed 14 years ago by
Milestone: | CKEditor 3.3 → CKEditor 3.1 |
---|---|
Owner: | set to Garry Yao |
Status: | new → assigned |
comment:3 Changed 14 years ago by
Component: | General → Core : Styles |
---|---|
Keywords: | Review? added |
Ticket Test added at :
http://ckeditor.t/tt/4886/1.html.
comment:4 Changed 14 years ago by
Milestone: | CKEditor 3.1 → 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 14 years ago by
Cc: | jkavanag@… added |
---|---|
Component: | Core : Styles → General |
Keywords: | IBM added |
[FF] Line breaks are also getting added to paragraphs.
- Open any sample page, click 'New Page' to clear up content.
- Type one word, 'text', then press Enter.
- Repeat step 2 above two more times to create three paragraphs in total.
- On the second paragraph select 'text' by double clicking it, and delete it.
- Press Enter, then press Backspace.
- 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 14 years ago by
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 Changed 14 years ago by
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 14 years ago by
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 Changed 14 years ago by
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 14 years ago by
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 14 years ago by
Keywords: | Review- added; Review? removed |
---|---|
Owner: | changed from Garry Yao to Frederico Caldeira Knabben |
Status: | assigned → new |
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 14 years ago by
Attachment: | 4886_2.patch added |
---|
comment:12 Changed 14 years ago by
Keywords: | Review? added; Review- removed |
---|---|
Status: | new → assigned |
comment:13 Changed 14 years ago by
Keywords: | Review- added; Review? removed |
---|
4886_2.patch has brought the following regression:
Reproducing Procedures
- Open any sample page, load the editor with the following source and selection:
[line1<br/><br />]
- Apply bold style and move cursor to the second empty line;
- Start typing.
- Expected Result: The inserted text should be bold styled.
Changed 14 years ago by
Attachment: | 4886_3.patch added |
---|
comment:14 Changed 14 years ago by
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 14 years ago by
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 14 years ago by
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:18 Changed 14 years ago by
Keywords: | Review? added |
---|
After some investigation, it looks like the last patch is the best approach for now.
comment:19 Changed 14 years ago by
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.
Re-target it to 3.1 since 'Link' command isn't the only victim of this bug, one more serious symptom below:
Reproducing Procedures