393263 - Download link on front page does not redirect for Safari and IE

Status: VERIFIED FIXED

(www.mozilla.org :: General, defect)

Description

[Michael Morgan [:morgamic]]

Expected:
- user clicks on link
- user gets download plus redirect to download page

Seen:
- user clicks link and gets download
- download page does not pop up

Browsers in question:
- Safari (all versions) for OSX and Win
- IE6 & IE7 (this was known, but would be ideal to have a workaround -- see flock.com or opera site)

Comment 1

[Martijn Wargers (dead)]

Isn't it perhaps possible to use a timer for this? I mean the link gives the url of the to downloaded file and the onclick handler gives a timer that redirects the user to the appropriate page.

Flock seems to open a window on onclick handler in IE (do_download). The url of that window is the to downloading file. Other browsers just get redirected and the to downloaded file is given there.

Comment 2

[John Slater]

Hey guys...check out the DHTML overlay that pops up when you click on a screen shot here:
https://addons.mozilla.org/en-US/firefox/addon/3633

Would something like this work as a workaround for this IE issue?

Comment 3

[Paul Kim]

Following on from jslater's comment, goal is to display the download and install instructions we've created for IE6/IE7 users. The overlay on AMO is at: https://addons.mozilla.org/en-US/firefox/addons/previews/3633

Comment 4

[Mike Connor [:mconnor]]

Can't you just redirect to the download page (without the download trigger active) on a timeout?  i.e. <a href="path/to/download" onclick="setTimeout(window.location.href='path/to/download/page', 1000)">download</a>

I don't have the source (or IE) handy, but this should work, and its a legitimate JS redirect to a page, but the downlaod is still from a direct link and thus shouldn't raise flags.  We could do this for everything, really, and avoid the JS-invoked download on the next page, but I guess there's something noscript involved there...

Comment 5

[Steven Garrity [:sgarrity]]

Are you sure this is a problem in Safari? There's another bug having to do with a delay/hang in Safari (Bug #393265), but I still see the download "thanks" page in safari. As far as I knew, this was an IE problem only.

Mike, wouldn't the solution you propose above doesn't work because the browser seems to stop dealing with JS from the current page as soon as the link to another page has been clicked.

The only solution I've seen work is a small popup-window that initiates the download. Both Flock and Download.com use this method for IE.

Comment 6

[Michael Morgan [:morgamic]]

Version 2.0.4 (419.3)

When I'm on the front page and click download, Safari doesn't see the download page, just starts the download and that's it.

Comment 7

[Steven Garrity [:sgarrity]]

I think that behaviour still might  fall under Bug #393265. One report said that if you resized the browser windows, you'd then see the Thank You page. It was as though the page was loading, but not displaying until the windows size changed.

Comment 8

[John Slater]

Hi all. I just wanted to jump back in and check up on the status of things here. This issue has now come up at the directors meeting and making this page viewable for IE users is a pretty high priority for a lot of people.

So, what's the path for figuring out and implementing a solution to this?

Thanks,
John

Comment 9

[Michael Morgan [:morgamic]]

I can work on using the pop-up window scenario, which might alleviate both issues.  Should have something tonight or tomorrow.

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

<a href="link/to/file" onblur="link/to/download/page"> works in IE (not in Safari which only downloads the file), test page here :
http://www.chevrel.org/temp/tele.html

building the page in javascript is a bad idea for localization of the download page, using an overlay built with DOM changes would make it even more complex.

Comment 11

[Steven Garrity [:sgarrity]]

Pascal, I really like this solution since the basic (non-javascript) behaviour is just a basic default HTML link to the installer. I also like that the download starts BEFORE you go to the "your download will start shortly" page.

If we could figure out a way to get this to work in Safari, I think it could be quite promising.

Comment 12

[Michael Morgan [:morgamic]]

Attachment: v1, changes to en-US to use IE7/6 workaround

Comment 13

[Michael Morgan [:morgamic]]

Comment on attachment 278434 [details] [diff] [review]
v1, changes to en-US to use IE7/6 workaround

Wil let me know that my urchin change breaks stats.  That was an ancillary change and I can move the urchin call out of do_download() rest of the stuff should fix the IE/Safari redirect issues though.  See if it works for you.

Comment 14

[Reed Loden [:reed]]

