320504 - Prompt users to submit feedback when uninstalling FF

Status: RESOLVED FIXED

(Firefox :: Installer, enhancement)

Description

[Darin Fisher]

When users choose to uninstall FF, it would be nice if we prompted them to optionally send us some feedback as to their reasons for uninstalling FF.

This could provide a source of useful data to help us better understand what
we should be most focusing on to improve FF.

This should be something extremely lightweight that tries not to get in the 
way of users too much.  If a user chooses to submit feedback, then we will
just fire up Internet Explorer and point them at a form on Mozilla's website.

Comment 1

[Daniel Veditz [:dveditz]]

Might consider for a later dot-release if there was a tested patch.

Comment 2

[Darin Fisher]

Attachment: v1 patch

This patch implements a simple MessageBox prompt that notifies the user that the uninstall process is complete and asks them provide feedback to help us make FF/TB better.  If they agree to do so (which is the default dialog action), then we find iexplore.exe and launch it with the following URL:

  https://survey.mozilla.com/1/$productname$/$useragent/exit.html

The variables are taken from uninstall.ini and the "/ua" command line flag.  So, for example, the URL looks like the following when run from FF 1.5:

  https://survey.mozilla.com/1/Mozilla%20Firefox/1.5%20(en-US)/exit.html

This URL is not as clean as the AUS URL, but it is sufficient, and it is easy to generate from the uninstaller.

Comment 3

[Darin Fisher]

A couple comments about the survey URL:

  -  It uses https:// to give people warm fuzzies about privacy and such.
  -  The path begins with "/1/" as a version field for the rest of the URL,
     which is similar to what we adopted for the AUS URL.
  -  The keyword "exit.html" is appended to indicate that we're interested
     in showing the user an exit survey.  Note: we may have other surveys
     in the future (e.g., inactivity survey).
  -  It does not bother to include the OS since we know the request is only
     for Windows users.  If we want more detailed info about the Windows
     version, then we could probably just inspect IE's UA string.

Comment 4

[James Ross]

If the uninstall has finished, the setup program will have correctly restored the user's previous default browser settings (right?), so why not just launch the URL directly? If you're really paranoid, you could do it with IE explicitly after the direct launch fails.

Comment 5

[Darin Fisher]

> If the uninstall has finished, the setup program will have correctly restored
> the user's previous default browser settings (right?), so why not just launch
> the URL directly? If you're really paranoid, you could do it with IE explicitly
> after the direct launch fails.

One would think so, but I couldn't find the place where the uninstaller restores the default protocol associations, so I decided to manually invoke IE.

Comment 6

[Benjamin Smedberg]

Comment on attachment 208067 [details] [diff] [review]
v1 patch

>Index: browser/installer/windows/uninstall.it

>+; Localize me!!
>+MSG_EXIT_SURVEY=@BRAND_PRODUCT_NAME@ has been uninstalled.  Please provide us with your feedback to assist us in our efforts to make @BRAND_PRODUCT_NAME@ better.

I'd like beltzner to review this string and the general concept before checking the uninstall.it changes in. I personally think this whole idea sounds like hanging on too long after a breakup.

>+  GetPrivateProfileString("Messages", "MSG_EXIT_SURVEY", "", buf, sizeof(buf),
>+                          szFileIniUninstall);

result-check this and bail early please if the while message is not present.

>+  /* launch IE, and point it at the survey URL (whitespace in the product name
>+   * or user agent is okay) */

Please add an XXXcomment explaning why we don't launch the default http/https handler.

Comment 7

[Darin Fisher]

OK, I landed just the code changes w/ the checks to disable the dialog and iexplore execution if the MSG_EXIT_SURVEY string is not defined in uninstall.ini.

Comment 8

[Darin Fisher]

I'll post a follow-up patch that includes just the uninstall.it changes.

Comment 9

[Darin Fisher]

Attachment: uninstall survey text

Comment 10

[Daniel Cater]

