1285328 - Hitting Escape or the X on the "Restart Firefox" dialog when enabling "Never remember history" is treated as a confirmation

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

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

STR:
Open Preferences > Privacy
In the History dropdown, select "Never remember history"
When the dialog appears, hit Escape on the keyboard

Expected results:
The "Revert" option is chosen and Firefox doesn't restart

Actual results:
Firefox restarts and loses all history

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

The way the dialogs work, the enter/return key presses the default, the escape key presses the other one (not sure what happens when there are three buttons.)

Based on bug 973014 comment 24, the two choices are [Restart Firefox now] and [Revert] (in that position, with the revert to the far right.)

However, we never explicitly talked about what should be the default operation.  Right now, the default is [Revert].  In that configuration, enter/return key will revert the operation, escape key will restart Firefox.

It may make more sense to switch that around - make [Restart Firefox now] the default, have return key cause the restart, and have escape do nothing/revert.

(I actually don't like any of the keys causing an such a "destructive" operation, but I suppose that enter/return doing that is less annoying than escape.)

Stephen, thoughts on this?  Should we change the default to be [Restart Firefox now], so that enter will restart and escape will do nothing?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

With bug 345067, the "cancel" is always button 1.  This means that, if we do want to go with "return restarts Firefox, escape does nothing", we would need to have the buttons be:

[Revert] [Restart Firefox now] 

instead of today's (bug 973014 comment 24):

[Restart Firefox now] [Revert]

Comment 3

[:Gijs (he/him)]

(In reply to Milan Sreckovic [:milan] from comment #2)
> With bug 345067, the "cancel" is always button 1.  This means that, if we do
> want to go with "return restarts Firefox, escape does nothing", we would
> need to have the buttons be:
> 
> [Revert] [Restart Firefox now] 
> 
> instead of today's (bug 973014 comment 24):
> 
> [Restart Firefox now] [Revert]

Note that the ordering of default / non-default buttons is platform-specific, and so it's not really relevant. The issue is what the 'default' (and therefore highlighted) option is.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to :Gijs Kruitbosch from comment #3)
> ...
> 
> Note that the ordering of default / non-default buttons is
> platform-specific, and so it's not really relevant. The issue is what the
> 'default' (and therefore highlighted) option is.

Right - that is one issue, probably a main one.  We also have (until/unless we deal with bug 345067) an issue that, regardless of what the default/highlighted option is, the "cancel" button is always in the same position, in each dialog - even though you can choose which button is the default, you cannot choose which button is the cancel.  Subject to bug 345067, that is.

Comment 5

[Mike Kaply [:mkaply]]

Then we need to reopen bug 345067 and put it somewhere...

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1285328: Part 1. Make sure the first button/escape is a no-op in these dialogs, and in the potential data loss case, make the default also a no-op. Refactor some common code into preferences.js.

Review commit: https://reviewboard.mozilla.org/r/63710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63710/

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

We'll want an uplift to aurora if this passes review.

Comment 8

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

Comment on attachment 8770175 [details]
Bug 1285328: Part 1. Make sure the first button/escape is a no-op in these dialogs, and in the potential data loss case, make the default also a no-op. Refactor some common code into preferences.js.

https://reviewboard.mozilla.org/r/63710/#review60772

::: browser/components/preferences/in-content/privacy.js:379
(Diff revision 1)
> -      let restartText = bundle.getFormattedString("okToRestartButton", [brandName]);
> -      let revertText = bundle.getString("revertNoRestartButton");
> +      let button0Text = bundle.getFormattedString("okToRestartButton", [brandName]);
> +      let button1Text = bundle.getString("revertNoRestartButton");

These two fuctions are near identical, with the exception of the strings. Can you factor out this to a shared function that lives in /browser/components/preferences/in-content/preferences.js ?

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Which two functions - the two in privacy.js and main.js?  That's a good idea.

We'd want this uplifted to 49 (that's where bug 973014 landed), and doing this refactoring makes me a bit nervous about doing that.  I'll get the refactored patch, and we can then decide.  If it ends up being too large for an uplift, we may want to spin that out into a separate bug, and land this one with the simple change.

Comment 10

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

(In reply to Milan Sreckovic [:milan] from comment #9)
> Which two functions - the two in privacy.js and main.js?  That's a good idea.

Yeah, the two functions that you made very similar in the previous patch.

> We'd want this uplifted to 49 (that's where bug 973014 landed), and doing
> this refactoring makes me a bit nervous about doing that.  I'll get the
> refactored patch, and we can then decide.  If it ends up being too large for
> an uplift, we may want to spin that out into a separate bug, and land this
> one with the simple change.

If it's as I imagine it would be just as risky as your first patch, which is to say not that risky.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1285328: Part 2. Add an ability to change the revert/cancel label and/or show the restart later button to the confirmRestartPrompt dialog.

Review commit: https://reviewboard.mozilla.org/r/64282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64282/

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8770175 [details]
Bug 1285328: Part 1. Make sure the first button/escape is a no-op in these dialogs, and in the potential data loss case, make the default also a no-op. Refactor some common code into preferences.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63710/diff/1-2/

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Minimal aurora patch for regression from bug 973014 and bug 1156176

Minimal aurora patch to fix the regression from bug 973014, reverting the buttons so that escape is a no-op.  Also an Aurora specific fix for another dialog not popping up (regression from bug 1156176, otherwise fixed with the other patches.)

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8771027 [details]
Bug 1285328: Part 2. Add an ability to change the revert/cancel label and/or show the restart later button to the confirmRestartPrompt dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64282/diff/1-2/

Comment 15

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

Comment on attachment 8770175 [details]
Bug 1285328: Part 1. Make sure the first button/escape is a no-op in these dialogs, and in the potential data loss case, make the default also a no-op. Refactor some common code into preferences.js.

https://reviewboard.mozilla.org/r/63710/#review61330

Comment 16

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

Comment on attachment 8771027 [details]
Bug 1285328: Part 2. Add an ability to change the revert/cancel label and/or show the restart later button to the confirmRestartPrompt dialog.

https://reviewboard.mozilla.org/r/64282/#review61332

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfae47ac3dba
Part 1. Make sure the first button/escape is a no-op in these dialogs, and in the potential data loss case, make the default also a no-op. Refactor some common code into preferences.js.  r=jaws
https://hg.mozilla.org/integration/autoland/rev/06bec0a41dac
Part 2. Add an ability to change the revert/cancel label and/or show the restart later button to the confirmRestartPrompt dialog. r=jaws

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8771029 [details] [diff] [review]
Minimal aurora patch for regression from bug 973014 and bug 1156176

Approval Request Comment
[Feature/regressing bug #]: 973014
[User impact if declined]: Using the escape key would lead to action being performed, and data loss.
[Describe test coverage new/current, TreeHerder]: Nothing automatic
[Risks and why]: Not much risk I can think of.
[String/UUID change made/needed]: None

Comment 19

[Guilherme Lima]

https://hg.mozilla.org/mozilla-central/rev/dfae47ac3dba
https://hg.mozilla.org/mozilla-central/rev/06bec0a41dac

Landed this morning.

Comment 20

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

Comment on attachment 8771029 [details] [diff] [review]
Minimal aurora patch for regression from bug 973014 and bug 1156176

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

The order of the buttons are swapped, but hitting the X on the dialog or pressing Escape still restarts the browser with this patch.

Also, the commit message is garbled. Please change it to:
Bug 1285328 - Swap the buttons in the privacy change confirm dialog to make sure Escape reverts the change. r=jaws

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to (unavailable 07/20-07/21) Jared Wein [:jaws] (please needinfo? me) from comment #20)
> Comment on attachment 8771029 [details] [diff] [review]
> Minimal aurora patch for regression from bug 973014 and bug 1156176
> 
> Review of attachment 8771029 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The order of the buttons are swapped, but hitting the X on the dialog or
> pressing Escape still restarts the browser with this patch.
> 

Yikes.  Silly mistake, thanks for catching that.

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Minimal aurora patch for regression from bug 973014 and bug 1156176

Comment 23

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

Comment on attachment 8772511 [details] [diff] [review]
Minimal aurora patch for regression from bug 973014 and bug 1156176

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

Thanks, works good.

Comment 24

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

Comment on attachment 8772511 [details] [diff] [review]
Minimal aurora patch for regression from bug 973014 and bug 1156176

Approval Request Comment
[Feature/regressing bug #]: 973014
[User impact if declined]: Using the escape key would lead to action being performed, and data loss.
[Describe test coverage new/current, TreeHerder]: Nothing automatic
[Risks and why]: Not much risk I can think of.
[String/UUID change made/needed]: None

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8772511 [details] [diff] [review]
Minimal aurora patch for regression from bug 973014 and bug 1156176

new regression, taking it.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4860a3b8fddf

Comment 27

[Nazir Ahmed Sabbir [:NaSb]]

I've managed this issue on this bug in Nightly 50.0a1 (2016-07-07) ; (Build ID: 20160707083343) from Linux.

This Bug is now verified as fixed on Latest Firefox Nightly 50.0a1 (2016-07-30)

Build ID: 20160730030204
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
OS: Linux 4.4.0-31-generic ; Ubuntu 16.04 (64 Bit)

Comment 28

[Mohammad Maruf Rahman]

I have successfully reproduced this bug with Nightly 50.0a1 (2016-07-07) on Windows 7, 64 Bit!

This bug's fix is verified on latest Beta, Nightly

Build  ID     20160811031722
User Agent    Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0


Build  ID     20160813030202
User Agent    Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0

[testday-20160812]

Comment 29

[Mohammad Maruf Rahman]

(In reply to Maruf Rahman from comment #28)
> I have successfully reproduced this bug with Nightly 50.0a1 (2016-07-07) on
> Windows 7, 64 Bit!
> 
> This bug's fix is verified on latest Beta, Nightly
> 
> Build  ID     20160811031722
> User Agent    Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0)
> Gecko/20100101 Firefox/49.0
> 
> 
> Build  ID     20160813030202
> User Agent    Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:51.0)
> Gecko/20100101 Firefox/51.0
> 
> [testday-20160812]

Sorry, my bad. This bug's fix is verified on latest Beta and Aurora.(No Nightly) 

Build       ID     20160803004014
User     Agent     Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

Comment 30

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced this issue on Fx 50.0a1 Nightly on Windows 10x64.

Confirming the fix on Windows 10x64, Ubuntu 12.04x86 and Mac OS X 10.9.5 using:
- Fx 49.0b7, build ID 20160825132718
- Latest 50.0a2 Aurora, build ID 20160825004011.