Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

propertychange event still not firing in 2.1.0beta #406

Closed
andrewchch opened this issue Mar 26, 2012 · 6 comments
Closed

propertychange event still not firing in 2.1.0beta #406

andrewchch opened this issue Mar 26, 2012 · 6 comments
Milestone

Comments

@andrewchch
Copy link

Hi All,

I've just fetched the 2.1.0beta release and the issues others have reported (#102 and #122) are still there for me. I can see the propertychange event being registered:

'''javascript

    if (ieAutoCompleteHackNeeded && ko.utils.arrayIndexOf(eventsToCatch, "propertychange") == -1) {
        var propertyChangedFired = false;
        ko.utils.registerEventHandler(element, "propertychange", function () {
           propertyChangedFired = true
        });

...
}
'''

but it's not being caught by this handler. After digging into the jQuery side of things I came across this jQuery issue from a while back:

http://bugs.jquery.com/ticket/8485

"The bind() method uses feature detection to decide how to attach an event to an element. First it checks for "addEventListener" (DOM Events) and then "attachEvent" (IE specific implementation). IE 9 supports both "addEventListener" and "attachEvent" but IE 8 and bellow only support attachEvent.

The problem is that some IE specific events in IE 9 (e.g. onpropertychange - http://msdn.microsoft.com/en-us/library/ms536956(v=vs.85).aspx ) must be added using "attachEvent" and NOT "addEventListener". Since the code in first checks for "addEventListener" and IE 9 supports that as well, IE specific events will not be registered correctly using bind()."

I'm guessing the best place to handle this would be in ko.utils.registerEventHandler. Any thoughts?

Cheers, Andrew.

@mbest
Copy link
Member

mbest commented Mar 26, 2012

IE9 supports the input event. Maybe use that instead if it's IE9? (http://help.dottoro.com/ljhxklln.php)

@andrewchch
Copy link
Author

Thanks, Michael, but do you mean add an "event" binding in the view? FWIW, I've patched my local knockout for now with:

ko.utils.registerEventHandler = function (element, eventType, handler)
{
// [AJG 27/3/2012] Nasty hack: if this is propertychange event we need to explicitly attach
// the handler with attachEvent because we're running in IE and jQuery (quite reasonably) doesn't
// handle some non-standard IE events properly (http://bugs.jquery.com/ticket/8485)
if (eventType == "propertychange" && typeof element.attachEvent != "undefined")
{
element.attachEvent("on" + eventType, function (event)
{
handler.call(element, event);
});
return;
}
...

@SteveSanderson
Copy link
Contributor

Possible implementation in this pull request: #444

I'm happy with merging this into master, even though it is a bit annoying to have yet more IE-specific hacks, especially when it's new weirdness introduced in IE9. If we don't do this, our original fix for the autocomplete issue is a bit pointless as it won't work on current versions of IE.

Can anyone think of any risks with bypassing jQuery for propertychange event registrations? This would be the only event type for which we don't use $.bind when it's available.

@mbest
Copy link
Member

mbest commented Apr 23, 2012

Can anyone think of any risks with bypassing jQuery for propertychange event registrations? This would be the only event type for which we don't use $.bind when it's available.

I think it's acceptable, but if you want to reduce the risk, you could make the bypass only happen in IE9+.

@SteveSanderson
Copy link
Contributor

To maximise the chance of all IE versions behaving the same, I'd rather not introduce even more version-specific special cases.

Do you want to merge this in? Or just let me know if you regard this as "signed off" and I'll merge it in.

@mbest
Copy link
Member

mbest commented Apr 25, 2012

Merged #444.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants