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

RESOLVED FIXED in Firefox 2 beta2

Status

()

Firefox
Preferences
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Steffen Wilberg, Assigned: Steffen Wilberg)

Tracking

({dataloss, fixed1.8.1, late-l10n})

Trunk
Firefox 2 beta2
x86
Linux
dataloss, fixed1.8.1, late-l10n
Points:
---
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 230282 [details] [diff] [review]
patch

Updates the "Clear Now..." button label when the privacy pane is initialized, and after opening the sanitize settings dialog.
Assignee: nobody → steffen.wilberg
Status: NEW → ASSIGNED
Attachment #230282 - Flags: review?(mconnor)
(Assignee)

Updated

12 years ago
Version: 2.0 Branch → Trunk
(Assignee)

Comment 2

12 years ago
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.
(Assignee)

Comment 3

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

Updated

12 years ago
Flags: blocking-firefox2?
Keywords: late-l10n
Target Milestone: --- → Firefox 2 beta2
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.
Flags: blocking-firefox2? → blocking-firefox2+
(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 on attachment 230282 [details] [diff] [review]
patch

r=me, but use setAttribute instead of .label =
Attachment #230282 - Flags: review?(mconnor) → review+
Please do use .label, the button binding has this property (from basetext).

Updated

12 years ago
Whiteboard: [has patch][checkin needed]
(Assignee)

Comment 8

12 years ago
(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.
(Assignee)

Comment 9

12 years ago
Checked in on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][checkin needed]
(Assignee)

Comment 10

12 years ago
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.
Attachment #230282 - Flags: approval1.8.1?
(Assignee)

Comment 11

12 years ago
Forgot to mention that I kept using .label= as Mano requested. And yes, I've tested it, and it works just fine.
Comment on attachment 230282 [details] [diff] [review]
patch

a=mconnor on behalf of drivers for checkin after the new prefwindow lands
Attachment #230282 - Flags: approval1.8.1? → approval1.8.1+
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.
Keywords: fixed1.8.1
(Assignee)

Comment 14

12 years ago
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?
(Assignee)

Comment 15

12 years ago
(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.)
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 "()"
(Assignee)

Comment 17

12 years ago
Created attachment 231506 [details] [diff] [review]
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 ''.
(Assignee)

Comment 18

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

This is now part of bug 345769.
Attachment #231506 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.