Side navigation
#6593 closed bug (fixed)
Opened May 23, 2010 07:25PM UTC
Closed September 21, 2011 02:29AM UTC
Last modified April 19, 2012 08:07AM UTC
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: | |
Blocked by: | Blocking: |
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
Attachments (0)
Change History (30)
Changed June 15, 2010 01:45AM UTC by comment:1
component: | unfiled → event |
---|
Changed October 02, 2010 05:47PM UTC by comment:2
priority: | → undecided |
---|
#7000 is a duplicate of this bug.
Changed October 02, 2010 06:07PM UTC by comment:3
priority: | undecided → low |
---|
#6310 is a duplicate of this bug.
Changed October 18, 2010 12:36AM UTC by comment:4
milestone: | 1.4.3 → 1.5 |
---|---|
status: | new → open |
version: | 1.4.2 → 1.4.3 |
Changed November 20, 2010 03:48PM UTC by comment:6
A possible workaround is using :
$('#file').bind($.browser.msie? 'propertychange': 'change', handler);
instead of
$('#file').change(handler);
Changed December 01, 2010 04:30PM UTC by comment:7
version: | 1.4.3 → 1.4.4 |
---|
Changed December 16, 2010 09:39PM UTC by comment:9
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.
Changed December 29, 2010 09:02PM UTC by comment:10
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.
Changed December 30, 2010 04:24AM UTC by comment:11
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.
Changed December 30, 2010 02:52PM UTC by comment:12
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;
Changed December 30, 2010 05:54PM UTC by comment:13
I've submitted the patch to GITHUB: https://github.com/jquery/jquery/pull/156
Changed February 09, 2011 07:34AM UTC by comment:14
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); });
Changed February 22, 2011 12:34PM UTC by comment:15
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
Changed February 24, 2011 04:07PM UTC by comment:16
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.
Changed March 22, 2011 04:42PM UTC by comment:17
Priority of this ticket should'nt be low.
Changed April 17, 2011 08:07PM UTC by comment:18
milestone: | → 1.next |
---|---|
priority: | low → high |
We're discussing the patch over in the pull request.
Changed April 17, 2011 08:13PM UTC by comment:19
Changed September 08, 2011 05:23PM UTC by comment:20
milestone: | 1.next → 1.7 |
---|---|
owner: | → dmethvin |
status: | open → assigned |
I have a fix working on my 1.7 branch, will test with this once it's landed later this month:
Changed September 08, 2011 05:33PM UTC by comment:21
blocking: | 6705 |
---|
(In #6705) Confirmed: http://jsfiddle.net/dmethvin/gsJyE/
This appears to be fixed in my 1.7 branch, I'll confirm once it lands.
Changed September 19, 2011 07:43PM UTC by comment:22
resolution: | → fixed |
---|---|
status: | assigned → closed |
Fix #6593. Don't trigger change
event twice when simulating it on IE.
Changeset: 619a89d5ff6f5bcf678bdb88c3b47dea96b06ea7
Changed September 19, 2011 07:43PM UTC by comment:23
Update "Fix #6593. Don't trigger change
event twice when simulating it on IE."
This reverts commit 3d0de29d5615c1b1d74c72e6272484961a4ba243.
Changeset: 4030de9519e91fdc247af3c508a8ede277242c3f
Changed September 19, 2011 07:43PM UTC by comment:24
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
Changed September 19, 2011 07:52PM UTC by comment:25
resolution: | fixed |
---|---|
status: | closed → reopened |
Changed September 19, 2011 07:55PM UTC by comment:26
status: | reopened → 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.
Changed September 21, 2011 02:29AM UTC by comment:27
resolution: | → fixed |
---|---|
status: | open → closed |
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
Changed November 23, 2011 07:45PM UTC by comment:28
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.
Changed November 23, 2011 07:51PM UTC by comment:29
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.
Changed April 19, 2012 08:07AM UTC by comment:30
is this bug fixed?If yes tell me which version??