Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix 2170 ie element event change propertychange #2203

Merged
merged 2 commits into from

3 participants

@ibolmo
Owner

Adds fix and spec coverage for Element.Event.change. Specifically propertychange event in IE that was called too many times.

See #2170 and #2172.

Source/Element/Element.Event.js
@@ -175,7 +175,7 @@ if (!window.addEventListener){
return (this.get('tag') == 'input' && (type == 'radio' || type == 'checkbox')) ? 'propertychange' : 'change'
},
condition: function(event){
- return !!(this.type != 'radio' || this.checked);
+ return event.type == 'change' || Boolean(this.checked) && event.type == 'propertychange' && event.event.propertyName == 'checked';
@DimitarChristoff Collaborator

isn't this more expensive - Boolean(this.checked) - can it affect delegation in IEx?

@arian Owner
arian added a note

I don't think this check for this.checked is necessary is it? I just tested it with Specs/1.4client/Element/Element.Event.change.html and with this check it logs:

native change
change delegation

only if you check the checkbox.

Without that Boolean(this.checked) it always logs:

native change
change bubbles
change delegation

However with radio buttons it fires the event twice...

native change
change bubbles
change delegation
native change

So I guess that check is for radio buttons, however how it is in this commit, it won't work for checkboxes.

@csuwldcat, can you confirm?

@DimitarChristoff Collaborator

it is needed.

if you don't have it, on a set of radios, eg. <input name='r' value='1' checked='checked' /> <input name='r' value='2' />- it will fire change on BOTH of the input elements, one for the checked -> true and one checked -> false. this does not make sense as they represent variations of the same core element, input[name=r] and only one value will submit.

the spec test is too elementary and does not cover this - i found it through our production use and onChange branching events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ibolmo added some commits
@ibolmo ibolmo commented on the diff
Source/Element/Element.Event.js
@@ -175,7 +175,7 @@ if (!window.addEventListener){
return (this.get('tag') == 'input' && (type == 'radio' || type == 'checkbox')) ? 'propertychange' : 'change'
},
condition: function(event){
- return !!(this.type != 'radio' || this.checked);
+ return this.type != 'radio' || (event.event.propertyName == 'checked' && this.checked);
@ibolmo Owner
ibolmo added a note

@DimitarChristoff @arian @csuwldcat

Ok folks, have a go at it. As I understand, the issue can be resolved by just ensuring that the Event.propertyName is for checked. Then we just depend on the this.checked value (false|true).

@DimitarChristoff Collaborator

return !!(event.type === 'change' || this.checked && event.type === 'propertychange' && event.event.propertyName == 'checked') was my original suggestion. the difference is, event.type === 'propertychange' also ensures it is a radio or checkbox that has triggered it. there is no reason to go to element.type again and access a property on the element after the setup cond already has determined that and type won't change in-between.

@ibolmo Owner
ibolmo added a note

There's no need for that check if we have this.type != 'radio' which I believe is the premise of the custom Element.Event.change to begin with. We were fixing the change event for radios.

Also, I prefer this.type != 'radio' vs event.type == 'propertychange' since the former is more readable.

@DimitarChristoff Collaborator

i am happy with above, seems to work and checkbox' are fine + it fixes bubbling.

return event.type == 'change' || this.type != 'radio' || (event.event.propertyName == 'checked' && this.checked); - seems faster for other els - input, select etc - when it's a native change there is no need to access type or the event object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ibolmo ibolmo merged commit bc70096 into mootools:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 18, 2012
  1. @ibolmo

    Fixes #2170.

    ibolmo authored
    Using DimitarChristoff's fix. See #2170 and #2172.
    
    PASSED: IE6-9.
  2. @ibolmo
This page is out of date. Refresh to see the latest.
View
2  Source/Element/Element.Event.js
@@ -175,7 +175,7 @@ if (!window.addEventListener){
return (this.get('tag') == 'input' && (type == 'radio' || type == 'checkbox')) ? 'propertychange' : 'change'
},
condition: function(event){
- return !!(this.type != 'radio' || this.checked);
+ return this.type != 'radio' || (event.event.propertyName == 'checked' && this.checked);
@ibolmo Owner
ibolmo added a note

@DimitarChristoff @arian @csuwldcat

Ok folks, have a go at it. As I understand, the issue can be resolved by just ensuring that the Event.propertyName is for checked. Then we just depend on the this.checked value (false|true).

@DimitarChristoff Collaborator

return !!(event.type === 'change' || this.checked && event.type === 'propertychange' && event.event.propertyName == 'checked') was my original suggestion. the difference is, event.type === 'propertychange' also ensures it is a radio or checkbox that has triggered it. there is no reason to go to element.type again and access a property on the element after the setup cond already has determined that and type won't change in-between.

@ibolmo Owner
ibolmo added a note

There's no need for that check if we have this.type != 'radio' which I believe is the premise of the custom Element.Event.change to begin with. We were fixing the change event for radios.

Also, I prefer this.type != 'radio' vs event.type == 'propertychange' since the former is more readable.

@DimitarChristoff Collaborator

i am happy with above, seems to work and checkbox' are fine + it fixes bubbling.

return event.type == 'change' || this.type != 'radio' || (event.event.propertyName == 'checked' && this.checked); - seems faster for other els - input, select etc - when it's a native change there is no need to access type or the event object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
}
}
View
28 Specs/1.4client/Element/Element.Event.js
@@ -82,4 +82,32 @@ describe('Element.Event', function(){
});
+describe('Element.Event.change', function(){
+
+ it('should not fire "change" for any property', function(){
+ var callback = jasmine.createSpy('Element.Event.change');
+
+ var input = new Element('input', {
+ 'type': 'radio',
+ 'class': 'someClass',
+ 'checked': 'checked'
+ }).addEvent('change', callback).inject(document.body);
+
+ input.removeClass('someClass');
+ expect(callback).not.toHaveBeenCalled();
+
+ var text = new Element('input', {
+ 'type': 'text',
+ 'class': 'otherClass',
+ 'value': 'text value'
+ }).addEvent('change', callback).inject(document.body);
+
+ text.removeClass('otherClass');
+ expect(callback).not.toHaveBeenCalled();
+
+ [input, text].invoke('destroy');
+ });
+
+});
+
})();
Something went wrong with that request. Please try again.