for (var n in userFlags) breaks in older IE because it iterates over the methods of the array as well as the actual data. We should use $.each() instead (patch forthcoming).

CommentFileSizeAuthor
#1 1824374_jquery_each.patch1.46 KBdalin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dalin’s picture

dalin
he/him
Primary language English
Location 🇲🇽, 🇨🇦, 🌍
CreditAttribution: dalin commented
Status: Active » Needs review
FileSize
1824374_jquery_each.patch1.46 KB
joachim’s picture

joachim CreditAttribution: joachim commented

Thanks for the patch!

Would be great to get a review from someone with anon flagging set up.

joachim’s picture

joachim CreditAttribution: joachim commented

Could you explain a bit more about the userFlags problem?

Would this fix #1821942: Adding error handling to JS too?

dalin’s picture

dalin
he/him
Primary language English
Location 🇲🇽, 🇨🇦, 🌍
CreditAttribution: dalin commented

I'm a bit rusty on my understanding of how JavaScript prototypes and such work. But everything in JS is an object, even arrays. And so they have methods. Arrays have a method called 'filter'
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Obj...
So if you do a for loop iterating over the array, you will iterate over the methods as well. The .match() statement obviously fails here and in the case of IE7/8 the script just dies. This is the primary issue that $.each() overcomes.

So in short, yes #1821942: Adding error handling to JS tries to fix the same problem in a different way. IMO $.each() is the better way to go rather than just ignoring all errors.

joachim’s picture

joachim CreditAttribution: joachim commented

> IMO $.each() is the better way to go rather than just ignoring all errors.

I am entirely in agreement with that!

In which case, I'll close that one as a duplicate. Thanks for the explanation.

joachim’s picture

joachim CreditAttribution: joachim commented

I'd really like this patch to get at least one review before I commit it.

joachim’s picture

joachim CreditAttribution: joachim commented
Issue tags: +Needs JavaScript review

Tagging.

MaWebDesigns’s picture

MaWebDesigns CreditAttribution: MaWebDesigns commented
Issue summary: View changes

Not 100% sure if this is related, but my problem is that the flag icons simply never change from unchecked to checked like they do on other browsers (I am using simple ASCII text as my check and unchecked marks). This is true for anonymous or logged-in users.

I tested this patch against 7.x-3.4, and while it does-no-harm it did not solve my problem.

I have no other JavaScript errors that are being reported by my browser, so I'm not sure what my issue with flags is (other than IE8 itself)