345599 - label of the "Clear Now" button in the Privacy pane shouldn't contain ellipsis (...) when set to not ask before clearing

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Steffen Wilberg]

In the new prefwindow, the label of the "Clear Now" button in the Privacy pane shouldn't contain ellipsis when set to not ask before clearing.

So it should be "Clear Now..." if clicking the button would show the dialog, otherwise just "Clear Now".

This is a potential dataloss issue because a user might expect the dialog to come up because of the ellipsis.

Comment 1

[Steffen Wilberg]

Attachment: patch

Updates the "Clear Now..." button label when the privacy pane is initialized, and after opening the sanitize settings dialog.

Comment 2

[Steffen Wilberg]

Oh, I forgot to change the updateClearNowButtonLabel() comment. It should be:
+   * Sets the label of the "Clear Now..." button according to the
+   * privacy.sanitize.promptOnSanitize pref.

I've fixed that locally.

Comment 3

[Steffen Wilberg]

The menu item in the History menu is already updated dynamically via SanitizeListener() in browser.js.

Comment 4

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

I take it that this button always clears the private data regardless of the "Always ask me" setting in the Clear Private Data settings? That actually seems unwise to me, but if that's the case, then yes, the ellipses should vanish.

Comment 5

[Jeff Walden [:Waldo]]

(In reply to comment #4)
> I take it that this button always clears the private data regardless of the
> "Always ask me" setting in the Clear Private Data settings? That actually seems
> unwise to me, but if that's the case, then yes, the ellipses should vanish.

Actually, it's sorta broken that way -- it only shows a dialog if "Always ask me" is checked.  Note that this also applies to Tools>Clear Private Data...

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 230282 [details] [diff] [review]
patch

r=me, but use setAttribute instead of .label =

Comment 7

[Asaf Romano (gone)]

Please do use .label, the button binding has this property (from basetext).

Comment 8

[Steffen Wilberg]

(In reply to comment #4)
> I take it that this button always clears the private data regardless of the
> "Always ask me" setting in the Clear Private Data settings? That actually seems
> unwise to me, but if that's the case, then yes, the ellipses should vanish.

If the "ask me before clearing" checkbox in Privacy->Private Data->Settings is checked, clicking the "Clear Now..." button displays a dialog with "Cancel" and "Clear" buttons.
If it is not checked however, clicking the Clear button just clears without asking. In that case the button label should be "Clear Now", without ellipsis. That's what this bug is about.

Comment 9

[Steffen Wilberg]

Checked in on trunk.

Comment 10

[Steffen Wilberg]

Comment on attachment 230282 [details] [diff] [review]
patch

Branch checkin depends on the branch landing of the new prefwindow, but I could already add the new string.

Comment 11

[Steffen Wilberg]

Forgot to mention that I kept using .label= as Mano requested. And yes, I've tested it, and it works just fine.

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 230282 [details] [diff] [review]
patch

a=mconnor on behalf of drivers for checkin after the new prefwindow lands

Comment 13

[Jeff Walden [:Waldo]]

To save localizers the trouble of having slightly-out-of-sync DTDs (and thus to allow a direct copy over of trunk DTDs to branch code without changes, hopefully ameliorating the problem of dealing with wicked huge l10n changes on branch), I rolled this patch into the commit of the prefwindow to branch.  I did a final build of everything and did a quick functionality smoketest for all prefwindow features, including this one, to make sure everything worked correctly.

Consequently, this is fixed on branch.

Comment 14

[Steffen Wilberg]

Comment on attachment 230282 [details] [diff] [review]
patch

>Index: mozilla/browser/components/preferences/privacy.js
>+   updateClearNowButtonLabel: function()
>+   {
>+     var prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
>+                                .getService(Components.interfaces.nsIPrefBranch);
>+     var clearNowButton = document.getElementById("clearDataNow");
>+     if (prefBranch.getBoolPref("privacy.sanitize.promptOnSanitize"))
>+       clearNowButton.label = clearNowButton.getAttribute("label1"); // "Clear Now..."
>+     else
>+       clearNowButton.label = clearNowButton.getAttribute("label2"); // "Clear Now"
>+   },
>+   
>+   /**
>    * Displays the Clear Private Data settings dialog.
>    */
>   showClearPrivateDataSettings: function ()
>   {
>     document.documentElement.openSubDialog("chrome://browser/content/preferences/sanitize.xul",
>                                            "", null);
>+    this.updateClearNowButtonLabel();
>   },
Hmm, the button label should be updated after leaving the Clear Private Data settings dialog. But on Windows, where browser.preferences.instantApply is false by default, the pref and therefore the button label is only updated after clicking OK on the main prefwindow...
How can I check whether the promptOnSanitize pref *will be* changed (and to which value) when the user clicks OK?

Comment 15

[Steffen Wilberg]

(Note to self: get the <preference> with the name "privacy.sanitize.promptOnSanitize", which is inserted into the parent prefwindow after leaving the child dialog, and look at preference.value.)

Comment 16

[Jeff Walden [:Waldo]]

If you plan on doing a second patch, a couple requests:

- change the new method name to start with a "_" -- convention in prefwindow
  files is to "_"-prefix any method only used within the containing JS file (and
  not within XUL) and to use an unprefixed name for anything called from outside
  the containing JS file
- except in very rare circumstances (e.g. populating the list of feed readers),
  it should never be necessary to manually get a preference value -- create a
  preferences element instead, and the .value property on it will always be
  what's shown in UI (I believe even with subdialogs as happens here), on
  systems with or without instant-apply
- random style nits in comments (fixed in the branch checkin, but trunk's still
  screwy):

>   /**
>-   * Sets up the UI for the number of days of history to keep.
>+   * Sets up the UI for the number of days of history to keep, and updates the
>+     label of the "Clear Now..." button.
>    */

Missing *

>-  /**
>+   /**
>+   * Removes the ellipsis from the "Clear Now..." button if
>+   * privacy.sanitize.promptOnSanitize is false.
>+   */

>+   /**
>    * Displays the Clear Private Data settings dialog.

Funky "/**" indentation

>+   updateClearNowButtonLabel: function()

Space between "function" and "()"

Comment 17

[Steffen Wilberg]

Attachment: patch to fix on Windows

Adds a <preference> for the promptOnSanitize pref and checks that.
The only drawback is that the Error Console shows this message 7 times after clicking OK in the Clear Private Data settings dialog:
<preference> with id='' and name='' has unknown type ''.

Comment 18

[Steffen Wilberg]

Comment on attachment 231506 [details] [diff] [review]
patch to fix on Windows

This is now part of bug 345769.