1086958 - back out default browser prompting change

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

In bug 951627, we switched from using a modal prompt to a notification bar to ask the user about default browser.

FHR data suggests that this change results in a lower percentage of new users setting Firefox as their default, presumably because the notification bar is easier to miss/ignore.

We tried to mitigate that with bug 1079372, but it's not yet clear that will be sufficient.

We're probably not striking the right balance between discoverability/obtrusiveness here, so let's back it out for at least 34, and work on alternatives.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Complicating this is the fact that the original patch removed the old strings, so now we're pretty limited by the l10n freeze.

In theory we can use the new strings in a dialog rather than a notification bar. I gave that a shot and ended up with this:

https://cloudup.com/cjSOe2bTeI6

Clearly not ideal for a prompt, but it might be our only option.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: beta patch with no string changes

The patch that produced the screenshot from comment 1.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: beta patch with no string changes

Slightly improved version (with better button labels):

https://cloudup.com/ceVYd21YskO

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Note: that patch also introduces a slight behavior tweak that mitigates bug 1041516 for this particular prompt (by only shouldCheckDefaultBrowser=false if you check the "don't ask again" box and then decline the prompt, rather than setting it even if you check the box and accept the prompt).

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8508998 [details] [diff] [review]
beta patch with no string changes

Philipp, obviously this is not ideal, but I don't think we can do better for beta, unfortunately. Are you OK with shipping comment 3's version?

Comment 6

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8508998 [details] [diff] [review]
beta patch with no string changes

Thanks for the patch Gavin!
Yeah, understanding the constraints here, I agree that this is probably the closest we can get to the old behavior.

Can we wait until next Tuesday with uplifting that (in case the yellow bar yields dramatically different results)?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #3)
> Created attachment 8508998 [details] [diff] [review]
> beta patch with no string changes
> 
> Slightly improved version (with better button labels):
> 
> https://cloudup.com/ceVYd21YskO

Can we replace the "Confirm" at the top of the dialog with something else, such as setAsMyDefaultBrowser.label from preferences' main.dtd ("Make &brandShortName; My Default Browser")?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Can we replace the "Confirm" at the top of the dialog with something else,
> such as setAsMyDefaultBrowser.label from preferences' main.dtd ("Make
> &brandShortName; My Default Browser")?

We could, but I think that's stretching it (both in terms of "different string context" and implementation ugliness). We can live with the generic title.

Comment 9

[:Gijs (out for now; he/him)]

Comment on attachment 8508998 [details] [diff] [review]
beta patch with no string changes

