Opened 9 years ago

Closed 8 years ago

#1491 closed Bug (fixed)

Error when indenting and outdenting empty list elements

Reported by: jonhg Owned by: martinkou
Priority: Normal Milestone: FCKeditor 2.5
Component: General Version: FCKeditor 2.5 Beta
Keywords: Confirmed IE Cc:

Description

Scenario:
1. Go to sample01 nightly build and clear content
2. Make a list with two elements
3. Go to the end of the first element and press enter to make a new empty list element between the other two.
4. Press indent button.
5. Press outdent button.
6. The list is now completely broken.

Tested with IE7
It seems to have something to do with the end of line charachter beeing selected after indentation. This makes a lot of unexpected results in other cases too.

Change History (25)

comment:1 Changed 9 years ago by martinkou

  • Keywords Confirmed IE added
  • Milestone set to FCKeditor 2.5
  • Owner set to martinkou
  • Status changed from new to assigned
  • Version set to FCKeditor 2.5 Beta

comment:2 Changed 9 years ago by jonhg

Other related strange behaviours:

At step 5:

  1. Press numbered list instead of bullet list causes the last element to change from bullet list to numbered list.
  1. Press enter - a new element are made on root level and the cursor is moved to the last element.
  1. Write some text - the list element are deleted and the text are added to the first element.

comment:3 Changed 9 years ago by martinkou

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

Fixed with [1064].

Click here for more info about our SVN system.

comment:4 Changed 9 years ago by jonhg

  • Resolution fixed deleted
  • Status changed from closed to reopened

This bug is not completely crushed.

Instead of pressing outdent button in step 5. Try to write some text. You will see that this text is added to the list element above the current element. The current element seems to be deleted.

As I see it the problem is that the indented list element is selected after indentation.

comment:5 Changed 9 years ago by jonhg

Another related scenario:
1. Empty content in sample01.html
2. Add two list elements with text
3. Add new list element and indent it, write text without clearing selection.
4. Add new element to the sublist. Try to indent it - result: javascript error: 'indent' is null or not an object.

If I deselect the selection before writing text in step 3 this error does not accur.

comment:6 Changed 9 years ago by jonhg

After a lot of testing it seems like the problem is located at line 74 in fckindentcommands.js where FCK.Focus() is called after the list element creation. When I comment out this line the new list element is no longer selected and everything seems to be working..

I don't know the reason this method is called, but maybe you could just drop this call?

comment:7 Changed 9 years ago by jonhg

To be more specific - it seems to be inside FCKEditingArea._EnsureFocusIE();

comment:8 Changed 9 years ago by jonhg

The problem is the kludge code added for #141 with [621] / [623]. This code should ensure correct focus in IE and that the cursor should be inside the first element and not between body and the first child element.

My testing show that this kludge does not work anymore, the cursor is now always placed before the first child element on load (very annoying) and just causes a lot of trouble with lists.

comment:9 Changed 9 years ago by martinkou

I've fixed the list item selection problem in [1082].

I couldn't get the cursor to appear like |<p>&nbsp;<p> as you said in #141 though. How did you trigger the bugged behavior? I've tried setting StartupFocus = true and tested with sample01.html with a few different default values (e.g. oFCKeditor.Value = '<p>&nbsp;</p>'). I haven't seen the caret appearing before the first block element.

comment:10 Changed 9 years ago by jonhg

First of all: I'm using IE6.

When I try setting startupFocus = true in sample01.html I still get the same problem as described before. The cursor is placed outside the paragraph. This causes several unwanted behaviours. I.e. When I start to write text, the existing paragraph jumps down a line.

When I debug the content of the editor with IE developer toolbar I can see that the text is placed directly into the body element. This is of course fixed after a enter press or when switching editing mode.

comment:11 Changed 9 years ago by martinkou

I can't reproduce the problem on my local machine anyway but I've got some friends who told me he could occasionally see it happen. Tracing the logic a bit shows that the kludge in _EnsureFocusIE() isn't executed when FCKeditor was loaded with StartupFocus = true, and I guess that's the reason you're getting the bug.

The kludge in _EnsureFocusIE() isn't running because of the changes in [799]... I've changed the kludge's logic in [1084] such that:

  1. It won't select the "end of line" character in IE anymore, and
  2. It will be executed at startup when StartupFocus=true.

This should fix the bug you've encountered while not breaking previously fixed bugs (e.g. #1178). Since I can't reproduce your bug in the first place, could you tell me if this fix works on your side?

