Ticket #117 (closed Bug: fixed)

Opened 8 years ago

Last modified 8 years ago

FCK.IsDirty() fails in Source view

Reported by: alfonsoml Owned by: alfonsoml
Priority: Normal Milestone:
Component: General Version: FCKeditor 2.4
Keywords: HasPatch Cc:

Description

The check is run against EditorDocument so now that the object is destroyed it raises an error, but previously it did return a wrong state while the user changed the source in the textarea.

	IsDirty : function()
	{
		return ( this.StartupValue != this.EditorDocument.body.innerHTML ) ;
	},

	ResetIsDirty : function()
	{
		if ( this.EditorDocument.body )
			this.StartupValue = this.EditorDocument.body.innerHTML ;
	},

Attachments

isdirty.patch (2.7 KB) - added by fredck 8 years ago.

Change History

comment:1 follow-up: ↓ 3 Changed 8 years ago by alfonsoml

I'm trying to think how to make it work properly but it doesn't seem easy.

The check gets the innerHTML of the body (that can be different to the provided source and to the HTML that it's output, just due to browser parsing and the HTML beautification)

So if we try to compare this.StartupValue (set while in WYSIWYG) and the textarea value it will fail most of the times although the user hasn't changed anything.

Checking the value of FCK.GetXHTML might give a better check, but it will be more expensive and FCKXHtml.GetXHTML does a call itself to FCK.IsDirty, so it has to be protected to avoid an infinite loop.

It's not possible to update this.StartupValue when the user switch to source mode because then the IsDirty check wouldn't be run against the original HTML, but against the HTML that existed in the moment the user switched modes.

Maybe what we can do is to add another reference value: save the cleaned HTML source and depending on the editing mode do the check against one or another.

Pseudocode to explain this idea:

StartupValue : '',
HtmlStartupValue : '',

IsDirty : function()
{
	if ( FCK.EditMode == FCK_EDITMODE_SOURCE )
		return ( this.HtmlStartupValue != FCK.EditingArea.Textarea.value ) ;
	else
		return ( this.StartupValue != this.EditorDocument.body.innerHTML ) ;
},

ResetIsDirty : function()
{
	if ( FCK.EditMode == FCK_EDITMODE_SOURCE )
	{
		if ( this.EditorDocument.body )
		{
			this.StartupValue = this.EditorDocument.body.innerHTML ;
			this.HtmlStartupValue = this.GetXHTML() ;
		}
	}
	else
	{
		this.StartupValue =  What can we put here ???
		this.HtmlStartupValue != FCK.EditingArea.Textarea.value ;
	}
},

This can take care of returning the right IsDirty() value in both modes, but only if ResetIsDirty is called while in WYSIWYG. If it's called while in source we can get the HtmlStartupValue from the textarea value, but how do we get the StartupValue?

comment:2 Changed 8 years ago by alfonsoml

Sorry, the last pseudocode is obviously wrong, I copy-pasted without changing the !=

StartupValue : '',
HtmlStartupValue : '',

IsDirty : function()
{
	if ( FCK.EditMode == FCK_EDITMODE_SOURCE )
		return ( this.HtmlStartupValue != FCK.EditingArea.Textarea.value ) ;
	else
		return ( this.StartupValue != this.EditorDocument.body.innerHTML ) ;
},

ResetIsDirty : function()
{
	if ( FCK.EditMode == FCK_EDITMODE_SOURCE )
	{
		if ( this.EditorDocument.body )
		{
			this.StartupValue = this.EditorDocument.body.innerHTML ;
			this.HtmlStartupValue = this.GetXHTML() ;
		}
	}
	else
	{
		this.StartupValue =  What can we put here ???
		this.HtmlStartupValue = FCK.EditingArea.Textarea.value ;
	}
},

comment:3 in reply to: ↑ 1 Changed 8 years ago by fredck

  • Keywords HasPatch added
  • Version set to FCKeditor 2.4

Replying to alfonsoml:

The check gets the innerHTML of the body (that can be different to the provided source and to the HTML that it's output, just due to browser parsing and the HTML beautification)

So if we try to compare this.StartupValue (set while in WYSIWYG) and the textarea value it will fail most of the times although the user hasn't changed anything.

You are right. This is why ResetIsDirty is called right after the HTML loading on startup.

The problem is that we do that only on startup, not when switching mode.

Checking the value of FCK.GetXHTML might give a better check, but it will be more expensive and FCKXHtml.GetXHTML does a call itself to FCK.IsDirty, so it has to be protected to avoid an infinite loop.

You are right. The solution is not here.

It's not possible to update this.StartupValue when the user switch to source mode because then the IsDirty check wouldn't be run against the original HTML, but against the HTML that existed in the moment the user switched modes.

You are right, and here we have the base concept of the IsDirty feature to help us.

As you yourself have noted, if we just get the input, and compare it with the innerHTML, it will always be dirty. But developers looking to use the IsDirty want to be "notified" by changes made by "the user" to the document. They don't really expect the editor to make changes to it by itself. So, IsDirty is there to signalize a user action in the editor, not really a difference from the input and the output.

So, if a user opens a document, switch to source ten times and check IsDirty, the editor must say that anything has been done. If the user then changes a single space in the text (or in the source), then it will be dirty.

This is why it would be ok to ResetIsDirty when switching mode, much like we do in FCKXHtml.GetXHTML, making the fix quite simple.

Well... not that simple. The problem is that the EditorDocument is not available right after calls to SetHTML. So, a asynchronous solution must be found.

I'm attaching a proposal patch for it. It also adds a "resetIsDirty" param to SetHTML, making the previous start only code more generic, and making the asynchronous magic work.

Let me know your thoughts about it, Alfonso.

Changed 8 years ago by fredck

comment:4 Changed 8 years ago by anonymous

The only problem that I can think of that solution is that if the user does some change, then switch modes and undo it, the IsDirty will remain true all the way, but I didn't found any better solution.

So I think that until someone comes out with something better we must include it as it is completly broken as soon as the user switches to Source mode.

comment:5 Changed 8 years ago by fredck

Ok, the patch solves the ticket problem. I think we should proceed with it.

I believe the undo combination could happen on very few cases, and actually an IsDirty inaccuracy is not a big issue on most sites that use it. In this way we at least reduce the problem occurrence to very few (unfortunate) cases.

Then we can open a dedicated ticket to the undo issue, which I already have a few ideas about (saving the IsDirty state with the Undo snapshot). I'm just worried about the performance of it.

comment:6 Changed 8 years ago by alfonsoml

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

I've tested that patch and it seems to work OK, so I've merged it to trunk in [185] and have filed #167 so we can remember about the little problem after switching modes.

Thanks for the patch.

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