Opened 10 years ago

Closed 10 years ago

#1522 closed Bug (fixed)

Can't see the new line created when pressing enter in front of paragraph with inline formatting

Reported by: Jon Håvard Gundersen Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: FCKeditor 2.6
Component: General Version:
Keywords: Review+ Cc:

Description


Scenario:
1. Go to sample01.html on nightly build and clear content
2. Write some text and press enter to make it a paragraph
3. Select the text and apply bold
4. Move cursor in front of the text and press enter

Expected the current line to move down when pressing enter, but it is not moved. It seems like the new paragrahs is not inserted as they should be, but when you switch edit mode they are inserted above the current line.

Attachments (2)

1522.patch (1.8 KB) - added by Martin Kou 10 years ago.
Proposed fix for #1522 using the local approach.
1522_2.patch (2.6 KB) - added by Frederico Caldeira Knabben 10 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Pending WorksForMe added

I'm not able to reproduce it. Worked for me with both IE and FF.

Do you have more info on how to reproduce it?

comment:2 Changed 10 years ago by Jon Håvard Gundersen

Ok, I'm not able to reproduce on nightly I either after I cleared the cache. Maybe I had some cache problem, or maybe some conflicts with my local version.

You can close this bug, seems like I have to crush this one myself :)

comment:3 Changed 10 years ago by Jon Håvard Gundersen

Strange though, if I has produced the bug in my local version of fckeditor and then go to the nightly build the bug appears both places. If I clear the cache and then load the nigtly build sample the bug is not there.

Are these two version of the editor sharing some code?

I'm using IE6.

comment:4 Changed 10 years ago by Jon Håvard Gundersen

Finally I've reproduced it in a clean nightly build :)

Between step 3 and 4 you have to switch edit mode to source mode and back again. Then press enter.

comment:5 Changed 10 years ago by Martin Kou

Keywords: Confirmed IE added; Pending WorksForMe removed
Owner: set to Martin Kou
Status: newassigned

Confirmed on IE, ok on Firefox, Safari and Opera.

comment:6 Changed 10 years ago by Martin Kou

Milestone: FCKeditor 2.6

comment:7 Changed 10 years ago by Martin Kou

The bug occurs because the logic in FCKDomRange.CheckIsCollapsed() models whether a selection is empty instead of whether it is visually collapsed. The problem encountered in the ticket is that it is possible for a selection to be visually collapsed while not empty.

The bug occurs like this:

  1. FCKEnterKey._ExecuteEnterBlock() is called.
  2. FCKEnterKey._ExecuteEnterBlock() creates an empty new paragraph block.
  3. FCKEnterKey._ExecuteEnterBlock() duplicates a <B></B> tag to the empty new paragraph.
  4. FCKEnterKey._ExecuteEnterBlock() inserts the new paragraph before the current paragraph.
  5. FCKDomRange.Select() is called to select the new paragraph's contents (i.e. "<B></B>").
  6. FCKDomRange.SelectBookmark() in fckdomrange_ie.js is called.
  7. FCKDomRange.SelectBookmark() thinks the current selection isn't collapsed because it contains "<B></B>", and thus it does not execute the dummy span kludge to expand the paragraph block.
  8. Since the empty paragraph block is not expanded upon creation, it remains invisible after it is created.

There are two ways to fix this bug: either fix CheckIsCollapsed() and change the kludge logic in fckdomrange_ie.js or just fix the bug locally in fckdomrange_ie.js. Either way, the kludge logic needs to be changed. I'm attaching the simpler fix (i.e. the latter one) as a patch for review as I'm not perfectly sure what are the other consequences if CheckIsCollasped() is changed.

Changed 10 years ago by Martin Kou

Attachment: 1522.patch added

Proposed fix for #1522 using the local approach.

comment:8 Changed 10 years ago by Martin Kou

Keywords: Review? added; Confirmed IE removed

comment:9 Changed 10 years ago by Jon Håvard Gundersen