Could this use http://feedback.mozilla.org/ (http://hendrix.mozilla.org/) in some way? Unfortunately https://feedback.mozilla.org/ and https://hendrix.mozilla.org/ don't work as expected...

I'm also wondering about default browser settings. Something somewhere must change the default browser, otherwise opening links from external applications after uninstalling wouldn't work?

(Removed dumbass comment regarding Mac and Linux... d'oh)

Comment 11

[Darin Fisher]

> I'm also wondering about default browser settings. Something somewhere must
> change the default browser, otherwise opening links from external applications
> after uninstalling wouldn't work?

That's right.  It's a bug with our uninstaller.  I just verified the bug by uninstalling FF 1.5.  Sure enough, my registry was left with entries pointing to FF 1.5's old location for http:// and https:// links.

Comment 12

[Mike Schroepfer]

Can we get the locale encoded in the URL so we can localize the survey as appropriate?

Comment 13

[Darin Fisher]

> Can we get the locale encoded in the URL so we can localize the survey as
> appropriate?

it is.  see comment #2.

Comment 14

[Mike Schroepfer]

Thanks - (sorry for missing comment #2) 

Comment 15

[Mike Schroepfer]

CC'ing axel since there is possible l10n impact.

Darin - can we case the code out so the prompt only runs on en-us?  If so we can avoid localization changes (which we promised would not occur in the 1.5.0.x branches)?

Comment 16

[Darin Fisher]

Yes, I can easily make this en-US only if that is desired.  Just let me know how you want it to be.

Comment 17

[Mike Schroepfer]

Yes - let's do en-us only to start to avoid localization isseues (realizing we won't get data from outside the us either).   

Beltzner can we get your review on the prompt?

Comment 18

[Benjamin Smedberg]

In 1.5 the uninstaller is always in English (it is not localizable), so we really can't make this en-US only (or I don't see a good way to do this) because it is the exact same file for all locales.

Comment 19

[Darin Fisher]

(In reply to comment #18)
> In 1.5 the uninstaller is always in English (it is not localizable), so we
> really can't make this en-US only (or I don't see a good way to do this)
> because it is the exact same file for all locales.

bsmedberg: don't we set the "/ua" command line switch w/ the proper locale in localized builds?  it looks like ab-CD.jst is responsible for writing the registry keys for the add/remove programs menu, which affect the "/ua" command line switch.   Or, am I mistaken?

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 208112 [details] [diff] [review]
uninstall survey text

