Bug Tracker

Ticket #6593 (closed bug: fixed)

Opened 4 years ago

Last modified 3 years ago

IE8: DOM 0 event handler called twice when a separate handler is attached via jQuery

Reported by: Pointy Owned by: dmethvin
Priority: high Milestone: 1.7
Component: event Version: 1.4.4
Keywords: events IE8 Cc:
Blocking: Blocked by:

Description

If a <select> element is coded with its own "onchange" attribute, and is also referenced by a jQuery expression to bind "change" to some other code, then the handler bound via the "onchange" attribute is called twice: once before the jQuery-bound handler, and once afterwards.

This happens in IE8 but not Firefox or Chrome.

Example code:  http://gutfullofbeer.net/onchange.html

Change History

comment:1 Changed 4 years ago by dmethvin

  • Component changed from unfiled to event

comment:2 Changed 4 years ago by snover

  • Priority set to undecided

#7000 is a duplicate of this bug.

comment:3 Changed 4 years ago by snover

  • Priority changed from undecided to low

#6310 is a duplicate of this bug.

comment:4 Changed 4 years ago by snover

  • Status changed from new to open
  • Version changed from 1.4.2 to 1.4.3
  • Milestone changed from 1.4.3 to 1.5

comment:5 Changed 4 years ago by snover

#6109 is a duplicate of this ticket.

comment:6 Changed 4 years ago by adel.m.salah@…

A possible workaround is using :

$('#file').bind($.browser.msie? 'propertychange': 'change', handler);

instead of

$('#file').change(handler);

comment:7 Changed 4 years ago by jitter

  • Version changed from 1.4.3 to 1.4.4

comment:8 Changed 4 years ago by jitter

#7674 is a duplicate of this ticket.

comment:9 Changed 4 years ago by dennisjq

I think this issue is more serious than it first appears. If you have a live 'change' event tied to anything else on the DOM the select tag's onchange event still fires twice.

See the modified example here:  http://jsfiddle.net/ETzwU/1/

The .live('change') call matches *nothing* on the page, yet the select tag's onchange event is still fired twice.

comment:10 Changed 4 years ago by Brandon

Note: This issue occurs also in IE7 and IE6 and applies to <input type='text' /> (and probably all INPUT types).

And I agree with dennisjq that it is more serious than "low". Using live change events on input text fields in conjunction with ASP.NET textboxes that are set to AutoPostback=true attempt to perform 2 partial postbacks when the user types in a value.

comment:11 Changed 4 years ago by brandon@…

I've written a more complete jsFiddle example which shows the tricky nature of the bug:  http://jsfiddle.net/2ARLa/3/ When it comes up, just spend a bit modifying the text in the 2 textboxes then hit continue and it will show QUnit test results of:

programmatic changes on the textbox and how change events are handled vs user-interaction changes on the textbox and how change events are handled.

The bug only occurs on IE, and *only* when the user interactively changes the value. It *works correctly when the value is changed via code* so it will not show up in a normal regression test.

I spent the day on this since it was impacting my project and I've coded a fix for 1.4.2. Once I port it to 1.4.4, I'll post the fix here and explain it.

comment:12 Changed 4 years ago by brandon@…

Ok,

Here's the fix for 1.4.4 and how it works:

The trick is that on IE, if the value changed due to user interaction, IE will fire the inline onchange event automatically and jQuery should *not* fire it. But if the value changed due to programming, or the change event was triggered via code, then IE will *not* fire the onchange event and jQuery should do it (as it currently does).

I never could come up with an entirely satisfactory way to determine when the value was changed by the user that would always work. I finally settled on a sort of hack:

1 - When a change event handler is registered, I also register a handler for the IE "onpropertychange" event. event.handle() has special case code that watches for "propertychange" events on the "value" property. If it sees one of these, it examines the current call stack. If the call stack is very short, then this means the propertychange event must have come from the browser in response to a user action. If the stack is not short, then the event must have been fired in response to programmer action (code setting the .value property of the input). This code sets a flag on the element indicating whether or not the element was modified by the user. This codes sits inside event.handle() instead of inside the propertychange event handler to keep the stack trace code as simple and robust to changes as possible.

2 - Inside event.trigger(), just before calling the inline onchange event handler, there is code which checks for this user edited flag. If it is set, then it will not call the onchange event handler, but will instead just clear the flag for next time.

Here is a unified diff of the required changes to jquery-1.4.4.js.