comment:12 Changed 9 years ago by jonhg

I've copied the changes you made in fckeditingarea.js to my local version and everything seems to be working fine now! The list isn't selected and the caret is moved inside the first paragraph on startup.

Good work!

btw, I guess you can remove the oldlength variable at line 313 which is not in use anymore..

comment:13 Changed 9 years ago by martinkou

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

Oh yes, good observation, I missed that one. Fixed that with [1087]. :)

Closing ticket.

comment:14 Changed 9 years ago by fredck

  • Resolution fixed deleted
  • Status changed from closed to reopened

It seems like [1064] is not anymore needed for this ticket. Am I right? Should we revert it? Can you provide a TC that shows that it's needed?

comment:15 Changed 9 years ago by fredck

Also, it it is supposed to be used, there is a small improvement to it:

	if ( /^\s*<LI><\/LI>$/.test( htmlText ) )

comment:16 Changed 9 years ago by martinkou

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

You're right, [1064] is no longer needed. It was intended only for fixing the issue originally reported by jonhg, and [1084] is a better solution for it.

I've reverted the unnecessary changes in [1118].

comment:17 Changed 8 years ago by fredck

  • Resolution fixed deleted
  • Status changed from closed to reopened

To fix #1587, I had to partially revert [1084]. I've also changed part of its logic, to be sure it will not interfere in this ticket.

Can martin and johng confirm it is still working properly after my change?

comment:18 Changed 8 years ago by martinkou

I've tested the updated code again with johng's test cases and I didn't find any problem so far.

comment:19 Changed 8 years ago by martinkou

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

comment:20 follow-up: Changed 8 years ago by jonhg

  • Resolution fixed deleted
  • Status changed from closed to reopened

This revert causes the bug with startupfocus to reappear in my testcase. When writing text directly after loading the editor the text is placed under the body and not inside a paragraph.

comment:21 follow-up: Changed 8 years ago by jonhg

As I see it the problem is the test 'parentNode.childNodes.length > 0' this will always return true if the parentNode contains text, both #1587 and this ticket seems to be working ok if I alter the test to 'parentNode.childNodes.length > 1'.

You can maybe make a more elegant fix...

comment:22 in reply to: ↑ 21 ; follow-up: Changed 8 years ago by martinkou

Replying to jonhg:

As I see it the problem is the test 'parentNode.childNodes.length > 0' this will always return true if the parentNode contains text, both #1587 and this ticket seems to be working ok if I alter the test to 'parentNode.childNodes.length > 1'.

You can maybe make a more elegant fix...

I've added an extra condition check in [1136] to hopefully fix the problem. Can you confirm whether this fix works or not on your side?

comment:23 in reply to: ↑ 20 Changed 8 years ago by fredck

Replying to jonhg:

This revert causes the bug with startupfocus to reappear in my testcase. When writing text directly after loading the editor the text is placed under the body and not inside a paragraph.

It would be nice to have a TC for it, because we are not able to consistently reproduce it here.

comment:24 in reply to: ↑ 22 Changed 8 years ago by jonhg

Replying to martinkou:

Replying to jonhg:

As I see it the problem is the test 'parentNode.childNodes.length > 0' this will always return true if the parentNode contains text, both #1587 and this ticket seems to be working ok if I alter the test to 'parentNode.childNodes.length > 1'.

You can maybe make a more elegant fix...

I've added an extra condition check in [1136] to hopefully fix the problem. Can you confirm whether this fix works or not on your side?

When debugging I have found thath the parentTag is P so it would not help to check if the parent tag is body, but that was my first thought too. It seems to be some weird IE problem, the range is inside the P, but moved outside when writing. Checking for childNodes.length > 1 does not break bold and places the cursor inside the paragraph as expected.

I can try to make a TC tomorrow, in what way do you want it?

comment:25 Changed 8 years ago by fredck

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

I've reverted [1136] with [1137].

johng, I'm closing this ticket because the issue you are having has nothing to do with it. Not even #141 seems to be related as we are simply talking about the focus there.

So, I would ask you to open a dedicated ticket for it, providing a TC so we can reproduce it here. A TC would be a simple HTML file with an editor instance on it (like sample01.html) which consistently shows the problem. This is needed because there could be even external factors interfering in the editor... a classic IE joke with us.

It is important to have in mind that _EnsureFocusIE is there to "ensure the focus", not to place the caret inside the paragraph. Of course it is ok to use it to solve both problems, but we must remember that it is also there for other purposes.

Note: tomorrow is the release date for the 2.5.

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