Comment on attachment 278434 [details] [diff] [review]
v1, changes to en-US to use IE7/6 workaround

 	function initDownload()
 	{
+        urchinTracker('/firefox/download/0706abtest/?mozhpcontrol');

Considering this page is used by other pages besides en-US/index.html, I would think this would cause duplicate urchin stats to be created, as there would be one from this file and one from the link going to this page (since you only removed the urchinTracker() from en-US/index.html).

-		// 5. automatically start the download of the file at the constructed download.mozilla.org URL
-		window.location = download_url;
+        // 5. automatically start the download of the file at the constructed download.mozilla.org URL
+        window.location = download_url;

Nit: Add some more spaces here to indent the code rather than have it be left-aligned.
 
-	if (download_url.length != 0)
-		window.onload = downloadURL;
+	if (download_url.length != 0) 
+    {
+        var appVer = navigator.appVersion.toLowerCase();
+        if(appVer.indexOf('msie') == -1) 

There's no need to create a variable for a one time use here. Please use:
  if (navigator.appVersion.toLowerCase().indexOf('msie') == -1)

Also, is there a reason why you need the toLowerCase()? Seems like this would work fine:
  if (navigator.appVersion.indexOf('MSIE') == -1) 

Nit: space after the |if| before the '(' and line up the starting bracket '{' with the |if| in the first block.

 // Sending everything to bouncer v1 for now
 function getDownloadURLForProduct(product, version)
 {
-  var BOUNCER1_URL = "http://download.mozilla.org/?product=";
-  // var BOUNCER2_URL = "http://download2.mozilla.org/?product=";
+  var BOUNCER_URL = "http://download.mozilla.org/?product=";
 
-  return BOUNCER1_URL;
+  return BOUNCER_URL;
 }

Is there a particular reason for this change? I'm not opposed to it, but if you're going to get rid of everything concerning non-bouncer v1 urls, please remove the comment before the function.

-  // If we are testing the site locally, give the direct download URL.

Why are you removing this comment?
 
+function getBouncerURLForLanguage(aLangID, aPlatform)

Most of this entire function is unnecessary and causes duplicate code. If some way around having this function can't be found, the code between getDownloadURLForLanguage() and getBouncerURLForLanguage() should be split out so nothing is the same in each of them.

I see why you're using both getDownloadURLForLanguage() and getBouncerURLForLanguage(), but I think it would be pretty easy to add a parameter to getDownloadURLForLanguage() to make it just ignore the MSIE checks and return the products/download.html page.

+  item = item.replace(/%BOUNCER_URL%/g,   getBouncerURLForLanguage(aLanguageID, gPlatform));

Actually, you probably don't need this at all. You can call whatever function you need from do_download() to get the link instead of using the replace stuff to replace it in a weird way.

+function do_download(link){
+    urchinTracker('/firefox/download/0706abtest/?mozhpcontrol');

urchinTracker() is duplicated here (it was added to initDownload()).

+    var appVer = navigator.appVersion.toLowerCase();
+    // If we have IE, use a new window to push the download.
+    // Warning: CheesyHack++ -- do not touch, hold or stare at CheesyHack.
+    if(appVer.indexOf('msie') != -1){

Same note as above about the one-time-only appVer variable and toLowerCase().

+        window.open(link, 'download_window', 'toolbar=0,location=no,directories=0,status=0,scrollbars=0,resizeable=0,width=1,height=1,top=0,left=0');
+        window.focus();

Could this possibly go as an else path where you made |window.onload| not work for IE, or does it have to be in a separate function so that onclick() can call it? I haven't tested with IE, so I don't know what exactly will work with it. Just a thought...

Thanks for working on this!

Comment 15

[Michael Morgan [:morgamic]]

Thanks for the review, reed.  The urchin stuff I was planning on backing out -- that was from bug 393265 and was trying to remove it in order to see if it fixed the Safari hang -- it crept into this patch.

Will repost the following: a) improved prod tag w/ tweaks mentioned above b) patch for trunk for when we figure out the use-case for locale d/l links

Comment 16

[Michael Morgan [:morgamic]]

Attachment: v2, all links fixed plus urchin left untouched, for prod tag

Has fixes mentioned above.  Did not care to adjust %BOUNCER_URL%, it's how the other vars are done so I am using the same convention.  Also the duped code in the two funcs could be the same function, but I don't want to have to change callback for the existing function so I am okay with just adding a new one (re: getBouncerURLForLanguage()).  If anything the platform if else could be a function, but I'd rather leave it as-is because it's simple enough and again I don't want to make any unnecessary changes.

Comment 17

[Michael Morgan [:morgamic]]

Comment on attachment 278612 [details] [diff] [review]
v2, all links fixed plus urchin left untouched, for prod tag

Looks like people were using tabs in download.html -- I can fix the indent.  >:|

Comment 18

[Wil Clouser [:clouserw]]

Comment on attachment 278612 [details] [diff] [review]
v2, all links fixed plus urchin left untouched, for prod tag

Logically it looks correct.  I don't have the ability to test in all our required browsers yet.

Comment 19

[Michael Morgan [:morgamic]]

Attachment: Adjustment, adding location.href to try to fix safari bug

Comment 20

[Michael Morgan [:morgamic]]

Attachment: Additional change to download init to allow for Safari page rendering

Sorry for the merry go round on patches/checkins.  Trying to get a fix and working on testing w/ Stephen.

Comment 21

[Michael Morgan [:morgamic]]

Attachment: v1, patch against trunk containing all changes

Comment 22

[Michael Morgan [:morgamic]]

Note that I dropped the window.location.href = foobar because it as not the fix for Safari.  Also note that by using a setTimeout() call for downloadURL we fix the Safari issue but we could be making the page not completely render for people with really slow connections (if they take longer than 1.8s to load).

The NS has this page cached, though -- so unless they are on a really slow connection it shouldn't be a problem... but we should decide if this is an issue for UX... ?

Comment 23

[Wil Clouser [:clouserw]]

Comment on attachment 278879 [details] [diff] [review]
v1, patch against trunk containing all changes

Thanks for the patch morgamic - I think it works great for the pages that are patched.  There are a few more pages that have download buttons on them though, which need the additional code:

 grep -rsl "download.old.js" * | grep -v ".svn" | grep -v "en-US"
en/index.html
en/launch/index.html
en/thunderbird/index.html
en/firefox/pgbffx/index.html
en/firefox/index.html
en/firefox/flicks/index.html
en/firefox/pgdlong/index.html
en/firefox/features.html
en/firefox/phase2/exec/index.html
en/firefox/phase2/skater/index.html
en/firefox/phase2/dragdrop/index.html
en/firefox/phase2/family/index.html
en/firefox/landing/IE/index.html
en/firefox/landing/vista/index.html
en/firefox/landing/better/index.html
en/firefox/pgcshort/index.html
en/firefox/pganonffx/index.html

Thanks!

Comment 24

[Reed Loden [:reed]]

Comment on attachment 278879 [details] [diff] [review]
v1, patch against trunk containing all changes

r- on the fact that you have the duplicate code. I was going to allow that for temp on prod tag, but I don't want it at all on trunk.

Comment 25

[Michael Morgan [:morgamic]]

(In reply to comment #24)
> (From update of attachment 278879 [details] [diff] [review])
> r- on the fact that you have the duplicate code. I was going to allow that for
> temp on prod tag, but I don't want it at all on trunk.
Whatever.

Comment 26

[Michael Morgan [:morgamic]]

Attachment: v2, included missing files and removed dupe code >:\

Comment 27

[Reed Loden [:reed]]

Comment on attachment 278923 [details] [diff] [review]
v2, included missing files and removed dupe code >:\

>+function getBouncerURLForLanguage(aLangID, aPlatform)

As long as you don't forget to remove this (now unused) function before your check-in, r+. Thanks for the hard work!

Comment 28

[Michael Morgan [:morgamic]]

Yeah, good catch. I dropped the extraneous function and checked it in.

Paul can you see my comment in comment #22 about UX?  I'd like some feedback on that.

Comment 29

[Paul Kim]

Hey Mike -- what qualifies as a "really slow connection"? Is it the equiv. of a dialup connection? If so, and given this is for Safari users only, this feels acceptable to me to push live. (If I'm understanding you correctly the timeout issue will only affect Safari users on a dialup connection, which I would guess is a really small #).

Comment 30

[Steven Garrity [:sgarrity]]

If "slow" is defined by taking more than 1.8s to load the page (including graphics), then that's a lot of people. Also, is this issue for Safari only, or for everyone?

Comment 31

[Michael Morgan [:morgamic]]

Yes, Steven, that is why I brought it up.  The 1.8s covers all users that are not IE.

I can add more hackery to make the setTimeout() work only for Safari and use the original onload for Firefox users or increase the setTimeout() to 3-4 seconds.

Which one would you guys prefer?

Comment 32

[Michael Morgan [:morgamic]]

Attachment: Patch to increase Safari delay to 2.5s and make setTimeout() Safari-only.

Paul, this patch would:
- make the delay only happen for Safari users and increase that to 2.5s
- revert to the onload method for non-Safari and non-IE users

Comment 33

[David Rolnitzky]

I think that this is the right play -- to add the setTimeout only for Safari.

Comment 34

[Stephen Donner [:stephend] Not actively reading bugmail]

Please let QA know when the final, approved changes have landed on staging, and we'll begin our testing.  Thanks!

Comment 35

[Michael Morgan [:morgamic]]

The last adjustment should be in on trunk.  So you'd be testing:
https://www.trunk.stage.mozilla.com/en-US/

Wil are you planning on merging trunk at this point?  If so they can test on staging but same difference as far as I'm concerned.  Up to you.

Comment 36

[Paul Kim]

(In reply to comment #32)
> Paul, this patch would:
> - make the delay only happen for Safari users and increase that to 2.5s
> - revert to the onload method for non-Safari and non-IE users

+1 

Comment 37

[Wil Clouser [:clouserw]]

> Wil are you planning on merging trunk at this point?  If so they can test on
> staging but same difference as far as I'm concerned.  Up to you.
> 

Reed is going to do it this afternoon.

Comment 38

[Stephen Donner [:stephend] Not actively reading bugmail]

Just for the record, QA has signed off and we're awaiting the push.

Comment 39

[Paul Kim]

bug filed to sync with IT: https://bugzilla.mozilla.org/show_bug.cgi?id=394532

Comment 40

[Michael Morgan [:morgamic]]

Comment on attachment 278979 [details] [diff] [review]
Patch to increase Safari delay to 2.5s and make setTimeout() Safari-only.

This was pushed based on IRC discussions.

Comment 41

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED; this has been working in production for quite a while now.