Review of attachment 8508998 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ +2168,5 @@
> +                          yesLabel, notNowLabel, null, checkboxLabel, dontAskAgain);
> +    if (rv == 0) {
> +      this.setAsDefault();
> +    } else if (dontAskAgain.value) {
> +      shell.shouldCheckDefaultBrowser = false;

shell is not defined here, so this fails:

System JS : ERROR file:///Users/gkruitbosch/dev/builds/beta-x86_64-apple-darwin13.4.0/dist/Nightly.app/Contents/Resources/browser/components/nsBrowserGlue.js:2092 - ReferenceError: shell is not defined

You want ShellService instead. With that, r=me

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #8)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> > Can we replace the "Confirm" at the top of the dialog with something else,
> > such as setAsMyDefaultBrowser.label from preferences' main.dtd ("Make
> > &brandShortName; My Default Browser")?
> 
> We could, but I think that's stretching it (both in terms of "different
> string context" and implementation ugliness). We can live with the generic
> title.

Are we going to add back the missing strings to Firefox 36 so we will have them in-tree in case we decide to continue this back out as we test out changes to the implementation?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: beta patch with no string changes

Comment 12

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> Are we going to add back the missing strings to Firefox 36 so we will have
> them in-tree in case we decide to continue this back out as we test out
> changes to the implementation?

I think we should definitely do that.
One of the takeaways from this for me is that we need to test different configurations here. Having the strings in the product will provide us with more flexibility there.

Comment 13

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Adding Camelia so she knows when this happens.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> Are we going to add back the missing strings to Firefox 36 so we will have
> them in-tree in case we decide to continue this back out as we test out
> changes to the implementation?

Sure.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Attachment: trunk patch to re-add strings

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8509173 [details] [diff] [review]
beta patch with no string changes

Approval Request Comment
[Feature/regressing bug #]: bug 951627
[User impact if declined]: lower default browser "take" rate, negative impact on usage/growth
[Describe test coverage new/current, TBPL]: manual tests, no automated coverage
[Risks and why]: we're not quite wholly reverting the patch directly because that would involve string changes. There isn't overall much risk since the functionality is straightforward, and manual test coverage is good.
[String/UUID change made/needed]: none

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8509173 [details] [diff] [review]
beta patch with no string changes

I take it this fix needs to land on both 35 and 36 as well. If that's correct, can you please submit an Aurora approval request and get this landed on m-c. I'm approving for Beta as this is a Beta specific patch.

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Just adding a note that we should have numbers on Tuesday that let us know whether the new yellow notification bar still shows a drop in default percentage, so we should probably hold off on checking this in until then if possible…

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d080a93fd9e1

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Re-adding the strings on trunk:
https://hg.mozilla.org/integration/fx-team/rev/7f9adc144d57

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8509173 [details] [diff] [review]
beta patch with no string changes

Going to take this on Aurora too.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f9adc144d57

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Sorry, shouldn't have left-open. I think we will effectively back this out entirely and put the new stuff behind a pref on trunk.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Landed a slightly tweaked version of the beta patch on Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f4cb8cbeaed

Comment 25

[:Gavin Sharp [email: [e-mail]]]

In trying to figure out what to do on trunk to revert to the modal dialog, I have a strings dilemma. I could revert to exactly the old strings we used to use, but I actually think the new "hybrid" strings that we had to take for 34/35 are somewhat better.

Old prompt strings (33): https://cloudup.com/c36c4256oHK

"Hybrid" (34/35) prompt strings: https://cloudup.com/ceVYd21YskO

The things I prefer about the "hybrid" prompt is that the button labels are more useful, the message is more encouraging, and the "Don't ask again" seems simpler and less difficult to parse than "Always perform this check when starting Firefox". The only downside is that it's title ("Confirm") is not ideal. So I propose just fixing the title and going with this for 36:

https://cloudup.com/clsEw70MnbQ

Philipp, what do you think?

Comment 26

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

There are a couple of nuances here:

- I definitely think that the button captions are clearer with the new strings. That said, I wonder if having a non-standard caption on the button has any effect on default-conversion.
- The new checkbox label is technically not correct, since there are a couple of instances where we will ask in the future even if you've checked that. But it's probably correct enough.

It would be great if we could stay flexible there.
How about using your proposed version, but also re-landing the other strings so that we can do some testing with them?

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Philipp Sackl [:phlsa] from comment #26)
> There are a couple of nuances here:
> 
> - I definitely think that the button captions are clearer with the new
> strings. That said, I wonder if having a non-standard caption on the button
> has any effect on default-conversion.

Microsoft recommends using the specific action on the button, http://msdn.microsoft.com/en-us/library/dn742478.aspx (search for 'specific text' and also 'specific label' to find the two sections mentioning this). I imagine that they have done a fair amount of user research to see that this change results in better outcomes.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

(In reply to Philipp Sackl [:phlsa] from comment #26)
> - The new checkbox label is technically not correct, since there are a
> couple of instances where we will ask in the future even if you've checked
> that. But it's probably correct enough.

Which cases are you talking about? I don't know of any...

> It would be great if we could stay flexible there.
> How about using your proposed version, but also re-landing the other strings
> so that we can do some testing with them?

Sure.

Comment 29

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #28)
> (In reply to Philipp Sackl [:phlsa] from comment #26)
> > - The new checkbox label is technically not correct, since there are a
> > couple of instances where we will ask in the future even if you've checked
> > that. But it's probably correct enough.
> 
> Which cases are you talking about? I don't know of any...

I guess I was mainly referring to things that are planned but not implemented yet:
- If someone uninstalls Fx (which keeps the profile) and then reinstall it
- If someone makes Fx the default browser and then switches to a different default browser
- If someone comes back to Fx after a long time and the profile gets reset

But those are really technicalities, and in the users mind, both labels probably mean the same anyway.
So the new label is fine :)

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Attachment: trunk patch adding a pref for modal vs. notificationbox

Keeping the old strings around per comment 26.

Comment 31

[Dão Gottwald [:dao]]

What exactly is the plan for the old strings? I think I'd prefer if we added them back when we actually use them.

Speaking of which, what's the plan for the notification bar and associated hidden pref? The lower success rate seems inherent to the UI being non-modal, so it's unclear to me how we'd address that and re-enable the notification bar.

Comment 32

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Dão Gottwald [:dao] from comment #31)
> What exactly is the plan for the old strings? I think I'd prefer if we added
> them back when we actually use them.
> 
> Speaking of which, what's the plan for the notification bar and associated
> hidden pref? The lower success rate seems inherent to the UI being
> non-modal, so it's unclear to me how we'd address that and re-enable the
> notification bar.

The only thing we can say for sure right now is that the notification bar has been performing worse in Fx 34. There's some uncertainty around the data and a couple of things we could try to make it perform better.
What we don't know is what exactly caused the drop-off. While the non-modalness of the notification bar is a reasonable explanation, it is not the only explanation.
We even don't know for sure whether reverting to the modal with new strings will completely fix it, since the strings could have played a role in the drop-off as well.

With so much uncertainty around, the more flexible we are, the better. We were limited when backing it out because the strings weren't there and it would be great to avoid that situation in the future.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #31)
> What exactly is the plan for the old strings? I think I'd prefer if we added
> them back when we actually use them.

We might want to use them for testing in 36 on Aurora/Beta, and we can't add them there.

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 8515065 [details] [diff] [review]
trunk patch adding a pref for modal vs. notificationbox

>       if (shouldCheck && !isDefault && !willRecoverSession) {
>         Services.tm.mainThread.dispatch(function() {
>-          DefaultBrowserCheck.prompt(this.getMostRecentBrowserWindow());
>+          DefaultBrowserCheck.prompt(this.getMostRecentBrowserWindow(), shouldCheck);

I don't understand why you're passing shouldCheck, which is always true here. Seems like |let dontAsk = { value: !shouldCheck };| should just be |let dontAsk = { value: false };|.

>+    let neverButton = shellBundle.getString("setDefaultBrowserNever.label");
>+    let neverButtonKey = shellBundle.getString("setDefaultBrowserNever.accesskey");

nit: this isn't a button

>+      let selectedBrowser = win.gBrowser.selectedBrowser;

This is unused.

>+      this._notification.persistence = -1;

Shouldn't be needed, since the notification bar isn't associated with a browser and removeTransientNotifications won't be called.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/fx-team/rev/7d3e942ee884

Verification: default browser prompt appears and works correctly with browser.defaultbrowser.notificationbar set to true or false (notification bar appears if true, modal prompt otherwise).

Comment 36

[:Gavin Sharp [email: [e-mail]]]

The steps in comment 35 are for trunk - for 34/35, we just reverted to a modal dialog prompt and did not add the browser.defaultbrowser.notificationbar pref. Verification that that prompt works correctly in those builds would also be useful.

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7d3e942ee884

Comment 38

[Stefan Plewako [:stef]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #25)
> In trying to figure out what to do on trunk to revert to the modal dialog, I
> have a strings dilemma. I could revert to exactly the old strings we used to
> use, but I actually think the new "hybrid" strings that we had to take for
> 34/35 are somewhat better.
> 
> Old prompt strings (33): https://cloudup.com/c36c4256oHK
> 
> "Hybrid" (34/35) prompt strings: https://cloudup.com/ceVYd21YskO
> 
> The things I prefer about the "hybrid" prompt is that the button labels are
> more useful, the message is more encouraging, and the "Don't ask again"
> seems simpler and less difficult to parse than "Always perform this check
> when starting Firefox". The only downside is that it's title ("Confirm") is
> not ideal. So I propose just fixing the title and going with this for 36:
> 
> https://cloudup.com/clsEw70MnbQ

Never reuse strings in such way, never. 
Add new strings (change label) if you wish to change UI/strings/context.
If you are not sure about old ones, leave them for now and remove later.

https://hg.mozilla.org/mozilla-central/rev/72de0ffdfbd6
https://hg.mozilla.org/mozilla-central/rev/7d3e942ee884

Comment 39

[Stefan Plewako [:stef]]

Attachment: 1086958.png

I also see strange setDefaultBrowserConfirm button state before hover.

Comment 40

[:Gavin Sharp [email: [e-mail]]]

(In reply to Stefan Plewako [:stef] from comment #38)
> Never reuse strings in such way, never. 
> Add new strings (change label) if you wish to change UI/strings/context.
> If you are not sure about old ones, leave them for now and remove later.

Stefan, could you please file a new bug to cover this, and describe the issues it caused for your localization in a bit more detail? Happy to fix this in a followup on trunk.

Comment 41

[:Gijs (out for now; he/him)]

(In reply to Stefan Plewako [:stef] from comment #39)
> Created attachment 8523422 [details]
> 1086958.png
> 
> I also see strange setDefaultBrowserConfirm button state before hover.

Could you also file a separate bug about this, including details on what OS this is (looks like OS X Yosemite?) and with what build? (is this beta? nightly?)

It's likely not related to this change, but just to styling on that OS/build combination, and I'd like to look into it, but I need more details and this is /really/ not the right place.

Thank you!

Comment 42

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Nightly 36.0a1(buildID: 20141116030212), latest Aurora 35.0a2(buildID: 20141116004002) and Firefox 34 Beta 9 (buildID: 20141114133026).

Comment 43

[Stefan Plewako [:stef]]

Filed bug 1100399 for the strings issue, bug 1100401 for the active button background color and leaving the rest of this mess alone.

Comment 44

[Chris More [:cmore]]

Now that we are back to a modal dialog box, is there plans for developing a new UX and doing some A/B tests with cohorts to measure the effectiveness?

Comment 45

[Blake Winton (:bwinton) (:☕️)]

I don't think there are any concrete plans, but the new browser.defaultbrowser.notificationbar makes it quite a bit easier to do the A/B tests, and I'm reasonably sure no-one would object to them.  Would you like to file a new bug?