(In reply to comment #6)
> I'd like beltzner to review this string and the general concept before 
> checking the uninstall.it changes in. I personally think this whole idea 
> sounds like hanging on too long after a breakup.

I'll happily revoice the concern that uninstall surveys skew the data in two fairly predictable ways based on the fact that the people who take them have a) elected to take them, which skews slightly to the angriest of users, and b) have bothered to uninstall software on Windows, which to the best of my knowledge isn't that common an action in these days of mega disks and indicates a user with a decent amount of computer knowledge. But that's more about how we interpret the data.

As currently expressed, I agree that the request for feedback does feel a little clingy. The aim should be for this dialog to not be seen as a "nag"; at no point should any user think in their mind "I said go away, Firefox, stop asking me questions". I'd suggest that we make the dialog a simple "OK" button dialog, and put the request for feedback as a checkbox or link inside it.

So:

 ,--------------------------------------------------.
 |                                                  |
 | Firefox 1.5 has been removed from your computer. |
 |                                                  |
 | [ ] Tell us what you thought of Firefox          |
 |                                                  |
 |                   [ OK ]                         |
 '--------------------------------------------------'
 
or

 ,--------------------------------------------------.
 |                                                  |
 | Firefox 1.5 has been removed from your computer. |
 |                                                  |
 | _Let us know what you thought of Firefox_        |
 |                                                  |
 |                    [ OK ]                        |
 '--------------------------------------------------'

This makes the dialog primarily informative (confirming the uninstall task) and allows a feedback route for those inclined to take it.

Even if we don't go this route, I'd suggest the following rephrasing to the string:

MSG_EXIT_SURVEY=@BRAND_PRODUCT_NAME@ has been removed from your computer. Would you like to fill out a short survey to tell us what you thought of @BRAND_PRODUCT_NAME@?

Finally, I think (if I'm reading the code right) you're using an OK/CANCEL box here, and it should be a [YES]/[NO] choice, instead.

Comment 21

[James Ross]

FWIW, I don't think you can do anything more complex that text in a standard message box, so putting a checkbox or link in one would require creating a dialog template and so on, and you'd probably miss some of the standard features of a message box (like XUL ones currently do :( ). Not to say you shouldn't, but it *is* a lot of work to do a proper MessageBox replacement.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

also... why not just ShellExecute the URL? that avoids having to find where iexplore is, and they may actually have a different browser installed (afaik we restore the settings to the previous ones, so they might have used e.g. opera or something)

Comment 23

[James Ross]

biesi, see comment 4, comment 5 and comment 11.

Comment 24

[Darin Fisher]

Mike: Thanks for taking the time to suggest better UI.  I agree with your suggestions.  I also agree with James' point about your UI being more complex to implement.  That doesn't mean that it can't be done, of course ;-)

As for the exit survey being a skewed data source, I agree.  However, we have other skewed data sources as well, such as the broken website reporter (how discoverable is that anyways?), and yet it proves to be useful... right?

I have also considered prompting the user to take a survey if they have not used FF in over a month or so... thought, that sort of prompt is much more likely to raise objections -- I'm sure -- for being obnoxious.  The point being that the exit survey need not be our only means of prompting the user for feedback.

Comment 25

[Robert Strong (they/them - no direct email)]

Attachment: screenshot - NSIS uninstall finish page

I've been investigating the use of NSIS for the Win32 installer and built a basic installer this weekend. Here is a screenshot of a finish page that implements this.

Comment 26

[Darin Fisher]

> screenshot - NSIS uninstall finish page

Rob, that looks great!  Let's get in sync with respect to this uninstaller work.  I am still very interested in getting this into FF 1.5.0.x (hopefully 1.5.0.2) as I think it will help provide some useful data for the 2.0 product cycle.  Is your new uninstaller wizard planned for 2.0?

Comment 27

[Benjamin Smedberg]

Yes, the new uninstaller will match the new installer which is targeted at ff2.

Comment 28

[Darin Fisher]

Attachment: v2 patch

OK, this implements beltzner's suggested UI.

Comment 29

[Darin Fisher]

Attachment: screenshot for v2 patch

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #24)
> I have also considered prompting the user to take a survey if they have not
> used FF in over a month or so... thought, that sort of prompt is much more
> likely to raise objections -- I'm sure -- for being obnoxious.  The point 

I would be one of the first people to raise that point. Really what you want to do here is file a bug on Windows asking them to point out to people that the software they installed several years ago is rusting or something, and should really be removed ;)

But I do agree that some data is better than no data; just wanted to make sure that we're aware of the statistical significance, &c.

(In reply to comment #29)
> Created an attachment (id=209481) [edit]
> screenshot for v2 patch

Ugh, so can I nitpick my own design? :)

 * the checkbox and string should be indented a little bit, I think
 * s/Tell/Fill out a short survey to tell/

with those nits, ui-review=beltzner

Comment 31

[Darin Fisher]

> Ugh, so can I nitpick my own design? :)
> 
>  * the checkbox and string should be indented a little bit, I think
>  * s/Tell/Fill out a short survey to tell/

Yeah, I know it's an ugly dialog.  Using raw Win32 UI primitives is quite tedious ;-)

I'll make those suggested changes.  It will result in the checkbox message being longer than the main dialog text.  That means that indenting the checkbox a bit will result in a stair-case look to the two lines of text.  Is that okay, or do you have suggestions for how to present it in a better way?

Comment 32

[Darin Fisher]

I think I'd like to keep the shorter text for the checkbox.  Aesthetically, it fits better into the dialog.  Also, it seems clear enough to me that it results in further action, and what does it really add to say that the next thing will specifically be a survey?

I went ahead and made the indentation change locally (4 pixels as that seemed consistent with other dialogs on my system).

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Yeah, OK, sold. Keep the shorter text. :)

