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

Protect 'contains' method in Array prototype #2654

Merged
merged 1 commit into from Sep 25, 2014

Conversation

Projects
None yet
4 participants
@mattcoz
Contributor

mattcoz commented Sep 25, 2014

Firefox 35 includes support for the 'contains' method of ECMA 7. This breaks MooTools because it is not a protected method. Unfortunately a lot of sites are going to be broken by this, I've already tested a few including my own and jsfiddle. The specific error I see is that the Elements class does not implement the 'contains' method from the Array prototype. I've added this small change to my site and it fixes the problem.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/contains

Protect 'contains' method in Array prototype
Firefox 35 includes support for the 'contains' method of ECMA 7. This breaks MooTools because it is not a protected method. Unfortunately a lot of sites are going to be broken by this, I've already tested a few including my own and jsfiddle. The specific error I see is that the Elements class does not implement the 'contains' method from the Array prototype. I've added this small change to my site and it fixes the problem.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/contains
@arian

This comment has been minimized.

Show comment
Hide comment
@arian

arian Sep 25, 2014

Member

Don't know why Travis is failing, but this seems good to me!

Member

arian commented Sep 25, 2014

Don't know why Travis is failing, but this seems good to me!

@ibolmo

This comment has been minimized.

Show comment
Hide comment
@ibolmo

ibolmo Sep 25, 2014

Member

Let's pull and find out ;)

Member

ibolmo commented Sep 25, 2014

Let's pull and find out ;)

ibolmo added a commit that referenced this pull request Sep 25, 2014

Merge pull request #2654 from mattcoz/patch-1
Protect 'contains' method in Array prototype

@ibolmo ibolmo merged commit 6ada452 into mootools:master Sep 25, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
@arian

This comment has been minimized.

Show comment
Hide comment
@arian

arian Sep 25, 2014

Member

The Travis failure seems to be karma-runner/karma#1182

Member

arian commented Sep 25, 2014

The Travis failure seems to be karma-runner/karma#1182

@arian

This comment has been minimized.

Show comment
Hide comment
@arian

arian Sep 25, 2014

Member

@mattcoz, how did this break your site specifically? As far as I can see, Array.prototype.contains is the same for MooTools and Firefox, except in the case of NaN...

Member

arian commented Sep 25, 2014

@mattcoz, how did this break your site specifically? As far as I can see, Array.prototype.contains is the same for MooTools and Firefox, except in the case of NaN...

@mattcoz

This comment has been minimized.

Show comment
Hide comment
@mattcoz

mattcoz Sep 25, 2014

Contributor

The problem isn't with the method itself, it's that when implement(Array.prototype) is called, the contains method isn't implemented. Then when my site calls contains(or include, which calls contains) on an Elements object, an error is throws saying that the contains method doesn't exist.

Contributor

mattcoz commented Sep 25, 2014

The problem isn't with the method itself, it's that when implement(Array.prototype) is called, the contains method isn't implemented. Then when my site calls contains(or include, which calls contains) on an Elements object, an error is throws saying that the contains method doesn't exist.

@OscarGodson

This comment has been minimized.

Show comment
Hide comment
@OscarGodson

OscarGodson Sep 26, 2014

I'm trying to reproduce this error but Firefox 35 isn't giving me any errors with or without this patch. Could you give me a test case? I've tried ['foo'].contains('foo') and document.body.contains(document.body.getElement('div')). Both return true fine in Firefox 35 and I don't see any JS errors in FF35 on our site.

OscarGodson commented on 521471f Sep 26, 2014

I'm trying to reproduce this error but Firefox 35 isn't giving me any errors with or without this patch. Could you give me a test case? I've tried ['foo'].contains('foo') and document.body.contains(document.body.getElement('div')). Both return true fine in Firefox 35 and I don't see any JS errors in FF35 on our site.

This comment has been minimized.

Show comment
Hide comment
@arian

arian Sep 26, 2014

Member

$$('div').contains($$('div')[0]); probably.

Member

arian replied Sep 26, 2014

$$('div').contains($$('div')[0]); probably.

This comment has been minimized.

Show comment
Hide comment
@OscarGodson

OscarGodson Sep 26, 2014

Yep! that does it, thanks!

OscarGodson replied Sep 26, 2014

Yep! that does it, thanks!

@mootools mootools deleted a comment from lordeloalvaro01 Aug 25, 2017

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