--- E:\dev\bpa\src\BpaWeb\script\jquery-1.4.4.js	2010-12-30 08:11:07.000000000 -0600
+++ E:\dev\bpa\src\BpaWeb\script\jquery-1.4.4-bpafixes.js	2010-12-30 08:35:51.000000000 -0600
@@ -10,13 +10,20 @@
  * http://sizzlejs.com/
  * Copyright 2010, The Dojo Foundation
  * Released under the MIT, BSD, and GPL Licenses.
  *
  * Date: Thu Nov 11 19:04:53 2010 -0500
  */
-(function( window, undefined ) {
+/* ***********************
+*
+* Includes the following fixes patched in by BPA Team:
+*
+* BPA-7595 - a bugfix to the live change event that was causing the onchange to fire 2x for the target element
+*/
+(function (window, undefined)
+{
 
 // Use the correct document accordingly with window argument (sandbox)
 var document = window.document;
 var jQuery = (function() {
 
 // Define a local copy of jQuery
@@ -2161,23 +2168,31 @@
 		if ( handle ) {
 			handle.apply( elem, data );
 		}
 
 		var parent = elem.parentNode || elem.ownerDocument;
 
-		// Trigger an inline bound script
-		try {
-			if ( !(elem && elem.nodeName && jQuery.noData[elem.nodeName.toLowerCase()]) ) {
-				if ( elem[ "on" + type ] && elem[ "on" + type ].apply( elem, data ) === false ) {
-					event.result = false;
-					event.preventDefault();
+		// ***** BEGIN BPA-7595 *****
+		// Do *not* trigger inline bound script if this is the change event and the user edited us directly.  This is because IE will have already called the onchange
+		if (elem && type == "change" && jQuery.data(elem, "_edited_by_user"))
+		{
+			jQuery.data (elem, "_edited_by_user", false);
+		} else {
+			// Trigger an inline bound script
+			try {
+				if ( !(elem && elem.nodeName && jQuery.noData[elem.nodeName.toLowerCase()]) ) {
+					if ( elem[ "on" + type ] && elem[ "on" + type ].apply( elem, data ) === false ) {
+						event.result = false;
+						event.preventDefault();
+					}
 				}
-			}
 
-		// prevent IE from throwing an error for some elements with some event types, see #3533
-		} catch (inlineError) {}
+			// prevent IE from throwing an error for some elements with some event types, see #3533
+			} catch (inlineError) {}
+		}
+		// ***** END BPA-7595 *****
 
 		if ( !event.isPropagationStopped() && parent ) {
 			jQuery.event.trigger( event, data, parent, true );
 
 		} else if ( !event.isDefaultPrevented() ) {
 			var old,
@@ -2239,13 +2254,23 @@
 		if ( typeof events === "function" ) {
 			events = events.events;
 		}
 
 		handlers = (events || {})[ event.type ];
 
-		if ( events && handlers ) {
+		// ***** BEGIN BPA-7595 *****
+		if (event.type == "propertychange" && event.originalEvent && event.originalEvent.propertyName == "value")
+		{
+			// See if we are deep in a callstack (indicates the value was changed and this event triggered programatically and thus IE will *not* be calling onchange directly)
+			// or if we are near the top of the callstack (indicates this property was changed due to user action and thus IE will be calling onchange directly)
+			jQuery.data(this, "_edited_by_user", !arguments.callee.caller || !arguments.callee.caller.caller);
+		}
+		// ***** END BPA-7595 *****
+
+		if (events && handlers)
+		{
 			// Clone the handlers to prevent manipulation
 			handlers = handlers.slice(0);
 
 			for ( var j = 0, l = handlers.length; j < l; j++ ) {
 				var handleObj = handlers[ j ];
 
@@ -2601,12 +2626,16 @@
 			return jQuery.event.trigger( e, arguments[1], elem );
 		}
 	};
 
 	jQuery.event.special.change = {
 		filters: {
+			// ***** BEGIN BPA-7595 *****
+			propertychange: function (e) { /* no-op.  Just registering this event handler causes code in jQuery.event.handle() to examine all propertychange events for this input field. */ },
+			// ***** END BPA-7595 *****
+
 			focusout: testChange, 
 
 			beforedeactivate: testChange,
 
 			click: function( e ) {
 				var elem = e.target, type = elem.type;

comment:13 Changed 4 years ago by brandon@…

I've submitted the patch to GITHUB:  https://github.com/jquery/jquery/pull/156

comment:14 Changed 4 years ago by nelis

This should have a much higher priority. A select's onchange shouldn't be called twice. This way upgrading jQuery is a painful process.

Workaround: teardown this functionality for selects

var special = jQuery.event.special['change'] || {};
$('select').each(function() {
    special.teardown.call(this);
});

comment:15 Changed 4 years ago by nikolai.essel@…

We have the problem in IE7-9 with jQuery 1.4.2 & 1.5 too:

A select box has a change event binded via HTML. We add an additional handler via jQuery. If the select box entry are changed by user, the HTML-EventHandler is called twice because of the jQuery "testChange.call( this, e );" click handler.

Testable at  http://jsfiddle.net/q6yzC/ :

A counter is loaded with 0. The default handler is incrementing the counter. The dynamic handler does nothing. When changing the selectbox from "val2" to "val1", the counter is incremented twice -> counter status can be shown by clicking the button

comment:16 Changed 4 years ago by brandon@…

Note the patch I pasted above wasn't quite right. But the  pull request I linked *is* correct.

I've not yet tried to apply it to 1.5 yet but will in the next few weeks.

comment:17 Changed 4 years ago by anonymous

Priority of this ticket should'nt be low.

comment:18 Changed 4 years ago by john

  • Priority changed from low to high
  • Milestone set to 1.next

We're discussing the patch over in the pull request.

comment:19 Changed 4 years ago by john

  • Blocking 6705 added

(In #6705) I think this is actually very similar to #6593. We're working to try and fix the problem over there for the change event.

comment:20 Changed 3 years ago by dmethvin

  • Owner set to dmethvin
  • Status changed from open to assigned
  • Milestone changed from 1.next to 1.7

I have a fix working on my 1.7 branch, will test with this once it's landed later this month:

 http://jsfiddle.net/dmethvin/q6yzC/11/

comment:21 Changed 3 years ago by dmethvin

  • Blocking 6705 removed

(In #6705) Confirmed:  http://jsfiddle.net/dmethvin/gsJyE/

This appears to be fixed in my 1.7 branch, I'll confirm once it lands.

comment:22 Changed 3 years ago by Dave Methvin

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

Fix #6593. Don't trigger change event twice when simulating it on IE.

Changeset: 619a89d5ff6f5bcf678bdb88c3b47dea96b06ea7

comment:23 Changed 3 years ago by Dave Methvin

Update "Fix #6593. Don't trigger change event twice when simulating it on IE."

This reverts commit 3d0de29d5615c1b1d74c72e6272484961a4ba243.

Changeset: 4030de9519e91fdc247af3c508a8ede277242c3f

comment:24 Changed 3 years ago by Dave Methvin

Revert "Fix #6593. Don't trigger change event twice when simulating it on IE." Add inline handler monitor to delegatetest.html so we can see it fail.

This reverts commit e77686495b6e34938575c9c0fe978cb4a0be6f05.

Changeset: 6ece8f138f0c89d72fd597d95aa87304cdbe4d1d

comment:25 Changed 3 years ago by dmethvin

  • Status changed from closed to reopened
  • Resolution fixed deleted

comment:26 Changed 3 years ago by dmethvin

  • Status changed from reopened to open

Unfortunately this did not (yet) get fixed, each potential solution caused its own, more significant, problems. I'll note that inline handlers are generally a wontfix item, see  http://docs.jquery.com/Won't_Fix , but I'll leave this open for a while in hopes I can find a better solution.

comment:27 Changed 3 years ago by Dave Methvin

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

Fix #6593. Don't let onchange trigger twice for elements in IE.

This is a major revamp of the approach we use for IE change events. Instead of trying to track and simulate, we lazy-attach real change events to inputs and have only one workaround for check/radio. Somewhat more resource intensive but closes several sticky bugs. The onchange is still triggered for check/radio on blur but no double-trigger on any element occurs.

Changeset: 3bd7bed340f9077d39734ffce366ef2caeb9ce35

comment:28 Changed 3 years ago by mike@…

Thanks for finally fixing this. This was perfectly fine in jquery 1.3 and broken between 1.4 and 1.6.4 - which is a VERY long time to be broken.

This sort of big is extremely hard for your average developer to track down and we tend to rely on jquery being fairly solid. It is supposed to reduce browser incompatibilites but in this case it actually created browser incompatibility due to unnecessary dicking around with events dispatch.

One of the original tenets of jquery was leaving the native objects an global namespace alone (eg not adding a foreach method to "object"). I believe this ought to also apply to being extremely careful not to screw up native events. First, do no harm.

John et al - please give these thoughts some consideration.

comment:29 Changed 3 years ago by mike@…

Ps. I did not see this bug in lost of bugs fixed in 1.7 but it was a bug in 1.6.4 and was fixed in 1.7. Will this still be covered in your regression tests? Thanks.

comment:30 Changed 3 years ago by anonymous

is this bug fixed?If yes tell me which version??

Note: See TracTickets for help on using tickets.