Comment 34

[Darin Fisher]

fixed-on-trunk

Comment 35

[Darin Fisher]

Comment on attachment 209479 [details] [diff] [review]
v2 patch

The patch for this bug is broken into two pieces.  We'd need both pieces for a FF 1.5.0.2 release.

Comment 36

[RC]

If you really want feedback, how about doing what google toolbar does?  (Uninstaller uninstalls the app immediately.  You are then sent to an feedback form on google's site that asks something similar to: "What is the primary reason you uninstalled?", with a few choices, a textbox, and a submit button.  Very simple, guarantees a much higher rate of feedback, and the user can close the browser window if he doesn't want to say anything.

Comment 37

[Darin Fisher]

Attachment: fix for trunk (use deerpark name) [fixed-on-trunk]

This patch is only needed on the trunk.

Comment 38

[Axel Hecht [:Pike]]

I'm tempted to back this out.

I have no idea how to fix the l10n impact of this.

#if in l10n is evil, I'm tempted to make it verboten. This bug is not fixed until
someone comes up with a way to reliably fix the l10n build process, see bug 325924
Having two different keys sounds like a much more reliable way to go and #ifdef in
the calling site. (Shouldn't there be a better thing than to spread an old code
name all over the place, too?)
((using ah@numerik is completly pointless, filed bug 325997))

Comment 39

[Darin Fisher]

Axel: This was done explicitly for en-US only.  We are going to have an entire new installer and uninstaller for FF2, so this patch was just a crutch to help us get additional feedback from the FF 1.5 release.  Benjamin advised me not to try to make any string changes to FF 1.5, so that meant that the only thing I could do is restrict this to en-US.  Also, please note that the current uninstaller that ships with FF 1.5 is not localized.

Comment 40

[Benjamin Smedberg]

Actually this is not limited to en-US, it ships in all locales with English strings.

Comment 41

[Axel Hecht [:Pike]]

Comment on attachment 210235 [details] [diff] [review]
fix for trunk (use deerpark name) [fixed-on-trunk]

This patch was apparently commited on the trunk, and is burning all l10n boxens.

This is Firefox 3, right? I suggest to remove the #ifdef stuff from the l10n repository, it's dead-to-be code, it's wrong, and currently it just sets a bad example.

Comment 42

[Benjamin Smedberg]

This is not ff3 (or even ff2) code, it is targeted at ff1.5.0.2 or 1.5.0.3

#if in l10n is not evil per-se, as long as we use it correctly. The ifdef hacks in place are merely there to make sure that official and unofficial branding both work correctly.

Comment 43

[Axel Hecht [:Pike]]

Odd. If this is only for 1.5.0.x, how comes that stuff landed on the trunk?
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=uninstaller.inc&branch=&root=/cvsroot&subdir=mozilla/browser/locales/en-US/installer&command=DIFF_FRAMESET&rev1=1.2&rev2=1.3

And how comes that the tinderboxens pick it up? Right, the files doesn't say
that it must not be localized. Benjamin, you wanted l10n on the trunk, keep it
up.
Please fix the bustages for pl et al who actually added that file, see
http://lxr.mozilla.org/l10n/find?string=uninstaller.inc.

Note, I don't think that #if's have a use case that doesn't break the all
toolchains.
And I certainly won't approve those #ifs in the way they're used here, too.
Product names are trademarks relevant, and the stuff that landed on the trunk
would require QA to verify that string in each locale. I don't see that
happening.

Comment 44

[Benjamin Smedberg]

This was landed on trunk because we obviously need a place to test it before landing it on a stable branch. It doesn't matter if the file is localized because the localized version will not be used (see my original comments about this being in English in all builds). It is perfectly fine if you want to make compare-locales ignore the uninstall file for the time being. Having the uninstaller be English-only preserves the status quo on the branch.

Robstrong is working on the new installer architecture for ff2/ff3. That installer will be fully localizable (and will probably be multi-locale).

Comment 45

[Axel Hecht [:Pike]]

I have more important things on my toolchain work than fixing abuse of build 
foo and testdrives.

If you make me fix it, I'll back out the #if stuff in l10b and be done with it.

Comment 46

[Benjamin Smedberg]

Pike, I appreciate you're busy but there's no reason to threaten me. This is a properly reviewed checkin and is not to be backed out. Removing the (unused) uninstaller.inc from pl and whatever other locales is a better solution that does not break the uninstaller on trunk.

Comment 47

[Benjamin Smedberg]

Patch committed to ignore "uninstaller.inc" in compare-locales.pl

Comment 48

[Axel Hecht [:Pike]]

Note that I don't consider this fixed.

My current assessement is that I can't keep l10n alive on the trunk. 
I don't see any information on where the l10n part of this stuff is supposed to
go, and thus there is no way for me to help, neither design nor communication 
wise. There aren't comments in the source, I didn't see any bugmail nor posts in
.l10n nor anything.

I will continue to work on reducing the complexity of l10n and the patch that
landed here is a kick in the crotch for that. I'll just forget about this stuff
for now and bother fixing the mess once I have to look at the trunk.

Comment 49

[Darin Fisher]

Hey Axel, I really don't have any desire to leave l10n high-and-dry here.  Frankly, I wasn't aware of the l10n processes, and given that I thought I wasn't introducing any regression (based on the fact that the prior uninstaller is not localized), I figured that my change was not something that l10n folks would need to worry about.  I really apologize for causing any problems with the l10n trunk, and I want to work to fix those problems.  Is there a document somewhere that you can point me to that describes the main issues to watch out for when making changes that impact l10n.  For instance, I didn't know that #ifdef's in uninstaller.inc would be considered bad.  I also didn't know that I should be watching a special tinderbox for l10n bustage.  Thanks!

Comment 50

[Darin Fisher]

Attachment: branch patch (including fix for bug 307296)

This is the complete patch that would be needed for the 1.8.0 branch to present the exit survey to FF 1.5.0.x users.  It is what I landed on the 1.8 branch, and it contains the fix for bug 307296 as well as a couple minor follow-up fixes attached elsewhere.

Comment 51

[Darin Fisher]

fixed1.8.1

Comment 52

[Axel Hecht [:Pike]]

Comment on attachment 211301 [details] [diff] [review]
branch patch (including fix for bug 307296)

I suggest a- for this, this will turn all our l10n builds non-green.
I'm not fond of disabling our build foo for a release, either.

Darin, there is no document for not-breaking l10n so far, this is likely as other stuff, you find that out by doing it.

Bad thing here is duplicate keys, as any tool but the build process is going to ignore the preprocessor instructions.

I don't see this being ready for 0.2, we should fix trunk and 1.8 branch before landing this for a release.

Comment 53

[Darin Fisher]

Axel: What tools are you talking about?  I really don't understand what needs to be done differently here?  Would it help you if I moved uninstaller.inc to a different location in the tree?  Right now, it is not included in ab-CD.xpi, it is not even touched by a make under browser/locales.  Remember that the uninstaller in FF 1.5 is only in english too.  So, please... explain what it is that I need to do to prevent the tinderboxen from freaking out over this change.

Comment 54

[Darin Fisher]

I checked in the same compare-localse.pl change on the 1.8 branch w/ r+a=bsmedberg, which is apparently needed to prevent the locale system from bitching about the totally benign uninstaller.inc :-)

