fix for any propertychange firing onChange in ie7/8 on checkboxes AND radios #2172

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@DimitarChristoff
Member

DimitarChristoff commented Dec 13, 2011

see #2170, updated to also support checkboxes. testcase: http://jsfiddle.net/5CvBb/12/

@arian

This comment has been minimized.

Show comment Hide comment
@arian

arian Dec 13, 2011

Owner

nice, I'll wait for others to comment (@ibolmo, @csuwldcat, @cpojer), but it looks good to me!

Owner

arian commented Dec 13, 2011

nice, I'll wait for others to comment (@ibolmo, @csuwldcat, @cpojer), but it looks good to me!

@DimitarChristoff

This comment has been minimized.

Show comment Hide comment
@DimitarChristoff

DimitarChristoff Dec 13, 2011

Member

no, I am actually checking this atm for breakages on other input types which will likely fail.

Member

DimitarChristoff commented Dec 13, 2011

no, I am actually checking this atm for breakages on other input types which will likely fail.

@arian arian reopened this Dec 13, 2011

@DimitarChristoff

This comment has been minimized.

Show comment Hide comment
@DimitarChristoff

DimitarChristoff Dec 13, 2011

Member

actually. this causes errors.

return !!(event.type === 'change' || event.type === 'propertychange' && event.event.propertyName == 'checked')

works for all elements and change is fine. http://jsfiddle.net/5CvBb/23/ - however, it fires a change event on a radio group sharing the same name to all elements that experience propertychange of checked. not sure if this is the accepted dom model.

commit / sha:
DimitarChristoff/mootools-core@6abff54

let me know if i should make a new pull request.

Member

DimitarChristoff commented Dec 13, 2011

actually. this causes errors.

return !!(event.type === 'change' || event.type === 'propertychange' && event.event.propertyName == 'checked')

works for all elements and change is fine. http://jsfiddle.net/5CvBb/23/ - however, it fires a change event on a radio group sharing the same name to all elements that experience propertychange of checked. not sure if this is the accepted dom model.

commit / sha:
DimitarChristoff/mootools-core@6abff54

let me know if i should make a new pull request.

@csuwildcat

This comment has been minimized.

Show comment Hide comment
@csuwildcat

csuwildcat Dec 13, 2011

Contributor

Being that I haven't tested this yet, I hate to ask the seemingly obvious question: Why can't you just add a condition that ensures the changed attributed is "checked" to the part of the existing conditions that affect only radios and checkboxes?

Contributor

csuwildcat commented Dec 13, 2011

Being that I haven't tested this yet, I hate to ask the seemingly obvious question: Why can't you just add a condition that ensures the changed attributed is "checked" to the part of the existing conditions that affect only radios and checkboxes?

@DimitarChristoff

This comment has been minimized.

Show comment Hide comment
@DimitarChristoff

DimitarChristoff Dec 13, 2011

Member

because it broke change for input[type=text], i can dig out the jsfiddle that did this but it's been a lot...

Member

DimitarChristoff commented Dec 13, 2011

because it broke change for input[type=text], i can dig out the jsfiddle that did this but it's been a lot...

@DimitarChristoff

This comment has been minimized.

Show comment Hide comment
@DimitarChristoff

DimitarChristoff Dec 13, 2011

Member

this fixes it so it only fires the change once per radio group:

return !!(event.type === 'change' || this.checked && event.type === 'propertychange' && event.event.propertyName == 'checked')

DimitarChristoff/mootools-core@53b0dc5
it also does one less property lookup due to base which already has read the type and set the event accordingly.

Member

DimitarChristoff commented Dec 13, 2011

this fixes it so it only fires the change once per radio group:

return !!(event.type === 'change' || this.checked && event.type === 'propertychange' && event.event.propertyName == 'checked')

DimitarChristoff/mootools-core@53b0dc5
it also does one less property lookup due to base which already has read the type and set the event accordingly.

@csuwildcat

This comment has been minimized.

Show comment Hide comment
@csuwildcat

csuwildcat Dec 13, 2011

Contributor

Did you run the tests in all the various browsers yet? Other than that, the logic seems to make sense.

Contributor

csuwildcat commented Dec 13, 2011

Did you run the tests in all the various browsers yet? Other than that, the logic seems to make sense.

@DimitarChristoff

This comment has been minimized.

Show comment Hide comment
@DimitarChristoff

DimitarChristoff Dec 13, 2011

Member

i haven't got the test/spec runner or repo at home (windows), will do tomorrow morning at work - just tested IE7 and IE8 so far, don't even have an IE6 VM

Member

DimitarChristoff commented Dec 13, 2011

i haven't got the test/spec runner or repo at home (windows), will do tomorrow morning at work - just tested IE7 and IE8 so far, don't even have an IE6 VM

@arian

This comment has been minimized.

Show comment Hide comment
@arian

arian Dec 13, 2011

Owner

You don't need that, just open the html file I linked to, it's in the -core repo now, and that file doesn't require the spec-runner.

Owner

arian commented Dec 13, 2011

You don't need that, just open the html file I linked to, it's in the -core repo now, and that file doesn't require the spec-runner.

@DimitarChristoff

This comment has been minimized.

Show comment Hide comment
@DimitarChristoff

DimitarChristoff Dec 13, 2011

Member

@arian that spec file is out of date, it references Type/Event.js not DOMEvent.js. fixed here: DimitarChristoff/mootools-core@6c8f004

tests pass, btw - at least on IE9 in IE7 and IE8 doc mode (forced). if you want to try it's here http://fragged.org/dev/mootools-core/Specs/1.4client/Element/Element.Event.change.html

Member

DimitarChristoff commented Dec 13, 2011

@arian that spec file is out of date, it references Type/Event.js not DOMEvent.js. fixed here: DimitarChristoff/mootools-core@6c8f004

tests pass, btw - at least on IE9 in IE7 and IE8 doc mode (forced). if you want to try it's here http://fragged.org/dev/mootools-core/Specs/1.4client/Element/Element.Event.change.html

@csuwildcat

This comment has been minimized.

Show comment Hide comment
@csuwildcat

csuwildcat Jan 4, 2012

Contributor

This fix prevents change from firing when unchecking a checked checkbox in IE7 and IE8, probably 6 too, but I don't have access to it right now. I will revisit the issue this week.

Source of error: http://fragged.org/dev/mootools-core/Specs/1.4client/Element/Element.Event.change.html

Contributor

csuwildcat commented Jan 4, 2012

This fix prevents change from firing when unchecking a checked checkbox in IE7 and IE8, probably 6 too, but I don't have access to it right now. I will revisit the issue this week.

Source of error: http://fragged.org/dev/mootools-core/Specs/1.4client/Element/Element.Event.change.html

ibolmo added a commit to ibolmo/mootools-core that referenced this pull request Jan 18, 2012

Fixes #2170.
Using DimitarChristoff's fix. See #2170 and #2172.

PASSED: IE6-9.

@ibolmo ibolmo closed this Jan 18, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment