Closed Bug 1086958 Opened 10 years ago Closed 10 years ago

back out default browser prompting change

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
36.2
Tracking Status
firefox34 + verified
firefox35 + verified
firefox36 + verified

People

(Reporter: Gavin, Assigned: Gavin)

References

Details

(Whiteboard: [fxgrowth])

Attachments

(4 files, 2 obsolete files)

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.
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.
Flags: firefox-backlog+
The patch that produced the screenshot from comment 1.
Slightly improved version (with better button labels):

https://cloudup.com/ceVYd21YskO
Attachment #8508981 - Attachment is obsolete: true
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 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?
Attachment #8508998 - Flags: ui-review?(philipp)
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)?
Attachment #8508998 - Flags: ui-review?(philipp) → ui-review+
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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")?
(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.
Attachment #8508998 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8508998 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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?
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Flags: needinfo?(gavin.sharp)
(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.
Adding Camelia so she knows when this happens.
QA Contact: camelia.badau
(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.
Flags: needinfo?(gavin.sharp)
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
Attachment #8509173 - Flags: approval-mozilla-beta?
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.
Attachment #8509173 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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 on attachment 8509173 [details] [diff] [review]
beta patch with no string changes

Going to take this on Aurora too.
Attachment #8509173 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/7f9adc144d57
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Iteration: --- → 36.2
Flags: qe-verify?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Landed a slightly tweaked version of the beta patch on Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f4cb8cbeaed
Summary: back out default browser prompting change for Firefox 34 → back out default browser prompting change
Flags: qe-verify? → qe-verify-
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?
Flags: needinfo?(philipp)
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?
Flags: needinfo?(philipp)
(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.
(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.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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 :)
Keeping the old strings around per comment 26.
Attachment #8515065 - Flags: review?(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.
(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.
(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 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.
Attachment #8515065 - Flags: review?(dao) → review+
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).
Flags: qe-verify- → qe-verify+
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.
https://hg.mozilla.org/mozilla-central/rev/7d3e942ee884
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached image 1086958.png
I also see strange setDefaultBrowserConfirm button state before hover.
(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.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: needinfo?(splewako)
Resolution: --- → FIXED
(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!
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).
Depends on: 1100399
Depends on: 1100401
Filed bug 1100399 for the strings issue, bug 1100401 for the active button background color and leaving the rest of this mess alone.
Flags: needinfo?(splewako)
No longer depends on: 1100401
Whiteboard: [fxgrowth]
Depends on: 1120421
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?
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?
Blocks: 1772662
You need to log in before you can comment on or make changes to this bug.