Opened 8 years ago

Closed 8 years ago

#8007 closed Bug (fixed)

onBeforeUnload event is fired in IE by switchin modes

Reported by: Alfonso Martínez de Lizarrondo Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.1
Component: General Version: 3.6.1
Keywords: Cc:

Description

Modify the replacebyclass example adding this code:

	<script type="text/javascript">

		window.onbeforeunload = function() {
			if (typeof(CKEDITOR)!='undefined')
			{
				var oEditor = CKEDITOR.instances.editor1 ;
				if (oEditor.checkDirty())
					return "The text has been modified, do you want to save it before closing?.";
			}
			return undefined;
		}

	</script>

Now load the page and switch to source and back to design.

Expected: nothing special.

Results: the onBeforeUnload is fired on each change, and the checkDirty() returns true (#6274)

Tested with IE8. Firefox works fine, only shows the alert if the content has been modified and you want to close the page.

Attachments (1)

8007.patch (6.0 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Correction: this happens with almost any toolbar button.

The problems started with [6971]

This is a critical bug for anyone that has set a onBeforeUnload handler and is using IE, I suggest to nominate it for 3.6.1

comment:2 Changed 8 years ago by Wiktor Walc

Milestone: CKEditor 3.6.1
Status: newconfirmed

Nice catch

Changed 8 years ago by Garry Yao

Attachment: 8007.patch added

comment:3 Changed 8 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

This ticket just reveal some evilness of [6971], so re-propose a fix for #188 as I tried to do on that ticket.

comment:4 Changed 8 years ago by Garry Yao

I don't think unselectable should be applied recursively, the purpose of the selectable call is to prevent text selection from starting on the specified element, and a single attribute turns out to be enough unless you cite me an evidence that I'm wrong.

comment:5 in reply to:  3 Changed 8 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Replying to garry.yao:

This ticket just reveal some evilness of [6971], so re-propose a fix for #188 as I tried to do on that ticket.

The way you put seem sounds like this ticket was logically predictable, which was not (there is no logic for it). In any case the proposed patch already have R- on #188, so there is no reason to have it differently now.

Replying to garry.yao:

I don't think unselectable should be applied recursively, the purpose of the selectable call is to prevent text selection from starting on the specified element, and a single attribute turns out to be enough unless you cite me an evidence that I'm wrong.

That thing is there since I can remember it. There is no correlation between that change and the fix. If we have that code, I'm sure there is a good reason for it. We'll not spend time on looking for the reasons to justify changing stable code.

To conclude, the right way to close this ticket is simply reverting [6971] and reopening #188. I'll do that now.

comment:6 Changed 8 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_failedclosed

Reverted [6971] with [7027].

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