789036 - MooTools 1.2.x was broken by adding String.prototype.contains

Status: RESOLVED FIXED

(Tech Evangelism Graveyard :: English US, defect)

Description

[:Benjamin Peterson]

This is a meta bug for breakage caused by bug 772733's addition of String.prototype.contains. MooTools defines a incompatible version of String.prototype.contains. Versions of MooTools greater than 1.2 override the builtin implementation, but 1.2.x assumes it is a compatible implementation. Upgrading to any later version of MooTools fixes the problem.

A MooTools bug has been filed:
https://github.com/mootools/mootools-core/issues/2402

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Do we know how widespread this breakage is? MooTools claims to have somewhere between 5-8% marketshare, it's probably realistic that quite some of those aren't updating their framework regularly, if at all.

This might end up breaking a reasonable (>1%) amount of sites...

Comment 2

[:Benjamin Peterson]

For historical information about a similar snafu with Function.prototype.bind, see bug 784280 comment #10.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I think we should strongly consider disabling this on beta to avoid the breakage for the moment while continuing to evangelize the sites involved....

Comment 4

[:Benjamin Peterson]

It's not in beta yet, but yes I agree. We can wait at least another month, though.

Comment 5

[Alex Keybl [:akeybl]]

(In reply to Benjamin Peterson [:benjamin] from comment #4)
> It's not in beta yet, but yes I agree. We can wait at least another month,
> though.

I don't want our users to have a bad beta experience (even for a couple of betas), unless we think it's the only leverage for sites to fix. Were you thinking about fixing it for FF16b1 or later?

Comment 6

[:Benjamin Peterson]

(In reply to Alex Keybl [:akeybl] from comment #5)
> (In reply to Benjamin Peterson [:benjamin] from comment #4)
> > It's not in beta yet, but yes I agree. We can wait at least another month,
> > though.
> 
> I don't want our users to have a bad beta experience (even for a couple of
> betas), unless we think it's the only leverage for sites to fix. Were you
> thinking about fixing it for FF16b1 or later?

I propose beta 2 or 3.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Our "beta"s are actually release candidates.  That's how we're pitching them, and that's how we should treat them.

We shouldn't be shipping known bugs on beta if we can avoid it, imo.  That's what Aurora is for...

Comment 8

[:Benjamin Peterson]

I should note browser JS has started depending on contains.

Comment 9

[Boris Zbarsky [:bzbarsky]]

That's even more of a reason to not ship it in beta.

In fact, if extensions are going to depend on it, then we can't ship it in beta, and ideally shouldn't be shipping it in Aurora, because we have extension compat promises we make with Aurora and beta builds...

Comment 10

[Jeff Walden [:Waldo]]

(In reply to Boris Zbarsky (:bz) from comment #9)
> In fact, if extensions are going to depend on it, then we can't ship it in
> beta, and ideally shouldn't be shipping it in Aurora, because we have
> extension compat promises we make with Aurora and beta builds...

Extensions don't have to depend on it unless they specifically opt in.  Seeing as we didn't tell extension authors about any of the new string methods (going by bug 772733 still having dev-doc-needed, and there not being a page for String.prototype.contains yet, and if I say anything more right now I'm going to sound disturbingly like philor), and seeing as if they're targeting nightly or Aurora I think we have to presume they know they're playing with fire, I don't see that we need to disable this in Aurora too.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I can sort of buy that.  For beta, though, we promise extensions basically no breaking change of any sort after beta1, unless it's fixing a stop-ship bug kinda thing.  So we absolutely need to disable the string methods before going to beta.

Comment 12

[Alex Keybl [:akeybl]]

(In reply to Boris Zbarsky (:bz) from comment #9)
> That's even more of a reason to not ship it in beta.
> 
> In fact, if extensions are going to depend on it, then we can't ship it in
> beta, and ideally shouldn't be shipping it in Aurora, because we have
> extension compat promises we make with Aurora and beta builds...

I agree with this. Nobody's made the case for how trying to leverage beta to get a MooTools fix outweighs the add-on/browser risk to disabling later in the cycle. I'll clone for the in-product change and leave this open for TE.

Comment 13

[:Benjamin Peterson]

Fixed in bug 793781 for 17, so untracking there.

Comment 14

[David Bruant]

I've added a section on MDN on the topic https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/contains#Broken_websites_with_MooTools_1.2_and_incompatible_signature_with_MooTools_1.3.2B

Comment 15

[Alex Keybl [:akeybl]]

Untracking in favor of bug 793781.

Comment 16

[:Benjamin Peterson]

MooTools 1.5 will have a ES6 compatible contains signature!

https://github.com/arian/mootools-core/commit/f9307e0c9dcae6a63b8f905c0b977368179d3d8b

Comment 17

[Daniel Buchner [:dbuc]]

Hey guys, I used to slang code for MooTools and I can run point on contacting and coordinating with them to address this. What is the honeymoon scenario we're looking for here?

Comment 18

[:Benjamin Peterson]

Ideally, all the dependent bugs would be closed. :) I've tried to contact them all, but feel free send your own.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Looks like we shipped this breakage in Firefox 18.  Just for post-mortem purposes, was that a conscious decision, or did it just slip through the cracks?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Documented at <https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_18#Mootools_1.2.x_is_not_compatible_with_Firefox_18_and_newer>.  If we _did_ ship this on purpose, I'd like to understand why we didn't bother documenting it or relnoting it or anything.

Comment 21

[:Benjamin Peterson]

It was a conscious decision. At least in the case of HP, they told me several times they were going to fix it by now. Trust but verify, I suppose.

Comment 22

[Boris Zbarsky [:bzbarsky]]

OK.  Next time we ship something like this on purpose, please make sure it's relnoted and documented in our site compat docs, ok?

Comment 23

[Brendan Eich [:brendan]]

(In reply to :Benjamin Peterson from comment #21)
> It was a conscious decision. At least in the case of HP, they told me
> several times they were going to fix it by now. Trust but verify, I suppose.

HP is just one instance of the problem. Why should Firefox risk losing market share to take this hit? We need to push back in Ecma TC39 and get other browsers making the same change *after* a coordinated evangelism effort.

Should this get disabled in 18.0.1? It seems so from all the comments here.

/be

Comment 24

[:Benjamin Peterson]

(In reply to Brendan Eich [:brendan] from comment #23)
> (In reply to :Benjamin Peterson from comment #21)
> > It was a conscious decision. At least in the case of HP, they told me
> > several times they were going to fix it by now. Trust but verify, I suppose.
> 
> HP is just one instance of the problem. Why should Firefox risk losing
> market share to take this hit? We need to push back in Ecma TC39 and get
> other browsers making the same change *after* a coordinated evangelism
> effort.

Push back into ECMA TC39 for what?

> 
> Should this get disabled in 18.0.1? It seems so from all the comments here.

I don't think I'm one to make such a decision. I should not it's not completely trival because chrome code is using contains().

Comment 27

[David Weir (satdav)]

I am from support and don't see a lot of users complaining about this issue

Comment 28

[Jeremy Lwanga]

(In reply to David Weir (satdav) from comment #27)
That's probably because a lot of the users haven't upgraded to version 1.8 and haven't visited a site using mootools 1.2xx

You can see an issue here; try replying to this - http://www.techrepublic.com/forum/discussions/103-401241, firefox is failing to getParent(<selector>)

Here's another example, the social toolbar here [http://www.techrepublic.com/blog/window-on-windows/the-first-10-things-you-should-do-to-a-new-windows-8-desktop-installation/6467] should have a more button at the end. Check it out in other browsers or older versions of firefox

Comment 29

[Swarnava Sengupta (:Swarnava)]

Go to http://foto-treff-bielefeld.de/
try to visit any link from black navigation bar
if you click, nothing happening

Comment 31

[Daniel Buchner [:dbuc]]

(In reply to Brendan Eich [:brendan] from comment #23)
> (In reply to :Benjamin Peterson from comment #21)
> > It was a conscious decision. At least in the case of HP, they told me
> > several times they were going to fix it by now. Trust but verify, I suppose.
> 
> HP is just one instance of the problem. Why should Firefox risk losing
> market share to take this hit? We need to push back in Ecma TC39 and get
> other browsers making the same change *after* a coordinated evangelism
> effort.
> 
> Should this get disabled in 18.0.1? It seems so from all the comments here.
> 
> /be

As this affects quite old version of the MooTools library, specifically the 1.2.x branch which is now ~5 years old, MooTools will be releasing a 1.2.6 for that legacy code which corrects the issue for MooTools users. the 1.3+ branches of the library are all fine and have had no issues.

We'll need to updated our engagement actions on this front and push people to the new version. There's a blog post imminent that will describe all of this and includes the new code, I'll link to it here as soon as it's released.

Comment 32

[Loic]

MooTools 1.2.6 Released: http://mootools.net/blog/2013/02/19/mootools-1-2-6-released/
This is a new maintenance release for the old 1.2 series. The new ECMAScript 6 specification has a proposal for String.prototype.contains that unfortunately conflicts with the MooTools implementation of contains.
Firefox 18 already ships this new version of contains. This is not a problem for MooTools 1.3.x and onward, but this breaks MooTools 1.2.5, especially the code inside the MooTools framework that utilized this method, like selectors.

Comment 33

[Hallvord R. M. Steen [:hallvors]]

This can be resolved since MooTools shipped the fix already..