[IE7/8] Cannot add a favorite or vote on a collection

VERIFIED FIXED in 5.3

Status

defect
P3
normal
VERIFIED FIXED
10 years ago
3 years ago

People

(Reporter: retornam, Assigned: sancus)

Tracking

unspecified
x86
Windows XP

Details

()

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

10 years ago
STR:
Visit https://preview.addons.mozilla.org/en-US/firefox/collections/editors_picks  whiles logged in
2) Click on any addon to rate 
3) Click on any addons add to favorites button

Expected Results:
1) User is able to add a favorite
2) User is able to rate addon

Actual Results:
1)User is unable to rate an addon / add a favorite

Screencast attached

http://screencast.com/t/AI59QOQg
Priority: -- → P3
Target Milestone: 5.1 → 5.2
Target Milestone: 5.2 → 5.3
Assignee

Updated

10 years ago
Assignee: nobody → sancus
Status: NEW → ASSIGNED
Assignee

Comment 1

10 years ago
Posted patch IE 'Add Favorite' Fix (obsolete) — Splinter Review
I couldn't reproduce the voting not working -- in both IE7/8 it worked fine for me. So either that was randomly fixed, or it's something more subtle than just JS that's incompatible-with-IE.

This patch fixes Add To Favorite, which errored in two places, one because b.css('width') == 'auto' under IE, not a value. The jQuery .width() method seems to work fine under both IE and Firefox, so I just switched to using that. All the other CSS values seem valid under both.
Attachment #409992 - Flags: review?(fwenzel)
Assignee

Updated

10 years ago
OS: Mac OS X → Windows XP
Comment on attachment 409992 [details] [diff] [review]
IE 'Add Favorite' Fix

Looks good!

r55159, thanks, Sancus!
Attachment #409992 - Flags: review?(fwenzel) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Keywords: push-needed
Voting works on IE 7/8 but 'Add to Favorites' does not.

This is the error detail when you click 'Add to Favorites':

Webpage error details

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0)
Timestamp: Fri, 6 Nov 2009 19:18:32 UTC


Message: Invalid argument.
Line: 1
Char: 12974
Code: 0
URI: https://preview.addons.mozilla.org/js/amo2009/amo2009.min.js?55160
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Webpage error details

User Agent: Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30; .NET CLR 3.0.04506.648; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729)
Timestamp: Fri, 6 Nov 2009 19:38:26 UTC


Message: 'JSON' is undefined
Line: 137
Char: 13
Code: 0
URI: http://jbalogh.khan.mozilla.org/amo/site/js/amo2009/collections.js
The bad line is in a function that won't get executed unless we have localStorage, so I guess IE is trying to pre-parse everything and then bailing?
Assignee

Comment 6

10 years ago
Are you getting that error under IE7, IE8, or both? I get a different error in IE8(though still an error), and my Windows VM appears screwed up and IE7 refuses to report errors no matter what I check in Internet Options.

I'll get this re-fixed as soon as I straighten out my VM.
(In reply to comment #6)
> Are you getting that error under IE7, IE8, or both?

Just IE7.  IE8 has native JSON, so there wouldn't be a problem.
Assignee

Comment 8

10 years ago
Posted patch IE 'Add Favorite' Fix v2 (obsolete) — Splinter Review
Couple things here: The whole maintain_width function is replaceable by a jQuery function, so I removed that. The jQuery function comes with the side benefit of not erroring on IE due to NaN css properties, and produces the exact same sum testing on Firefox with Firebug as the function it replaced.

Secondly, the JSON issue seems to be intermittent and only happens when I'm logged in. I guess the recently viewed stuff is trying to function. I put in a couple of checks to make sure JSON is defined before calling methods, as that prevents the whole thing from exploding under IE7. If we want that to actually work properly under IE7 I guess we'd have to provide a JSON implementation for it, but I think that's a bit beyond the scope of this bug.
Attachment #409992 - Attachment is obsolete: true
Attachment #411308 - Flags: review?(fwenzel)
Comment on attachment 411308 [details] [diff] [review]
 IE 'Add Favorite' Fix v2

I like most of this patch, but I don't like that we just don't use JSON when the browser doesn't natively support it -- that means it won't work in Fx2 either.

Instead, we should just import json.js which is the JSON implementation from http://json.org -- if needed, we should probably update it to the latest version -- it'll only load its JSON implementation when it's not supported natively, which is exactly what we want.
Attachment #411308 - Flags: review?(fwenzel) → review-
(it's probably worth mentioning that we already have json.js in our js directory and are using it on the developer pages).
Assignee

Comment 11

10 years ago
OK, I added json.js to any place that depends on collections.js, this should solve the intermittent JSON undefined bugs, and also prevent them anywhere else.

Thanks wenzel!
Attachment #411308 - Attachment is obsolete: true
Attachment #411331 - Flags: review?(fwenzel)
Why is json.js getting packer'd?  That's lame.
Comment on attachment 411331 [details] [diff] [review]
 IE 'Add Favorite' Fix v3

Looks good, but we'll need to double-check once it's on preview -- I am unsure if we need to add it to the build script, too.
Attachment #411331 - Flags: review?(fwenzel) → review+
(In reply to comment #12)
> Why is json.js getting packer'd?  That's lame.

You could go ahead and upload the unpacked version instead, as well as adding it to the build script.
Assignee

Comment 15

10 years ago
I don't think it needs to be added to the build script. Unless I'm completely wrong(which is possible), it looks like there's a default set of .js that is minified, or echo'd explicitly if the site is in a dev state.

The per-controller and per-page jsAdds are still respected regardless of site state, so unless you want to add json.js to ALL pages site-wide(in which case my patch is wrong, and jsAdd is not necessary) then I don't think it belongs in the build script.

The reason it's packer'd is probably because of the above.
Assignee

Comment 16

10 years ago
And in all honesty, this is pretty lame :P
                // TODO: Stick this list somewhere shared with bin/build.py?
                echo $javascript->link('jquery-compressed.js')."\n";
                echo $javascript->link('jquery.cookie.js')."\n";              
                echo $javascript->link('amo2009/global.js')."\n";          
                echo $javascript->link('amo2009/slimbox2.js')."\n";
                echo $javascript->link('amo2009/addons.js');
                echo $javascript->link('amo2009/global-mozilla.js')."\n";
                echo $javascript->link('amo2009/install-button.js')."\n";    
                echo $javascript->link('jquery-ui/jqModal.js')."\n";
r55873.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.