Example : go to a node submission / edition form and type a new author for the node.

According to the FireBug extension for Firefox, an error is raised on line 259 of autocomplete.js :
matches[i].split is not a function

(seen on Firefox 1.5, btw. Untested on other browsers)

CommentFileSizeAuthor
#8 autocomplete.js_3.patch1 KByched
#3 autocomplete.js_2.patch1010 bytesyched

Comments

jjeff’s picture

jjeff commented

Interesting. I will look into this.

-Jeff

yched’s picture

yched commented
Title: The module breaks the autocomplete feature » autocomplete broken by Prototype library
Project: S/P Ajax » Drupal core
Version: master » x.y.z
Component: Code » forms system

Changing title and project : In fact, the bug is not related to the s/p ajax module itself, but to the presence of the Prototype library itself.

I'll try to use the JS debugger to see what's happening really...

yched’s picture

yched commented
StatusFileSize
new autocomplete.js_2.patch1010 bytes

Okay, here's what I (sort of) understood :

Prototype adds a lot of methods (iterators, helper functions...) to a lot of objects (including arrays)

A side effect is that when you walk an array with a loop like :

for (i in myArray) {
  aalert(i +' : '+ myArray[i]); 
}

you first catch the "regular' elements of the array, and then you catch these Prototype-added methods.

So the (simplified) code in autocomplete.js :

for (i in matches) {
    matches[i] = matches[i].split('|');
}

raises an error when it calls the (non existing) 'split' method of these extra elements (which are functions and not strings)

I just replaced 

for (i in matches) {

with

for (i=0; i<matches.length; i++) {

I'm no JS expert, so there might be more elegant workarounds, but this does the trick.

(It seems to me that no other drupal js code is impacted, but that might need double-checking)

yched’s picture

yched commented
Status: Active » Needs review

forgot to update the status

markus_petrux’s picture

markus_petrux commented

I was also a "victim" of prototype.js' method of overriding javascript core objects (Array in this case) with one of my phpBB MODs:
http://forums.phpmix.org/viewtopic.php?t=1014

As far as we saw in that topic, the problem seems to affect MSIE only (a browser bug maybe).

I do not use prototype.js myself, but I believe yched's fix would workaround the problem.

yched’s picture

yched commented

Well, the issue I'm dealing with did affect Firefox (1.5/win) - I didn't check for IE or other browsers.

The patch has been tested to work on both FF and IE6.

As it is outlined in the posts you link to, we're not talking about a drupal bug, but rather about allowing the use of a very useful yet quite intrusive external lib.

I'm aware that a 'won't fix' would be tempting for core devs here, Then again, the workaround is so simple...

m3avrck’s picture

m3avrck commented

yched, great find! Although the code needs some stylistic changes, I see no reason this can't be committed. Switching from a foreach() to a for() with a count is identical and IIRC, should be a tad bit faster.

Clean up the spacing in the patch and this one should get in.

yched’s picture

yched commented
StatusFileSize
new autocomplete.js_3.patch1 KB

Patch with clean spaces.

Apparently drupal js files do not currently follow a strict convention on the presence of the "var" keyword in
for (var i = 0; ...

I chose to include one...

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

nicely caught.

markus_petrux’s picture

markus_petrux commented

Just wanted to add (in case prototype.js coders read this) is that they ought to think about not overriding javascript core objects.

Main reason is such technique is intrusive and prone to conflicts. It happens very often that people use a mixture of javascript snippets in their pages, so some could implement different functions with the same name, hence breaking something here or there, sometimes very difficult to catch. Rather than implementing core extensions via javascript prototype they could create standalone functions.

Probably people should not mix different javascript libraries in the same page, however there are lots of little snippets that could still coexist with frameworks such as prototype.js, if non intrusive techniques were used.

Not sure how prototype.js deals with support, but it would be nice to report these conflicts to them too.

killes@www.drop.org’s picture

killes@www.drop.org commented

from the description it sounds to me as if this were a prototype.js bug rather than a Drupal bug. Anyway, I need a voe from our resident JS gurus before committing this.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Reviewed & tested by the community » Fixed

17:07 < chx> killes: it's not just Prototype that fucks this up but various other tools

applied.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)
c960657’s picture

c960657 commented
Version: x.y.z » 5.x-dev

Just for the record: This was broken in #57415: Remove drupal_implode_autocomplete(). The regression is being fixed in #784670: autocomplete.js can iterate over functions in Array objects.