I will revise the 1.8.0 branch patch to include that patch.

Comment 55

[Darin Fisher]

Attachment: revised branch patch (including fix for bug 307296)

OK, now this version is ready for 1.5.0.2

Comment 56

[Axel Hecht [:Pike]]

Sorry, but the stuff that bsmedberg did is a hack that I only accept as a 
temporary measure.

Moving that file out of en-US is one solution that I can live with, if it's not
supposed to be localized, don't put it into locales/en-US.

"Tools" is anything listed (or not) on http://wiki.mozilla.org/L10n:Tools, 
including hacks like converting back and forth to po files and other stuff.

Comment 57

[Darin Fisher]

> Sorry, but the stuff that bsmedberg did is a hack that I only accept as a 
> temporary measure.

Yup, he agrees that it is a hack.  Why is that hack not acceptable for the 1.8.0 branch?  Remember, we are getting a new fancy installer+uninstaller for FF2, so there isn't much to be gained from localizing or making _this_ solution all nice and pretty right now.  We just need something that works for FF 1.5.0.2 that doesn't regress anything and is easy to test/verify.  The current solution seems to fit the bill.


> Moving that file out of en-US is one solution that I can live with, if it's 
> not supposed to be localized, don't put it into locales/en-US.

We decided to put this file into locales/en-US in the off chance that we would need to localize it.  Given the current hackish workaround, does it really buy us anything to move the file around at this point?  Again, we're only talking about a hack for FF 1.5.0.2.

