Opened 11 years ago

Closed 11 years ago

#1384 closed Bug (fixed)

PATCH: FCKDomRange CheckStartOfBlock()

Reported by: Scott McNaught Owned by:
Priority: Normal Milestone:
Component: General Version: SVN (FCKeditor) - Retired
Keywords: Cc:

Description

I was playing around with FCKDomRange today and I have found some problems with it. I have altered it to work as expected and attached a patch. There are 4 things that this patch changes in FCKDomRange.

1) The CheckStartOfBlock() method does not factor in opening inline tags when the cursor is at the start. Eg:

<p><strong>Some text</strong></p>
          ^

oRange.CheckStartOfBlock() returns FALSE. This is incorrect behaviour when CheckEndofBlock() does check for empty inline tags.

2) Because the two methods use the same code to determine whether the selection has contents, I have moved the checking code into CheckIsEmpty(). I have added a parameter to this function: bCheckEmptyInline.

When this parameter is set to true, it will check for empty inline elements such as <strong></strong>

This also has the added benefit of providing the additional functionality to the CheckIsEmpty() function.

3) The existing code for checking to see whether the selection contents was buggy. The code does not detect a series of sibling empty inline elements such as: <strong></strong><u><strong></strong></u>

I have created an inline recursive function which will check to see if all the children of an element are inline. This function is called: AreAllChildrenInline

4) I have made CheckStartOfBlock and CheckEndOfBlock have the same method signatures, and execute the same code (except reversing the selection direction).


This patch will potentially impact: Anything that calls "CheckStartOfBlock", and potentially some things which rely on the existing detection of inline elements.

If anything though, I think that this patch will only make the code more stable, and fix some edge cases where there is messed up HTML with wierd formatting elements at the start and ends of blocks.

Please consider applying this patch.

Thanks,

Scott McNaught Synergy 8

Attachments (1)

fckdomrange.patch (4.1 KB) - added by Scott McNaught 11 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 11 years ago by Scott McNaught

Ignore this patch for the moment - it seems I have an old copy and its comparing against some of the newer changes.

Changed 11 years ago by Scott McNaught

Attachment: fckdomrange.patch added

comment:2 Changed 11 years ago by Scott McNaught

Patch updated - please have a look now as it is a valid diff to the head revision.

comment:3 Changed 11 years ago by Frederico Caldeira Knabben

Is this patch fixing any issue in the editor, or its a personal issue you are facing there?

comment:4 Changed 11 years ago by Frederico Caldeira Knabben

Milestone: FCKeditor 2.5

comment:5 Changed 11 years ago by Scott McNaught

A bit of both. The first:

A personal issue (I have been making some changes to the backspace behaviour specific to our software), however I think it is best to update your code as it makes it conform to the same signature and implementation of CheckEndOfBlock.

I can see other wierd problems arising in the editor later on if this is left.

The second: I had strange behaviour occuring earlier when I had code that looked similar to <p>Text<u><strong></strong></u><strong></strong></p>. Ie - two inline empty tags next to each other, when calling CheckEndOfBlock.

I can't exactly remember what I was doing, but it threw me into visual studio and I checked the div's innerHTML and it looked similar to the text above. This happened days ago and I off-hand ignored it because I was messing around with my own stuff at the time. I might suggest doing some testing on series of sibling empty inline nodes though.

I am not asking you to commit this patch straight away. I would just like you to give it some consideration as it could potentially make things more stable in the future and makes the API logical.

comment:6 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Pending added

With [1048] (fixing #1435), I've reviewed the logic for both the CheckStarOfBlock and CheckEndOfBlock. The previous implementation sounded a bit like a hack for me, so I've decided to make it more logic and predictable.

It should fix most of the issues you have pointed here. Do you see any problem with it?

comment:7 Changed 11 years ago by Scott McNaught

This appears to have fixed all of the major issues that I was having with these functions.

I have however noted a wierd enter bug in IE since this this was added. I am going to keep my eye out for what causes it. But basically when I was pressing enter at the end of a heading 1, I got in a state where by enter key presses were creating empty <h1> tags. IE - there was no &nbsp; inside it. So visually it did not look like anything was happening when enter was pressed as the cursor did not move down to the next line.

When I see this happen again, I will file a proper ticket for it.

Thank you for fixing the check start / end of block functions. They seem to function a little faster too which is always good.

comment:8 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Pending removed
Resolution: fixed
Status: newclosed

Thanks for your comments Scott. I've added a sanity check with [1055], since it thrown an error for me too. I really hope it will fix your finding too.

We are approaching the release of the 2.5 stable, so hopefully it hasn't introduced critical issues.

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