Opened 9 years ago

Closed 8 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: jonhg Owned by: fredck
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 martinkou 9 years ago.
Proposed fix for #1522 using the local approach.
1522_2.patch (2.6 KB) - added by fredck 8 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 9 years ago by fredck

  • 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 9 years ago by jonhg

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 9 years ago by jonhg

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 9 years ago by jonhg

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 9 years ago by martinkou

  • Keywords Confirmed IE added; Pending WorksForMe removed
  • Owner set to martinkou
  • Status changed from new to assigned

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

comment:6 Changed 9 years ago by martinkou

  • Milestone set to FCKeditor 2.6

comment:7 Changed 9 years ago by martinkou

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 9 years ago by martinkou

Proposed fix for #1522 using the local approach.

comment:8 Changed 9 years ago by martinkou

  • Keywords Review? added; Confirmed IE removed

comment:9 Changed 9 years ago by jonhg

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 8 years ago by martinkou

#1723 is marked as a dup to this bug.

Changed 8 years ago by fredck

comment:11 Changed 8 years ago by fredck

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

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

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

comment:13 Changed 8 years ago by fredck

  • Keywords Review? added; Review+ removed

Sorry... wrong keyword used before.

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

comment:15 in reply to: ↑ 14 ; follow-up: Changed 8 years ago by 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.

comment:16 in reply to: ↑ 15 Changed 8 years ago by mayank1711

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

comment:18 in reply to: ↑ 17 Changed 8 years ago by mayank1711

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 8 years ago by martinkou

  • Keywords Review+ added; Review? removed

comment:20 Changed 8 years ago by fredck

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

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 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy