Third-party scripts breaking Wysiwyg

> 		if(typeof(classes[i]) == 'string'){
> 			if (classes[i].substr(0, 8) == 'wysiwyg-') {
> 				var parts = classes[i].split('-');
> 				var value = parts.slice(2).join('-');
> 				params[parts[1]] = value;
> 			}
> 		}

Please use the -up switches when creating patches.
Indents should be two spaces per step.
typeof should be treated as a keyword, not function.
See JavaScript coding standards.

Thanks for finding, reporting and patching this.

Review powered by Dreditor

Here is an improved patch. Untested.

I'm curious about how the function got into that array in the first place. The split method would only leave strings in there, so the function is probably from some other code extending the Array prototype. If so, then it's likely that will cause problems in other places as well.
I only had access to Chromium in Ubuntu Karmic but there the module seems to work fine.

It would be great to know which array key/index the function used, and what code was in it (.toString should reveal that).

Yes, this sounds like babysitting some other broken code of something else.

little bit different problem but same result (not corrected by this patch), in IE7 only in this browser, the FCKEditor not shown and raise several javascript errors:

line 209; char 1; Expected identifier, string or number; sites/all/libraries/fckeditor/editor/fckeditor.html?InstanceName=edit-body&Toolbar=Wysiwyg

line 131; char 2; 'PluginsPath' is null or not an object; sites/all/libraries/fckeditor/editor/fckeditor.html?InstanceName=edit-body&Toolbar=Wysiwyg

line 46; char 20; FCKConfig.ContextMenu.lenght is null or not an object; sites/all/libraries/fckeditor/editor/fckeditor.html?InstanceName=edit-body&Toolbar=Wysiwyg

line 51; char 4; 'undefined' is null or not an object; sites/all/libraries/fckeditor/editor/fckeditor.html?InstanceName=edit-body&Toolbar=Wysiwyg

I just tested FCKEditor 2.6.5 in IE7 via Wysiwyg 2.0 and it works fine.
I'm afraid those errors aren't much help. The line numbers are completely off. (IE never was good at getting them right...) I searched for the variables mentioned in the descriptions by they didn't turn up much either.

I'd probably have to be able to attach a debugger to figure out where it goes wrong, unless maybe your editor configuration can give a hint. Can you post which editor settings and plugins you have enabled?

Are you running other modules which add scripts, modify the forms, AJAXify things etc?

Oh, this only happens in IE7, and does not include the previous error. Please open a new issue if there isn't already a matching one. Please provide the info I asked for above.
Reverting tags and subject.

This bug also caused IE8 and IE7 to fail with a JS error for me.

Thanks TwoD, you've really saved me a lot of headaches today.

Error in IE8 was:

Webpage error details

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0)
Timestamp: Fri, 5 Mar 2010 10:02:37 UTC

Message: Object doesn't support this property or method
Line: 195
Char: 5
Code: 0
URI: http:///sites/all/modules/wysiwyg/wysiwyg.js?t

OK, I tracked part of the reason why we were getting this error on IE. We are using the JQuery Plugin Stylish Select which does this:

  //create cross-browser indexOf
  Array.prototype.indexOf = function (obj, start) {
    for (var i = (start || 0); i < this.length; i++) {
      if (this[i] == obj) {
        return i;
      }
    }
  }

Instant addition of the function into the array. We removed the definition and it instantly worked. Hope it helps others [i.e. Do a code search for "Array.prototype.indexOf"]

Please see this issue of mine. Patch in #1 saved my life really though seems rather unrelated.

Also Wibiya support has many complaints about Drupal/jQuery/RTE combination, perhaps some of are related with this.

IMHO this type check issue is serious as it can go haywire at anytime, and this patch should be committed.

Sorry for hijacking here a bit.

Tx.

I tend to agree with Sun's opinion in #4. Extending the Array/Object prototypes is a bit rude and bound to cause problems in millions of scripts out there.
Here's a patch to work around the issue here though. Don't know yet if this problem will be encountered in more places, but I guess we'll find out.

Patch #11 works perfectly for my case (#10). Tx a lot.

Thus confirming.

The patch in #11 worked for my situation as well (http://groups.drupal.org/node/73253).

Thx for that!

- MWM

#11 works like a dream on toast for me, marking tested awaiting commit...

The bad/guilty code has to be fixed instead. Wysiwyg uses valid JavaScript, and by all means, even if someone changes the Array prototype to something special, then that code needs to ensure that array values still translate into the normally expected type and value.

Wysiwyg does use valid JavaScript, as does the Prototype library - which could be the cause of this conflict.
The problem is that JavaScript doesn't really have associative Arrays. It just has Objects with properties, which is what for...in loops iterates over. Arrays are Objects with an additional length property and some special treatment for numeric properties (indexes).
Methods can be considered callable properties of Objects. Browsers will expose all these properties to for...in loops, regardless of their value. Scripts - like Wysiwyg - can use the hasOwnProperty() method to check if an Object's property exists directly on that Object, or if it's part of its prototype chain. There is however no obvious way to distinguish between native properties in the prototype chain and custom ones. Their behavior also differs between browsers. This makes extending the prototypes of Arrays/Objects to be considered a bad habit in most cases.
Prototype 2.0 is said to move away from this practice - due to the many problems they themselves encountered - but lots of people will still be using earlier versions of Prototype and other JavaScript libraries which also extend the native Object prototypes.

Back to our case; Wysiwyg is likely to encounter several of these libraries due to other Drupal modules relying on them. jQuery being bundled with Drupal, and being a very capable library itself, doesn't stop existing code bridged into Drupal by other modules from being tied down to prototype-extending libraries, so we'll have to deal with this somehow. Wysiwyg's all about not having to modify library files (as in editors, plugins etc), right? Well, if we go with that line, we can't force people to have to rewrite JavaScript libraries used by some other script they want to integrate with Drupal while having Wysiwyg enabled either. We could however bend our code a bit if it helps alleviate the most common problems.

