424246 - Non-Firefox browsers should see "Download" rather than "Add to Firefox" button

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

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

I totally suck for not filing this eons ago :-(

For the non-Firefox browser experience, we should display the "Download", rather than "Add to Firefox" button, as Fred did for the non-browser experience over in bug 416541.

Again, sorry for filing this so late; the non-Firefox download experience was at the bottom of my must-check list, but it probably shouldn't have been.

Comment 1

[Michael Morgan [:morgamic]]

:\

Comment 2

[Chris Pollett]

Attachment: Patch for Download to show in non-firefox/seamonkey apps

This patch makes the word Download show-up for user-agents other than Firefox or Seamonkey. I tested Safari 3, Opera 9.5, and IE 7. The slight drawback is that without creating a new l10n string, it says Add to Firefox when using Seamonkey.
Actually, I am a little worried (based on the previously existing code) it might say this for any Seamonkey specific extension (if such exist).

Comment 3

[Chris Pollett]

Attachment: JS version of patch

fwenzel pointed out that UA detection of my previous patch wouldn't work because
of caching of PHP pages. This version solves that problem by giving a JS solution.

Comment 4

[Fred Wenzel [:wenzel]]

Comment on attachment 311460 [details] [diff] [review]
JS version of patch

It looks almost good. However, when you disable Javascript, you see a string containing a %s on the buttons, which shouldn't happen.

Please test your patch with JS on and off, and for different application subpages too, to make sure it works reasonably.

Thanks!

Comment 5

[Fred Wenzel [:wenzel]]

Attachment: Download buttons, JS disabled

This is how the install buttons look with the current patch when JS is disabled.

Comment 6

[Chris Pollett]

Attachment: fixes %s problem

This patch should correct the Download %s with JS off. I tried with a few browsers (Firefox, Safari, IE, Opera, SeaMonkey) with JS on and off and it seemed to work correctly

Comment 7

[Fred Wenzel [:wenzel]]

Comment on attachment 311634 [details] [diff] [review]
fixes %s problem

Yup, this works for me.

Comment 8

[Vlado Valastiak [:wladow]]

This probably doesn't work as expected, because now I see "Download now" button instead of "Add to Firefox" everywhere on preview site

Mozilla/5.0 (Windows; U; Windows NT 6.0; sk; rv:1.9b5pre) Gecko/2008032505 Minefield/3.0b5pre

Comment 9

[Vlado Valastiak [:wladow]]

Seems to be working again, so ignore my previous comment.

Comment 10

[Vlado Valastiak [:wladow]]

What about SeaMonkey? First patch includes SeaMonkey detection, final does not. I think SeaMonkey should have "Add to" button as clicking on "Download" button actually installs the addon.

Comment 11

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

Reopening; see forthcoming screenshots.

Comment 12

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

Attachment: Screenshot of Firefox 2.0.0.12

Here, "Add to SeaMonkey" is visible--so the l10n string already exists--when viewing a SeaMonkey-specific add-on's page in Firefox :-)

Comment 13

[Fred Wenzel [:wenzel]]

Oh, that was my fault: I mistakenly told cpollett, he could just use the Firefox-only regex from elsewhere to do browser detection. That of course, doesn't account for SeaMonkey.

Judging by how a SeaMonkey UA looks:
http://www.useragentstring.com/pages/SeaMonkey/
...just adding SeaMonkey to the list in the user agent pattern should do the trick.

Comment 14

[Chris Pollett]

Attachment: SeaMonkey back patch

This patch should make it say Add to SeaMonkey if you are using SeaMonkey but cruising the Firefox extensions. (Up to no good no doubt!)

Please check the regexp for uapattern . I changed it back to the old value as the new pattern after version 11554 seemed to break this patch.

Comment 15

[Fred Wenzel [:wenzel]]

Comment on attachment 311746 [details] [diff] [review]
SeaMonkey back patch

This may be the wrong doff? Don't see anything about SeaMonkey in there.

Regarding the regex, we shouldn't match for the version string ("2.0.0.1" or so) when we don't need it.

Comment 16

[Chris Pollett]

Attachment: correct patch

Should be the correct patch this time.

Comment 17

[Fred Wenzel [:wenzel]]

Yes the patch is the right one now. What I am confused about is why it shows me "add to SeaMonkey" when I surf the Firefox page now. Is that the expected behavior?

I'd rather expect it to be:
"Download" as the default everywhere, with the exception of:
When you surf the Firefox page with Firefox, you get "add to Firefox". If you surf it with anything else, including SeaMonkey, you still get "Download".
When you surf the SeaMonkey subpage with SeaMonkey, you see "add to SeaMonkey", or "Download" with anything else.

I think you may be overcomplicating your solution by generating an "app_prettynames" array in Javascript.

Comment 18

[Chris Pollett]

Here is what the patch is trying to solve.

There are two types of browsers where clicking the button will attempt to install the extension:
It could be a Firefox browser and a Firefox extension; or
it could be a SeaMonkey browser and a Firefox extension.
Patch then displays "Add to browser_name" since we can't do the
detection in PHP as was pointed out, I need all the names available in JS,
so I make the "app_prettynames" array. The use of "Add to .." is because we are 
attempting to install rather than just download.

For any other type of browser, it should display Download.

Comment 19

[Fred Wenzel [:wenzel]]

Yeah, I understand. But you could just pass the correct "add to X" text to the JS call like you used to.

The use case "SeaMonkey browser on the Firefox subpage sees a Firefox extension" should really not show "Add to SeaMonkey" for two reasons: When you surf AMO with SeaMonkey and you are looking for SeaMonkey add-ons, you should look at the SeaMonkey part of the page, not the Firefox one.

And, more importantly, when you do the Javascript check, you don't even know if the Firefox add-on you are currently looking at is also compatible with SeaMonkey; in fact, many aren't.

Therefore, the button should really show "Download Now" everywhere except for the two distinct cases I described above:
- Firefox browser surfing the Firefox sub-page
- SeaMonkey browser surfing the SeaMonkey sub-page.

I suggest you let the text default to "Download Now" (which your patch does, already), and in JS you just handle these two distinct cases (matches Firefox? yes -> make it "Add to Firefox" or no -> matches SM? yes -> make it "Add to SeaMonkey" and if this is also a no, just don't touch the text at all and return true). Making the texts default to SeaMonkey, then guess Firefox and eventually turning it back to "Download" seems to be much less straightforward.

A last comment (I hope you don't quite hate me yet): In order to avoid checking the .length attribute on a non-matching regex (which morgamic said IE doesn't like), we can just use "if (!re || re.length < 2)" because Javascript evaluates logical operators lazily and thus it won't look at the right part if the front already evaluates to true.

Comment 20

[Chris Pollett]

I think what you are suggesting is essentially the behaviour now without any patch or change applied. I am happy with that provided people are happy that if you are using SeaMonkey but are surfing the Firefox addons page, it says download, but when you click it actually will attempt to install.

Comment 21

[Fred Wenzel [:wenzel]]

Yeah, it's basically the behavior as we have right now with just a little fix, because right now:
- when you surf the SeaMonkey subpage with Firefox you see "add to SeaMonkey" which is wrong.
- and though you see "download" with a SeaMonkey browser on the Firefox page, it tries to install which it shouldn't.

Maybe we should dynamically remove the JS install trigger when we detect the browser to be incompatible with the subpage you are currently on? What do you think?

Comment 22

[Chris Pollett]

This seems reasonble to me. Let me see what I can do.

Comment 23

[Chris Pollett]

Attachment: adds the most recently agreed upon seamonkey behaviour

The just uploaded patch makes so that:
(1) In Seamonkey viewing Firefox extensions says download now and it downloads rather than installs.
(2) In Firefox viewing Seamonkey extensions says download now and it downloads rather than installs

Comment 24

[Fred Wenzel [:wenzel]]

Comment on attachment 311892 [details] [diff] [review]
adds the most recently agreed upon seamonkey behaviour

That works for me. You may want to give the checkMatch... function a less complicated name and a comment describing what it does, but afterwards you may check this in.

Note that this may lead to minor checkin conflicts with the patch I will propose for bug 425326, but I can help figure them out if they occur.

Comment 25

[Chris Pollett]

Okay, I check this in. Thanks for the review wenzel.

Comment 26

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

Verified FIXED using http://preview.addons.mozilla.org:

I tested the following browsers:

* Firefox 2.0.0.13/3.0 beta 5
* Opera 9.27
* Safari 2.01/3.1
* Internet Explorer 6.0/7.0
* SeaMonkey 1.1.9
* Flock 1.1

(Each one on Windows, and Firefox/Safari browsers additionally on Mac/Linux, and Mac, respectively.)

The button was "Download Now" when any of the following conditions was true:

* the app was a non-Firefox/non-SeaMonkey app
* the app was Firefox, but the subpages were Thunderbird, SeaMonkey, or Sunbird
* the app was SeaMonkey, but the subpages were Firefox, Sunbird, or Thunderbird

I tested that "Add to Firefox" appeared when:

* the app was Firefox (or Flock 1.1), and we were on a Firefox-specific page

The button was "Add to SeaMonkey" when:

* the app was SeaMonkey and we were on a SeaMonkey-specific page

A couple caveats:

1) Because of bug 423909, quite a few experimental add-ons display "Log in to install this add-on", but when logged in don't show a button; their buttons, however, correctly display (in the disabled state, of course) their respective "Download Now", "Add to Firefox", or "Add to SeaMonkey" labels; the text is always the same, "Log in to install this add-on", though (probably a separate, but minor bug).
2) re: comment 21, while using SeaMonkey on a Firefox add-on's detail page (like "Cooliris Previews" [https://preview.addons.mozilla.org/en-US/firefox/addon/2207]) spawn our installTrigger code, but I see that on production as well, so I don't think it changed with this patch, and the add-ons install, when they're not experiencing bug 424339 :-(  I also don't know if this is something the SeaMonkey team has been doing with their own iternal handling of XPI MIME types...

I also tested the JavaScript-disabled view, and verified that comment 6 is fixed.

Still, I'm wary, and am sadly likely to have missed _something_; will keep testing and file specific spin-off bugs as warranted.  I don't want issues found in this bug morphing or becoming harder to read/fix/verify :-)

/me passes out now