405037 - Should "Don't Save" really be the default button when Quitting with lots of tabs?

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P1)

Description

[Håkan Waara]

Firefox 3 beta 1, all platforms

I'm used to the safe option being the default, so sometimes I tend to be a little sloppy. The computer is supposed to help me make the safe decision, right? 

In this case, I was asked by Firefox whether I wanted to save tabs, quickly hitting Enter, assuming it would save it for me. I think Camino saves the tabs by default, and this is probably why I was so quick.

Comment 1

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

Taking.

Comment 2

[Mike Connor [:mconnor]]

Attachment: change default for normal quit dialog

Pretty straightforward, set the default as appropriate to the context of the quit dialog.

Comment 3

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

Comment on attachment 290561 [details] [diff] [review]
change default for normal quit dialog

POS_0 has "OK" semantics, POS_1 is "Cancel", and POS_2 is "Extra". 0 is the default if no other default is specified. We have two scenarios:

1) "Quit", "Save and Quit", and "Cancel"
2) "Restart" and "Cancel"

For 2, the obvious solution is to have "Restart" be OK, and "Cancel" be "Cancel", and default to OK (which is what we currently do). For 1, we currently use POS_0 for "Quit", POS_1 for "Cancel", and POS_2 for "Save and Quit". This patches keeps that positioning but makes POS_2 the default. For case 2, shouldn't we instead move "Save and Quit" to POS_0, and leave POS_0_DEFAULT for both cases?

Comment 4

[Mike Connor [:mconnor]]

hmm, that makes sense, new patch upcoming tonight

Comment 5

[Mike Connor [:mconnor]]

Attachment: patch v2

Comment 6

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

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

>Index: browser/components/nsBrowserGlue.js

>+      var button0Title, button2Title;
>+      var button1Title = quitBundle.GetStringFromName("cancelTitle");
>+      var neverAskText = quitBundle.GetStringFromName("neverAsk");
>+
>+      if (aQuitType != "restart") {
>         flags += promptService.BUTTON_TITLE_IS_STRING * promptService.BUTTON_POS_2;
>+        button0Title = quitBundle.GetStringFromName("saveTitle");
>+        button2Title = quitBundle.GetStringFromName("quitTitle");
>+      }
>+      else
>+        button0Title = quitBundle.GetStringFromName("restartTitle");

super nits: perhaps use "okButtonTitle"/"cancelButtonTitle"/"extraButtonTitle" rather than the numbered variable names? And flip the then/else blocks arounds and instead test aQuitType == "restart"?

Comment 7

[Mike Connor [:mconnor]]

Attachment: as checked in

left the button title vars as is, since that maps better to the consts already in use, but flipped the if/else

Comment 8

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007113004 Minefield/3.0b2pre. "Save and Quit" is now the default choice.

Comment 9

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5116 added in Litmus.