I've tested the patch here and it seems to be working. But I do not know if it effects other functionality.

comment:10 Changed 10 years ago by Martin Kou

#1723 is marked as a dup to this bug.

Changed 10 years ago by Frederico Caldeira Knabben

Attachment: 1522_2.patch added

comment:11 Changed 10 years ago by Frederico Caldeira Knabben

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

Martin, your analysis was perfect, and the culprit was there, right in front of you.

Changing CheckIsCollapsed() would be wrong, because if a "range" contains <b></b>, it is not collapsed. A "selection" would be instead collapsed in this case, but again, it important to understand that selections are not ranges.

Your fix works for me, almost. In the way it is done, it deletes the <b></b>, so the text will not be bold when moving to the empty paragraph (an IE only feature, btw).

Also, I'm always worried when we talk about changes in the IE selection code. It is quite delicate, and we have several things depending on it, so I prefer to not touch on it if it's not really needed.

The thing we could fix instead is point 5 in your comment. We could make it select a collapsed range inside <b></b>, instead of a range containing <b></b>. To do that it is enough to change the MoveToNodeContents call to MoveToElementEditStart.

I've attached a patch for it.

comment:12 Changed 10 years ago by Frederico Caldeira Knabben

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

comment:13 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review+ removed

Sorry... wrong keyword used before.

comment:14 Changed 10 years ago by Mayank Parmar

Well 1522.patch nor 1522_2.patch worked for my code that I paste in source http://dev.fckeditor.net/ticket/1723

Even I tried both patch simultaneously but didn't worked.

comment:15 in reply to:  14 ; Changed 10 years ago by Martin Kou

Replying to mayank1711:

Well 1522.patch nor 1522_2.patch worked for my code that I paste in source http://dev.fckeditor.net/ticket/1723

Even I tried both patch simultaneously but didn't worked.

1522_2.patch works for me for both #1522 and #1723. I used TortoiseSVN to apply the patch to the SVN trunk copy. Note that applying the patch to the release package has no effect, the patch is useful only for the SVN source.

comment:16 in reply to:  15 Changed 10 years ago by Mayank Parmar

Replying to martinkou:

Replying to mayank1711:

Well 1522.patch nor 1522_2.patch worked for my code that I paste in source http://dev.fckeditor.net/ticket/1723

Even I tried both patch simultaneously but didn't worked.

1522_2.patch works for me for both #1522 and #1723. I used TortoiseSVN to apply the patch to the SVN trunk copy. Note that applying the patch to the release package has no effect, the patch is useful only for the SVN source.

Can you plese tell me from where can I get SVN source?

comment:17 Changed 10 years ago by Martin Kou

You'll need an SVN client to down the SVN source. Some SVN clients (like TortoiseSVN) will allow you to apply patches too.

After you've installed an SVN client, do an SVN checkout from https://svn.fckeditor.net/FCKeditor/trunk to retrieve the newest SVN code.

After that, apply the patch to your local copy of the SVN trunk. For TortoiseSVN, you can use the "Apply Patch" option in the Explorer right click menu when you're in the trunk directory.

You can then test whether the patch is working correctly by opening _samples/default.html in the trunk directory in a browser.

comment:18 in reply to:  17 Changed 10 years ago by Mayank Parmar

Replying to martinkou:

You'll need an SVN client to down the SVN source. Some SVN clients (like TortoiseSVN) will allow you to apply patches too.

After you've installed an SVN client, do an SVN checkout from https://svn.fckeditor.net/FCKeditor/trunk to retrieve the newest SVN code.

After that, apply the patch to your local copy of the SVN trunk. For TortoiseSVN, you can use the "Apply Patch" option in the Explorer right click menu when you're in the trunk directory.

You can then test whether the patch is working correctly by opening _samples/default.html in the trunk directory in a browser.

Well it works with SVN but just post here when this patch is integrated in any new version.

comment:19 Changed 10 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:20 Changed 10 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [1284].

Btw, check out the SVN Basics page for some help on basic SVN tasks.

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