In this very specific case, we are guaranteed to have an Array with numeric indexes (as that's what the split() method returns). Hence, there's really no need for the for...in loop as we're only interested in the numeric properties of the Array.
Even if we didn't have this issue, I think using a regular for loop based on the length properly more clearly shows that we're only interested in the indexed values, and not all the properties of the Array.
jQuery's jQuery.each() method follows the expectations put on Arrays by only revealing the values of numerical indexes when passed an Array, vs the values of all properties when passed an Object. Internally it does its job by using a .length driven for loop when an Array is detected and a for...in loop in other cases.

My opinion is, if we don't change this line because some other library is injecting stuff where it's not expected, then how about changing it to more clearly show what we expect it to work with?
If it still breaks when using numerical indexes, the other code is doing some seriously careless tampering with the global scope and we can without a doubt rule out Wysiwyg as the "misbehaving" part.

So you changed your mind? :) If you think it's our responsibility to account for that possibility, then let's fix it.

While I generally prefer usage of $.each(), it's a (needless) function call in this case, so perhaps we should rather go with the patch. In any case, we'd have to define a standard method to resolve this conflict, since, as we've already figured, the same issue may arise in various other locations.

In case we go with the for loop, then this patch is fine. We should just squeeze in a space before and after the = equal sign before committing it :)

To some extent, yes, I changed my mind. Or rather, I found a motivation for the change in #11 which meant I wouldn't have to completely change my mind regarding prototype extensions, and then it became more of a side-effect that Wysiwyg will be able to survive a little longer amongst such scripts.

For future cases, where we have an Array we'll iterate over its numerical indexes only, otherwise we should have not been using an Array for storage because JavaScript doesn't really have associative arrays.
We deal with quite a lot of JSON data expressed using the literal {} and [] notations. This means we'll get Objects when we really wanted HasTables or Maps for associating keys with values. We currently use these in for...in loops without always checking that the property actually means something to us in all cases. That could most likely be done by calling obj.hasOwnProperty(key) to make sure we don't get anything in from the prototype chain. However, other libraries we work with, like jQuery and CKEditor, will still get issues when encountering extended Object prototypes. I've heard that hasOwnProperty won't work in some older browsers like IE5 and Safari 1.3, though that matters little to us.
There are probably other ways we could also check the properties that would be better at indicating what they represent to us (they have a 'format'-prefix, element ids they represent have corresponding elements, the property also exists in some other non-property place or is of a specific type etc).
I've tried to "reinforce" the 30+ for/each() loops over Objects we have like this, but it was not always successful because we use $.extend() to clone Objects, which will copy a prototype property down to a local Object instance, rendering hasOwnProperty() useless. But like I said earlier, other libraries we use will still break even if we add the checks, and Object prototype extensions don't seem to cause us much problems at this point anyway.
Where it's possible and practical, we have the responsibility to check our input. If that also catches cases where other code as modified global objects in an unexpected way, it's a nice bonus.

Ah spaces... Drupal's coding standard has more of them than I'm used to hehe.

Oh. Speaking of. The current JS coding standards http://drupal.org/node/172169#forin recommend the .hasOwnProperty() method.

So we actually have 2-3 issues here:

1) Use for...length for indexed arrays (rarely used in Wysiwyg, I think).

2) Use .hasOwnProperty() in for...in loops.

3) Consider standardization on for...in or $.each().

What do you think?

Is the root problem really that the .indexOf is somehow getting set to Enumerable? check for (propertyIsEnumerable('indexOf') == true). If indexOf was not enumerable, then all of the for..in loops would work fine. Maybe there's a better way to define it so it doesn't come up enumerable?

propertyIsEnumerable has (or had?) cross-browser issues, see Enumeration and Object Oriented JavaScript.

I haven't had time to check if all the browsers supported by the editors/Wysiwyg report the same result for this method nowdays, but I'll look into it ASAP.

Right, TwoD. Sorry, I wasn't saying we should depend on a check for propertyIsEnumerable. I was trying to say, somehow, in IE, it is becoming enumerable. If we could keep indexOf from becoming Enumerable in the first place, it would stop causing a problem with for..in under IE.

Ah, right. I'm afraid enumerability (word?) is determined by the browser. Not much we could do about how it's set either, as this happens in third-party code we can't touch. The problem is also not limited to IE, as noted in the first posts of this issue.

@TwoD - then it seems advisable to use a conventional loop:

for (var i = 0; i < myArray.length; i++)

Instead of

for (var i in myArray)

since .indexOf may end up enumerated, and "for..in" steps over the enumerated properties.

Thank you! The patch in #11 solved the problem for me, which has been plaguing me for months. I had no WYSIWYG in Safari/Chrome, IE 7 & 8. Interestingly, whatever was causing the problem wasn't a problem in IE 9 beta. (or Firefox.)

Patch in #11, with the minor coding style adjustment from #17, committed to all branches.
This seems to fix most, and the easiest, cases of third-party scripts incompatibilities where the changes they make don't affect the editor libraries as well.
If we encounter a situation where editor libraries can cope with a change to global Objects/prototypes implemented in third-party scrips, but Wysiwyg can't, we'll have to deal with that in a more thorough issue. For starters, I think we might have to implement our own $.extend method for cloning settings before passing them to editor wrappers as it doesn't use hasOwnProperty. Or find another way to clone settings without merging in things from object prototypes.
We could also put the items in #19 as a separate task, to investigate the impact of those changes and document our recommendations for plugin authors etc.

Anyway, I think we can mark this one as fixed for now, and deal with the more complex cases when we cross that bridge.