Closed Bug 1285328 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Preferences, defect, P1)

49 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox49 --- verified
firefox50 --- verified

People

(Reporter: jaws, Assigned: milan, Mentored)

References

Details

(Keywords: dataloss, regression, ux-error-prevention, Whiteboard: [outreachy-12])

Attachments

(3 files, 1 obsolete file)

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
Priority: -- → P2
Blocks: 973014
Priority: P2 → --
Priority: -- → P1
Flags: needinfo?(milan)
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?
Flags: needinfo?(milan) → needinfo?(shorlander)
Version: Trunk → 49 Branch
Assignee: nobody → milan
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]
(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.
(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.
Then we need to reopen bug 345067 and put it somewhere...
Flags: needinfo?(shorlander)
See Also: → 1282537
We'll want an uplift to aurora if this passes review.
Attachment #8770175 - Flags: review?(past) → review?(jaws)
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 ?
Attachment #8770175 - Flags: review?(jaws) → review-
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.
(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.
Review commit: https://reviewboard.mozilla.org/r/64282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64282/
Attachment #8770175 - Attachment description: Bug 1285328: 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. → 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.
Attachment #8771027 - Flags: review?(jaws)
Attachment #8770175 - Flags: review- → review?(jaws)
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/
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 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/
Attachment #8771027 - Attachment description: Bug 1285328: Part 2. Add an ability to hide the cancel and/or show the restart later buttons to the confirmRestartPrompt dialog. → Bug 1285328: Part 2. Add an ability to change the revert/cancel label and/or show the restart later button to the confirmRestartPrompt dialog.
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
Attachment #8770175 - Flags: review?(jaws) → review+
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
Attachment #8771027 - Flags: review?(jaws) → review+
Attachment #8771029 - Attachment is patch: true
Attachment #8771029 - Attachment mime type: application/x-archive → text/plain
Pushed by msreckovic@mozilla.com:
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 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
Attachment #8771029 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/dfae47ac3dba
https://hg.mozilla.org/mozilla-central/rev/06bec0a41dac

Landed this morning.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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
Attachment #8771029 - Flags: review?(jaws) → review-
Attachment #8771029 - Flags: approval-mozilla-aurora?
(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 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.
Attachment #8772511 - Flags: review?(jaws) → review+
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
Attachment #8772511 - Flags: approval-mozilla-aurora?
Comment on attachment 8772511 [details] [diff] [review]
Minimal aurora patch for regression from bug 973014 and bug 1156176

new regression, taking it.
Attachment #8772511 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
QA Whiteboard: [bugday-20160727]
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]
(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
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.