Comment 58

[Axel Hecht [:Pike]]

I disagree.

I'm sitting here half a world away and I try to live with the state of the 
branches. And I will note my strong opposition to this for each branch this
spreads on to.

The 1.8.0 branch is l10n frozen, and that means that a bonsai query on the
locales/ directories should show no changes, because that is what localizers do.
Adding files at one place and removing them at the other is brittle, and it
failed before, though it doesn't show on the tinderbox that it fails, it still
does. Just see how many locales still have that file on CVS in the above
lxr query.

I know that I should more clearly state what an non-l10n impact patch for frozen
tree looks like (as in, don't change en-US and fall back, but just hard code it
in content). On the other hand, where would I put that?

Comment 59

[Darin Fisher]

OK, I will move uninstaller.inc into browser/installer/windows.  I suggest documenting this information on the WIKI.  Send mail to the newsgroups, mailing-lists, blog about it, etc.  Spread the word to all the module owners and folks who review code.

Comment 60

[Darin Fisher]

OK, moving uninstaller.inc is covered by bug 327465.

Comment 61

[Dave Liebreich [:davel]]

adding flag to put on radar of FF 1.5.0.2 triage team

Comment 62

[Daniel Veditz [:dveditz]]

Comment on attachment 211307 [details] [diff] [review]
revised branch patch (including fix for bug 307296)

What I really want to approve for the 1.8.0 branch is a patch that combines this and the changes in bug 327465. There's no point in checking in and back out on the stable branch.

Comment 63

[Darin Fisher]

> What I really want to approve for the 1.8.0 branch is a patch that combines
> this and the changes in bug 327465. There's no point in checking in and back
> out on the stable branch.

Bah, ok... it's not like I was going to submit a broken patch ;-)  I will post final bits shortly that include all of the follow-up patches.

Comment 64

[Darin Fisher]

For reference, I'm referring to the patches in bug 327039 and bug 327465.

Comment 65

[Darin Fisher]

Attachment: revised 1.8.0 branch patch (including all regression fixes)

Comment 66

[Daniel Veditz [:dveditz]]

Comment on attachment 212702 [details] [diff] [review]
revised 1.8.0 branch patch (including all regression fixes)

Thanks! Approved for 1.8.0 branch, a=dveditz

Comment 67

[Darin Fisher]

fixed1.8.0.2

does this bug really need the late-l10n keyword?  what impact does it actually have on l10n?  IMO, none.

Comment 68

[Axel Hecht [:Pike]]

late-l10n can go