917052 - Change - Add a confirmation dialog for "Clear private data" in Options flyout

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Flyouts, defect, P2)

Description

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

To prevent accidentally clear private data by hitting the "Clear" button trying to scroll down Options panel.

Comment 1

[Allison Naaktgeboren :ally]

reviewed in triage

Comment 2

[Marina Samuel [:emtwo]]

Attachment: v1: Dialog box to confirm before clearing private data

Comment 3

[Marina Samuel [:emtwo]]

Attachment: confirm_dialog

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 813377 [details] [diff] [review]
v1: Dialog box to confirm before clearing private data

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

r=mbrubeck with some trivial changes

::: browser/metro/base/content/browser-ui.js
@@ +1077,5 @@
>          break;
>      }
>    },
>  
> +  confirmSanitizeDialog: function confirmSanitizeDialog() {

Note: You can now write this without repeating the name.  (The repetition used to be necessary for debugging, but the JS engine has gotten smarter since then.)  I've been moving to toward this style, though it's not a big deal in files that still use the old style:

  confirmSanitizeDialog: function () {

@@ +1096,5 @@
> +                          null,
> +                          { value: false });
> +
> +    // Clicking 'Clear' will call onSanitize().
> +    if (!buttonPressed) {

Nit: Could you change this to "if (buttonPressed === 0)" as a hint to the reader that the return value is an integer, not a boolean?

::: browser/metro/locales/en-US/chrome/browser.properties
@@ +36,5 @@
>  # LOCALIZATION NOTE (contextAppbar2.clear): Unselects pages without modification.
>  contextAppbar2.clear=Clear selection
>  
> +# Clear private data
> +clearPrivateData.clear=Clear

Nit: Please change this to something like "clearPrivateData.clearButton" so it's more obvious to the localizers how this string is used.

@@ +37,5 @@
>  contextAppbar2.clear=Clear selection
>  
> +# Clear private data
> +clearPrivateData.clear=Clear
> +clearPrivateData.title=Clear Private data

Style nit: "Data" should be capitalized in the title.

Comment 5

[Marco Mucci [:MarcoM]]

Hey Marina, can you provide a point estimate for this bug?

Comment 6

[Marina Samuel [:emtwo]]

p=1

Comment 7

[Marina Samuel [:emtwo]]

Attachment: Tests

Tests similar to those in browser_crashprompt.js to check for each button click case

Comment 8

[Marina Samuel [:emtwo]]

Attachment: v2: Tests

Last patch was not up to date.

Comment 9

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/13669c0bccdb
https://hg.mozilla.org/integration/fx-team/rev/57bc33e66107

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13669c0bccdb
https://hg.mozilla.org/mozilla-central/rev/57bc33e66107

Comment 11

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

Mozilla/5.0 (Windows NT 6.2; rv:27.0) Gecko/20100101 Firefox/27.0

Verified on latest Nightly (build ID: 20131016030202)
A confirmation dialog is now shown afer selecting "Clear private data" from the Options flyout.