overlay won't disappear on IE6, errors saying that method is not valid for object

IE6 and 7 do not support the indexOf method on array objects, so the overlay is never removed. The attached patch replaces the indexOf call with an explicit loop through the array.

If anyone can help me patch an alpha5 instance manually (text editor) I would be very grateful. I'd prefer to avoid the standard methods as it's it's been a long time, this is probably just one file and I have limited time. (Is this patch even viable for alpha5?)

Edited to add: Never mind, after studying the structure of the patch file I realized/remembered what the - and + symbols actually meant. :) I can confirm that the patch works here under alpha5 - fixes IE, doesn't break other browsers. One small thing to add to the issue: The gray overlay graphic does not cover the entire viewport. I run a zen fixed layout subtheme and when IE is full-screened, there are a couple of inches of web page visible outside the popup overlay on the left. (This part of the issue may only pertain to alpha5 for all I know).

Thanks, this was a big help. Fixed IE 6 & 7; FF 3 and Chrome are still happy.

Tested and now it works for IE6&7. Thanks.

Patch tested, works great!

This patch also works for me.

Since there is only one instance of the popup in the stack, the patch should break out of the for loop after it is found and spliced. There is no need to keep going down the array.

this patch worked for me as well, thanks.

This patch resolved the issue with IE in my usage as well.

Here's a patch file that will work with the 6.x-2.0-alpha5 release.

Are the committers deliberating on committing this patch?

http://drupal.org/node/199723/committers

User Last commit First commit Commits
starbow 19 weeks ago 1 year ago 337 commits
Takafumi 47 weeks ago 47 weeks ago 2 commits

Alternatively, you could use jQuery's inArray function instead of indexOf. (I didn't create a patch because I've already applied two other patches to popups.js...)

Change the line
Popups.popupStack.splice(Popups.popupStack.indexOf(popup), 1); // Remove popup from stack. Probably should rework into .pop()
to
Popups.popupStack.splice(jQuery.inArray(popup,Popups.popupStack), 1); // Remove popup from stack. Probably should rework into .pop()

w00t. Thanks longwave.

applied patch from post #11 and it worked great.

Here's a patch for the fix in #13.

I'm not sure about the latest patch. Is there any guarantee jquery will be available? I had a quick swizz through the module code and it seems to me full jquery is not necessarily available. I think this would require an additional piece of code to add jquery.js if it's not available already, and adding the whole of jquery.js might not be desired, when longwave's approach works just fine and has no dependencies...?

I'm a +1 for R&TBC for the patch in #11, btw. =)

EXCEPT! The file names at the top of the patch need sorting out - I would post, but don't want to confuse the current discussion point (jQuery or not jQuery?) ^^

I'm reliably informed (by longwave) that jquery is included anyway, so #16 should be cool. I've tested it and it works for me! =)

jQuery is part of Drupal core, and is automatically added to the page whenever drupal_add_js() is called—so yes, it is guaranteed to be available to all scripts in Drupal. Though the patch in #11 works, I think that a simpler solution that leverages jQuery makes more sense.

Oops, cross-posted.

I was unaware that jQuery.inArray() existed when I first wrote the patch in #1, the patch in #16 is functionally the same but much cleaner to read than my version.

I've also sent a message to starbow asking to commit this patch as this issue has been around for six months now.

sub

Patch from #13 has been committed to the 6.x-2.x-dev branch.