480169 - Clear recent history refresh (sprint)

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P1)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Mockup of the refresh to the clear recent history dialog

= Goals / Use Cases =
* Goal: add a time selection widget to both provide visual feedback and confirmation of what is going to be removed, and to give the user a finer grained level of control for selecting a specific range.
** Use case: guess that you want to clear about two hours, see that this looks right (or drag the range a little, since it was really more like two hours and 15 min), and then click clear.
* Goal: provide more warning before the user clears all of their history
** Use case: user selects "everything" by accident, and they subsequently realize that they don't want to nuke all the history in their profile thanks to the warning interface we have designed.

= Non Goals =
* The largest non-goal is to directly cater to the needs of users who expect the dialog to behave in the same way as the previous version.  For instance, if the user regularly opens this dialog to clear all of your active logins, this now a more complex operation since they need to now expand the details are, and also scroll down to get to active log-ins.
* Another non-goal is for this dialog to enable a full reset of Firefox (including personal data level things like passwords, bookmarks, new dictionary entries, add-ons and and setting).  A total Firefox reset is best handled with some type of higher level profile manager UI, and probably not from inside of the application itself.

= Design =
http://people.mozilla.com/~faaborg/files/shiretoko/clearRecentHistoryi3.png

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Given bug 465995, we still can't clear browsing history without clearing downloads, so the new UI will need to reflect that somehow...

Comment 3

[guy0307]

New UI also looks quite bad. I think the most user-friendly menu is the current 3.0 one. The 3.1 menu looks very bad.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

>New UI also looks quite bad. I think the most user-friendly menu is the current
>3.0 one. The 3.1 menu looks very bad.

not user-friendly and looks bad aren't really specific enough to be useful for us.  Can you explain exactly what you are trying to achieve that makes the new interface inferior to the previous one, or more detail on why you think it looks bad?

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

>Given bug 465995, we still can't clear browsing history without clearing
>downloads, so the new UI will need to reflect that somehow...

Ok, let's go with just one check box that says "Browsing and Download History" until the dependency issue is resolved.  The subset placement UI we currently have doesn't really work with the flat list box, which we want to have in order to avoid having a massive window, and in case we add more types of history in the future.

Comment 6

[Johnathan Nightingale [:johnath]]

Attachment: WIP

After conversation today on the fx-team call, I agreed to put some details in the bug so that Drew could take a look at this while I'm travelling.

Attached is a partial patch that:

  - Introduces the places history tree to the CRH dialog
  - Fixes the sanitizer to accept a supplied range (but fall back to the timespan pref if one is not supplied)
  - Changes the checkboxes for history items to a listbox autopopulated by the same prefs
  - Drops the "data" checkboxes since they don't map to the timespan parameter

What it doesn't do yet:

  - Populate the UI selection based on the chosen timespan or set the sanitizer's range property (i.e. it currently ignores selection in the places tree and trusts the combo box as before)
  - Supplementary UI behaviour (the details section isn't expandable, the merging of history and downloads hasn't happened, and the mockup's UI for "everything" is not implemented.)

Some other thoughts for Drew/Alex:

  - Changes to this code may impact the clear settings in the preferences ( http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/sanitize.xul ) so we have to step carefully here.  In particular, sanitize.js and sanitize.dtd are shared. The settings dialog from prefs should not have the tree, and should have the "data" section, in my opinion, so I don't think anything needs to change there, the important thing is just to make sure it isn't changed accidentally.

  - I understand the motivation behind removing the permanent items since they don't map to timespan, but I think there is an existing contingent of users that would be losing functionality through this (myself included!), and I think we can serve them too.  I'd propose that when (and only when) the user selects "everything", those get added to the scrollable list of items. The addition is basically invisible at that point, but avoids regressing functionality for users who care, and represents little additional code since we already support clearing these items (though not in a timespan-sensitive way).

If this does land for 3.5, it needs to be marked late-l10n, but I'm not marking it yet, since its future is not yet known.

Comment 7

[Drew Willcoxon :adw]

Attachment: WIP 2

This patch hooks up the duration dropdown to the tree and styles the tree.

A quick observation I had is that I'm not sure how we can implement contiguous range selection in the tree as described in the mockup.  Multiselection is easy, but restricting it to a contiguous range would involve something more, not sure what.  Are there any examples in the codebase?  Also, we need to disable drag and drop in the tree.  Probably sort too?

Comment 8

[Drew Willcoxon :adw]

Attachment: WIP 3

- Uses a custom tree view to prevent drag and drop and sorting by headers.
- Hooks up the tree to actual sanitization, so the dialog is now kind of usable.
- Adds the mockup's warning panel upon removal of all history.  (It's ugly right now.)
- Changes some strings so they better match the mockup, flexes the duration dropdown.

Again I'm not sure what to do about contiguous selection in the tree.  I played around with it, and all I can come up with are ugly hacks.  I wonder if contiguous selection is something that would be better implemented farther down in content.

Comment 9

[Drew Willcoxon :adw]

Attachment: WIP 4

Slowly getting there.

- Adds a progressive disclosure button, whose code and style is cribbed from the prog disclosure button in the bottom-right pane of the Places library.  Currently it's styled OK only on Pinstripe.
- Combines history and downloads prefs into single checkbox per comment 5.
- Adds checkboxes for all privacy prefs per comment 6.
- General progress, correctness

Some notes:
The dialog and the menuitem used to reach it are called "Clear Recent History", but the OK button in the dialog is "Clear Private Data".  Which is it?  (It's private data + recent history + all history seems to me, especially taking into account the permanent items mentioned in comment 6, so I think we should nix "Clear Recent History" in favor of "Clear Private Data", but I don't know the background at all for this feature.)

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

>The dialog and the menuitem used to reach it are called "Clear Recent History",
>but the OK button in the dialog is "Clear Private Data".  Which is it?  (It's
>private data + recent history + all history seems to me, especially taking into
>account the permanent items mentioned in comment 6, so I think we should nix
>"Clear Recent History" in favor of "Clear Private Data", but I don't know the
>background at all for this feature.)

Let's make the button say "Clear" (and default button is Cancel).  The mockup had "clear now" but technically saying now on a button is wrong.

For terminology we are trying to be consistent with this distinction:

things that are implicitly created by the browser -> history
things that are explicitly created by the user -> data

so stuff like cookies and form entries are "history," and things like bookmarks and saved passwords are "data."  The 8 items in the details list in the mockups should all be implicitly created history.

We also want to remove saved passwords and offline website data from the dialog, since we aren't offering to remove other forms of data (like bookmarks), and the inclusion of these two particular things was kind of random.

We are using bug 464204 to track consistent history terminology.

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

If the user selects "everything" perhaps we should change the window title to "Clear All History," we should probably try to get that in before the string freeze on Thursday.

Comment 12

[Nochum Sossonko [:Natch]]

> We also want to remove saved passwords and offline website data from the
> dialog, since we aren't offering to remove other forms of data (like
> bookmarks), and the inclusion of these two particular things was kind of
> random.

Well that's not a perfect comparison as bookmarks (managing/adding/removing)
are a part of the key ui in the app while the other two aren't. Although
passwords do have some ui for this (pm) it isn't all that discoverable (buried
deep in the options dialog). Don't forget that consolidation of private data
removal has got to be somewhat important, there should be some central place
where a user can eradicate any memory of his online adventures. Just my .02.

Comment 13

[Drew Willcoxon :adw]

Alex, thanks for the clarification.  As for the string freeze, the entire dialog will probably not be finished by Thursday if we're relying only on me.  :)  Is it kosher to file a patch that only adds new strings in anticipation of future code changes?  There are other new strings as well -- the warnings when deleting everything and the label(s) for the prog disclosure button.

(In reply to comment #12)
> deep in the options dialog). Don't forget that consolidation of private data
> removal has got to be somewhat important, there should be some central place
> where a user can eradicate any memory of his online adventures. Just my .02.

Alex knows better than I do, but the second non-goal at https://wiki.mozilla.org/Firefox3.1/Sprints/Clear_Recent_History_by_Time_Range#Non_Goals appears to argue against doing that in this bug.

Comment 14

[(no longer active)]

(In reply to comment #13)
> Alex, thanks for the clarification.  As for the string freeze, the entire
> dialog will probably not be finished by Thursday if we're relying only on me. 
> :)  Is it kosher to file a patch that only adds new strings in anticipation of
> future code changes?  There are other new strings as well -- the warnings when
> deleting everything and the label(s) for the prog disclosure button.

You can attach a separate patch containing only the strings, and ask Beltzner's ui-review on that patch, and land it before the string freeze, and then continue to use those strings in your main patch.

Comment 15

[Drew Willcoxon :adw]

Thanks, Ehsan.

Alex has kindly offered to cover the strings for this bug over in bug 464204 so that we make string freeze.  I note the strings we have added so far in our patches up to WIP 4:

browser.properties:
- everythingWarningSingular:  The first paragraph that appears in the warning panel when "Time range to clear" is set to "Everything".  Singular refers to the fact that this is used when there's only one visit in history.  In WIP 4 this warning paragraph is hidden if the user's history is empty.
- everythingWarningPlural:  Same as above, but used when the number of visits in history is != 1.  In WIP 4 this warning paragraph is hidden if the user's history is empty.

sanitize.dtd:
- clearDuration.dateColumn:  The title of the tree's "Visit Date" column header.
- clearDuration.nameColumn:  The title of the tree's "Name" column header.
- detailsSection.label:  The title of the "Details" disclosure button.  Might we want to specify both a "More" title and a "Less" title?  We can always make them the same.
- everythingWarning.width:  The width of description element used to show everythingWarningSingular/Plural.  Without a width the text doesn't wrap, causing the width of the entire dialog to be the width of the unwrapped text.  Its value should be a little less than the window.width entity.
- everythingUndoWarning:  The second paragraph that appears in the warning panel when "Time range to clear" is set to "Everything".

Comment 16

[Johnathan Nightingale [:johnath]]

(In reply to comment #15)
> browser.properties:
> - everythingWarningSingular:  The first paragraph that appears in the warning
> panel when "Time range to clear" is set to "Everything".  Singular refers to
> the fact that this is used when there's only one visit in history.  In WIP 4
> this warning paragraph is hidden if the user's history is empty.
> - everythingWarningPlural:  Same as above, but used when the number of visits
> in history is != 1.  In WIP 4 this warning paragraph is hidden if the user's
> history is empty.

Once you get into plurals, you're going to want to read:

https://developer.mozilla.org/En/Localization_and_Plurals

(Summary - it's not enough to just have two forms)

Comment 17

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

(In reply to comment #15)
> Alex has kindly offered to cover the strings for this bug over in bug 464204 
> so that we make string freeze.  I note the strings we have added so far in our
> patches up to WIP 4:

The strings should be attached in a patch here, not on bug 464204 - it's all part of the fix for this bug.

Comment 18

[Drew Willcoxon :adw]

(In reply to comment #16)
> Once you get into plurals, you're going to want to read:
> 
> https://developer.mozilla.org/En/Localization_and_Plurals
> 
> (Summary - it's not enough to just have two forms)

I'm learning some things with this bug. :) We can get rid of everythingWarningSingular and everythingWarningPlural in browser.properties and replace them with something like:

# LOCALIZATION NOTE (sanitizeEverythingWarning): Semi-colon list of plural
# forms. See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
# #1 is replaced with the number of history visits. %S is replaced with the
# date of the oldest visit.
sanitizeEverythingWarning=All history will be cleared.  Your history includes one page visit since %S.;All history will be cleared.  Your history includes #1 page visits since %S.

Alex, I assume you're still taking care of the strings patch but if you need me to do it I can.

Comment 21

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

Attachment: strings only patch for Clear Recent History

Includes the string fixes for bug 464588 and bug 464207 as well. Note that some of these new strings aren't yet used, but will be when the rest of the logic for this patch comes through.

Gavin: this also doesn't yet fix the Browsing History / Download History problem you mention in comment 2, but includes a new string which will allow us to do so after string freeze.

Comment 22

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

Attachment: strings only patch for Clear Recent History (v2)

Missed one. This is better.

Comment 23

[Drew Willcoxon :adw]

Attachment: strings only patch for Clear Recent History (v3)

Changes re: Gavin's comments.

Comment 24

[Drew Willcoxon :adw]

Attachment: WIP 5

Based on the "strings only patch for Clear Recent History (v3)" patch.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Attachment: review comments for attachment 368391

Just attaching these for posterity, they're already addressed in the latest patch.

Comment 26

[Drew Willcoxon :adw]

Attachment: strings only patch for Clear Recent History (v4)

Separates the 2nd paragraph undo warning and the 1st paragraph warning.  Includes new property to show when there are no visits in history.

Comment 27

[Drew Willcoxon :adw]

Attachment: WIP 6

Based off of "strings only patch for Clear Recent History (v4)" patch.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-1.9.1/rev/4053c44e993e

Comment 29

[:Gavin Sharp [email: [e-mail]]]

We should get the strings patch in on trunk as well, just to keep them in sync (tree's red, can't check in at the moment).

Comment 30

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/542e7bcb3e0f

Comment 31

[Drew Willcoxon :adw]

Why does clearing cookies exclude start time and not respect end time at all?  All other items include both start and end times.  Seems like a problem.  Relevant code is Sanitizer.prototype.items.cookies.clear() in browser/base/content/sanitize.js.

Alex:
1. Could you provide some direction on selection in the tree?  No widget like what's in the mockup exists presently.  Are there good alternatives that we can use today?
2. Where are the exclamation icons that appear when clearing everything?
3. Where are the glyphs for the details progressive disclosure button?

Comment 32

[(no longer active)]

(In reply to comment #31)
> Why does clearing cookies exclude start time and not respect end time at all? 
> All other items include both start and end times.  Seems like a problem. 
> Relevant code is Sanitizer.prototype.items.cookies.clear() in
> browser/base/content/sanitize.js.

Currently the end-time is always now, so I think that code incorrectly assumes that.  It should be easy to fix though.

> 2. Where are the exclamation icons that appear when clearing everything?

On Windows and Linux: <chrome://global/skin/icons/warningGhosted-64.png>.  I'm not sure if an alternative exists on Mac, though.

> 3. Where are the glyphs for the details progressive disclosure button?

On Windows and Linux: <chrome://global/skin/icons/expand.png> and <chrome://global/skin/icons/collapse.png>.  Again I'm not sure about the Mac equivalent.

Comment 33

[Alex Faaborg [:faaborg] (Firefox UX)]

>On Windows and Linux: <chrome://global/skin/icons/warningGhosted-64.png>.  I'm
>not sure if an alternative exists on Mac, though.

According to my records the OS X icon wasn't created (the original interface using that icon was cut for Firefox 3), in the meantime let's use chrome://global/skin/icons/warning-64.png on OS X.

>On Windows and Linux: <chrome://global/skin/icons/expand.png> and
><chrome://global/skin/icons/collapse.png>.  Again I'm not sure about the Mac
>equivalent.

OS X has:

chrome://browser/skin/places/expander-open.png
chrome://browser/skin/places/expander-closed.png

Use these when the use clicks down on the control:

chrome://browser/skin/places/expander-open-active.png
chrome://browser/skin/places/expander-closed-active.png

Comment 34

[Drew Willcoxon :adw]

Attachment: WIP 7

Got the continuous draggable tree selection working.  You can drag it with the mouse, move it with the keyboard, and click on rows to move it immediately.  As I discussed with Alex, the selection remains pinned to the top of the tree, so you can only move the bottom (the start time).

Stole a "Custom..." string from some print settings DTD to show in the duration dropdown when the user sets a custom time by changing selection in the tree.  Notice that when you drag back to one of the predetermined durations, the dropdown is updated to reflect that.

The high-hanging fruit is pretty much there, just needs polish.

This is a lot of work.  I can't believe it's been 11 days since my last WIP.  I've been going pretty hard at it.  The tests for this thing are gonna be awful.

Comment 35

[Axel Hecht [:Pike]]

Looking at http://mxr.mozilla.org/l10n-mozilla1.9.1/search?string=customPrompt.title&find=printPageSetup.dtd$, the "Custom..." string is translated in the sense of "Personalize..." rather often, I'm not sure that's a good choice. And that's just for the languages in latin script. Japanese and friends look long enough so that they could actually mention "print".

Can we push the "Custom..." part into a separate patch for detailed evaluation?

Comment 36

[Drew Willcoxon :adw]

Attachment: 2nd strings only patch (v1)

I'm not sure what the protocol is here for asking for strings after string freeze, but as you requested, here's a new strings patch.  It's based off the first strings patch we already pushed.

The "Custom" string rationale is described in comment 34.  An alternative to using a new string to represent custom duration selection is to leave the dropdown blank, e.g., an empty string.

As discussed with Axel and Beltzner in email and per Beltzner's suggestion, this patch also changes entity itemHistoryAndDownloads.label so that it uses an ampersand (like itemFormSearchHistory.label) but does not rename the entity.

Comment 37

[Axel Hecht [:Pike]]

Comment on attachment 370030 [details] [diff] [review]
2nd strings only patch (v1)

My question wasn't really to get another patch to preland strings, I somewhat consider that a nono at this point. I was wondering if the complete code path for "custom" could be factored into a separate patch, so that we could for example evaluate on whether to take it post-B4 or drop it.

r- to denote that I don't want to land more strings for B4.

Regarding the localization note, the mockup is rather useless to figure out what's going on, one has to almost reverse engineer to make an educated guess. From what I envision happening, is "Custom" a good label, or would "Selection" be better?

Comment 38

[Drew Willcoxon :adw]

(In reply to comment #37)
> Regarding the localization note, the mockup is rather useless to figure out
> what's going on, one has to almost reverse engineer to make an educated guess.

That's true.  Alex, maybe some help here?

> From what I envision happening, is "Custom" a good label, or would "Selection"
> be better?

There's always a selection.  (I'm not sure if you tried the previous WIPs, but WIP 7 is much different and based on recent conversation with Alex.)  When the user chooses "Today" for example, all rows in the tree that were visited today are selected.  When "Last Four Hours", all rows that were visited in the last four hours are selected.  But the user can manually adjust the selection so that it doesn't fall on one of the predefined boundaries.  In that case it would be good if something like "Custom" were displayed in the dropdown.

The custom part could be refactored into another patch so that the user couldn't manually adjust the selection, yes, but it would be too bad if we had to take it out because one string that would be nice to have is missing.  Hopefully there's a better compromise somewhere.  If b4 is the last beta, would it even be possible to add the custom part back in before 3.5 final?

Comment 39

[Alex Faaborg [:faaborg] (Firefox UX)]

>Regarding the localization note, the mockup is rather useless to figure out
>what's going on, one has to almost reverse engineer to make an educated guess.
>From what I envision happening, is "Custom" a good label, or would "Selection"
>be better?

The reason I selected "Custom" is because it needs to complete the idea "time range to clear."

A term like "selection" implies that when it is set to something else (like 2 hours), the selection is not cleared.  In reality the selection is always cleared, and "custom" just describes what is currently selected.

Let's leave the text blank for now and file a separate bug to add the string after this bug lands.

Comment 40

[Drew Willcoxon :adw]

Attachment: WIP 8

Added a new test for the UI and updated an existing one for lower-level custom sanitize ranges.  Johnathan, code is almost ready for review, probably the next patch.  Alex, need icons for grippy affordance and the clear-everything warning on OS X.  Still to do: clean up code, style Winstripe and Gnomestripe correctly.

Comment 41

[Johnathan Nightingale [:johnath]]

(In reply to comment #40)
> Created an attachment (id=370487) [details]
> WIP 8
> 
> Added a new test for the UI and updated an existing one for lower-level custom
> sanitize ranges.  Johnathan, code is almost ready for review, probably the next
> patch.  Alex, need icons for grippy affordance and the clear-everything warning
> on OS X.  Still to do: clean up code, style Winstripe and Gnomestripe
> correctly.

Drew - great to hear, really great. I will happily give drive-bys, but please flag mconnor for the official review - he reviewed my earlier CRH work (he may deflect to someone else, but is the best first target).

Comment 42

[Drew Willcoxon :adw]

Attachment: for review v1

Requesting mconnor's review per Johnathan's suggestion in comment 41.

Some notes, maybe helpful:
- Need a couple of icons from Alex (and a review of the stylesheets?).  The tree selection's grippy is currently drawn as a separator.
- The drag-and-drop code used to implement dragging the grippy makes it so that the mouse cursor during drag is the platform's drag-and-drop move icon.  I couldn't find any way around that.  On Gnome especially it's not ideal.  Obviously a grabbing hand cursor would be better.
- The patch updates an existing test so that it tests clearing custom timespans.
- Includes a new test for the new UI.
- You can click a row in the tree to make the selection jump immediately.
- You can use the Up, Down, Page Up/Down, Home, End keys in the tree.
- See Alex's comment 39 about selecting a custom timespan.  The duration dropdown should display "Custom", but I didn't realize we needed that before string freeze, so it's left blank for now.

Comment 43

[(no longer active)]

Comment on attachment 370593 [details] [diff] [review]
for review v1

I did a partial drive-by review on the patch.  I think it looks great overall.  Please find my comments below.

>diff --git a/browser/base/content/sanitize.js b/browser/base/content/sanitize.js
>-            if (cookie.creationTime > this.range[0])
>-              // This cookie was created after our cutoff, clear it
>+            if (cookie.creationTime >= this.range[0])
>+              // This cookie was created on or after our cutoff, clear it

Any particular reason why you're ignoring the end of the range here?

>diff --git a/browser/base/content/sanitize.xul b/browser/base/content/sanitize.xul
>+<?xml-stylesheet href="chrome://browser/content/places/places.css"?>

I don't think any style rule in this file is being used here.

> <prefwindow id="SanitizeDialog" type="child"
>             xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>             dlgbuttons="accept,cancel"
>             title="&sanitizeDialog2.title;"
>+            noneverythingtitle="&sanitizeDialog2.title;"

May I suggest another name, like clearrangetitle?

>diff --git a/browser/base/content/sanitizeDialog.css b/browser/base/content/sanitizeDialog.css
>+#placesTreechildren {
>+  -moz-user-focus: normal !important;

Is important needed here?

>+}
>+
>+#placesTreechildren::-moz-tree-cell(grippyRow),
>+#placesTreechildren::-moz-tree-cell-text(grippyRow),
>+#placesTreechildren::-moz-tree-image(grippyRow) {
>+  cursor: -moz-grab !important;

What about here?

>diff --git a/browser/base/content/sanitizeDialog.js b/browser/base/content/sanitizeDialog.js

I didn't get to review the changes to this file...

>diff --git a/browser/base/content/test/browser_sanitizeDialog.js b/browser/base/content/test/browser_sanitizeDialog.js

As a general comment, for testing UI elements I think it's better to either simulate user activity by synthesizing mouse and keyboard input (see EventUtils.js) or at least by dispatching the resulting events.  This has two benefits over your approach: the test doesn't need changing if a function is modified, and the test also ensures that the UI elements are properly tied up with implementation functions (for example, event handlers are set correctly, etc.

Comment 44

[Drew Willcoxon :adw]

(In reply to comment #43)
> Any particular reason why you're ignoring the end of the range here?

The existing code already ignores the end (i.e., uses now as the end), and with the redesign the end is always now, so I decided to leave it be so as to not risk breaking other code.

> >+<?xml-stylesheet href="chrome://browser/content/places/places.css"?>
> 
> I don't think any style rule in this file is being used here.

Ah, you're right.

> May I suggest another name, like clearrangetitle?

OK, it's just that everywhere else the warning is referred to using "everything", as in the "clear everything warning".  "Everything" matches the mockup.

> Is important needed here?

Maybe not.  I borrowed some of the CSS from elsewhere, but I should check whether it's really needed.

> >diff --git a/browser/base/content/sanitizeDialog.js b/browser/base/content/sanitizeDialog.js
> 
> I didn't get to review the changes to this file...

That's most of the patch. ;)

> >diff --git a/browser/base/content/test/browser_sanitizeDialog.js b/browser/base/content/test/browser_sanitizeDialog.js
> 
> As a general comment, for testing UI elements I think it's better to either
> simulate user activity by synthesizing mouse and keyboard input (see
> EventUtils.js) or at least by dispatching the resulting events.  This has two
> benefits over your approach:

Hmm, I'm synthesizing keypresses to move the grippy.  I agree it's important in that case.  But other than that I'm only checking checkboxes, selecting a row in the dropdown, and "pressing" the dialog's OK and Cancel buttons.  I really don't think it's important to dispatch an event just to check a checkbox or to do the other two things.

Thanks, Ehsan.

Comment 45

[Johnathan Nightingale [:johnath]]

Tryserver'd this a while ago and forgot.  In case it helps anyone:

https://build.mozilla.org/tryserver-builds/2009-04-02_05:[e-mail]-adw-crh-sprint/

Comment 46

[Marcia Knous [:marcia]]

Attachment: Screenshot of issue while running tryserver build

Jonathan: Here is a screenshot I grabbed while running the tryserver build on the mac. The only slightly unusual thing I did was import some history from Safari. So my STR are something like this:

1. Launch the build with a fresh profile
2. Import some history from Safari
3. Toggle some of the different parameters
4. Highlight the first few entries in my list - that seems to trigger what you see in the screenshot.

Comment 47

[Drew Willcoxon :adw]

(In reply to comment #46)
Thanks for trying it out.  That's how it's (kind of) supposed to work.  There's a blank entry in the dropdown when you select a custom timespan.  The string "Custom" should be there really, but we missed string freeze for that.  See comment 42, which summarizes the current state of the patch.

Comment 48

[Vlado Valastiak [:wladow]]

Comment on attachment 368442 [details] [diff] [review]
strings only patch for Clear Recent History (v4)

>+sanitizeEverythingWarning=All history will be cleared.  Your history includes one page visit since %S.;All history will be cleared.  Your history includes #1 page visits since %S.

After playing a bit with tryserver build I've got two main concerns regarding this date thing:

1) we are taking the date format and locale from OS (with .toLocaleDateString) - this means the date locale doesn't necessarily match browser's locale. And as the date is integral part of the sentense it looks really awful in this case. 

2) even if both browser and OS are using the same locale the date format may still suck. For example the sk locale - we do have different month's name forms for the cases when a date stays alone somewhere and when a date is a part of a string. Sadly we are getting the wrong one here. So instead of "...page visits since 19. *oktobra* 2008" I'm getting "...page visits since 19. *oktober* 2008". Too bad.
   
Given that we are string frozen, I'm a little unsure about how to proceed here. F.e. we could use toolkit's dateFormat.properties to get localized dates, but we'd still need a new set of month names - for the case when a date is a part of sentence. The current one defined in the file isn't applicable here...

Any thoughts? I'd like to avoid any unsufficient L10n workarounds, but I suppose I'm gonna need one (at least for 3.5) as the whole date thing is too complex to fix in a few days till our code freeze and even with no string changes allowed.

Comment 49

[Mike Connor [:mconnor]]

Comment on attachment 370593 [details] [diff] [review]
for review v1

Discussed with adw and faaborg, we're going to redo some of the custom timeframe stuff as a second step, more coming from them.

Comment 50

[Alex Faaborg [:faaborg] (Firefox UX)]

a new mockup reducing the scope so we can get this landed: http://people.mozilla.com/~faaborg/files/shiretoko/clearRecentHistoryi6.png

this doesn't address the date L10 concerns, we might just need to remove the date part until we get that sorted out

Comment 51

[Drew Willcoxon :adw]

Attachment: for review v2

- Matches mockup in comment 50.
- In light of comment 48, simply displays "All history will be cleared." instead of string customized to number and time of visits.
- That means the warning text doesn't fill up the entire width of the dialog.  I thought it looks better centered than scrunched up to the side, so I did it.

Comment 52

[Drew Willcoxon :adw]

Attachment: for review v2 screenshots

Comment 53

[Dão Gottwald [:dao]]

Comment on attachment 372586 [details] [diff] [review]
for review v2

a few drive-by nits:

>+    <script type="application/x-javascript"
>+            src="chrome://browser/content/sanitize.js"/>

application/javascript

>+          <box id="sanitizeEverythingWarningImgBox"/>

This should be a xul:image, and I'd suggest sanitizeEverythingWarningIcon as the id.

>+#sanitizeEverythingWarningImgBox {
>+  width:      48px;
>+  height:     48px;
>+  background: url("chrome://global/skin/icons/warning-large.png") top center no-repeat;
>+}

... and this should be list-style-image instead of background.

Generally there should be only one space after a colon, otherwise touching these style rules again will be painful.

Comment 54

[Mike Connor [:mconnor]]

Comment on attachment 372586 [details] [diff] [review]
for review v2

>@@ -133,18 +137,18 @@ Sanitizer.prototype = {
>-            if (cookie.creationTime > this.range[0])
>-              // This cookie was created after our cutoff, clear it
>+            if (cookie.creationTime >= this.range[0])
>+              // This cookie was created on or after our cutoff, clear it
>               cookieMgr.remove(cookie.host, cookie.name, cookie.path, false);

this change is almost certainly unnecessary.  We're dealing in microseconds here.

Scope creeping patches just makes reviewers grumpy, fwiw. :)

>diff --git a/browser/base/content/sanitize.xul b/browser/base/content/sanitize.xul

>+      <groupbox id="sanitizeEverythingWarningBox" align="center">
>+      </groupbox>

Ignoring this bit since we're updating the UI here.

>+    <hbox id="detailsExpanderWrapper" align="center">

Filed bug 488401 on fixing <expander> to be usable.


>+
>+  checkPrefs : function ()
>+  {
>+    var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                      .getService(Components.interfaces.nsIPrefService);

use the Cc/Ci consts here for readability, please.

>+
>+  /**
>+   * Called when the value of a preference element is synced from the actual
>+   * pref.  Enables or disables the OK button appropriately.
>+   */
>+  onReadGeneric: function ()
>+  {
>+    var found = false;
>+    for (var i = 0; i < this.sanitizePreferences.childNodes.length; ++i) {

let instead of var is preferred in for loops now, generally.

>+      var preference = this.sanitizePreferences.childNodes[i];
>+      if (preference.value && !preference.disabled) {
>+        found = true;
>+        break;
>+      }
>+    }

>+    // We don't update the separate history and downloads prefs until
>+    // dialogaccept.  So we need to take into account the checked state of
>+    // the combined history-downloads checkbox.
>+    var combinedCb = document.getElementById("history-downloads-checkbox");
>+    found = found || combinedCb.checked;

So, I think I'd prefer to check this first, since it's generally going to be checked, and just use a while statement to loop if !found.  Pretty sure we'd skip the loop nearly every time this way.

Looks good so far, but I'll want to see the XUL/CSS changes to the everything warning before it lands.

Comment 55

[Drew Willcoxon :adw]

Attachment: after review v3

Comment 56

[Drew Willcoxon :adw]

Attachment: after review v3.1

browser/base/content/test/Makefile.in didn't apply cleanly to trunk, fixed.  Will probably need to modify patch a little to land on 1.9.1, am checking that now.

Comment 57

[Drew Willcoxon :adw]

Attachment: after review v3.2

OK, forgot to change string "Browsing and Download History" to "Browser & Download History" as Beltzner, Axel, and I discussed.

Comment 58

[Axel Hecht [:Pike]]

Comment on attachment 372902 [details] [diff] [review]
after review v3.2

>diff --git a/browser/locales/en-US/chrome/browser/sanitize.dtd b/browser/locales/en-US/chrome/browser/sanitize.dtd
>--- a/browser/locales/en-US/chrome/browser/sanitize.dtd
>+++ b/browser/locales/en-US/chrome/browser/sanitize.dtd
>@@ -32,7 +32,7 @@ that require it.  -->
> <!-- LOCALIZATION NOTE (item*): itemHistoryAndDownloads.* and
>      itemBrowsingHistory.* will never be used at the same time, so they can
>      have the same accesskey. -->
>-<!ENTITY itemHistoryAndDownloads.label     "Browsing and Download History">
>+<!ENTITY itemHistoryAndDownloads.label     "Browsing &amp; Download History">
> <!ENTITY itemHistoryAndDownloads.accesskey "B">
> <!ENTITY itemBrowsingHistory.label         "Browsing History">
> <!ENTITY itemBrowsingHistory.accesskey     "B">
>@@ -58,5 +58,5 @@ that require it.  -->
>      mockup at bug 480169 -->
> <!ENTITY sanitizeEverythingUndoWarning     "This action cannot be undone.">
> 
>-<!ENTITY dialog.width                 "32em">
>+<!ENTITY dialog.width                 "28em">
> <!ENTITY column.width                 "14em">

I'd prefer to not land string changes on 1.9.1 at this point, but to push those out post B4, when we land the rest.

I somehow saw the dialog size coming, tbh.

Comment 59

[Drew Willcoxon :adw]

Attachment: after review v3.3

Argh, I misunderstood at what point we were going to update a couple of strings.  This patch updates no strings.

Comment 60

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9498df7e5ecd

Comment 61

[Drew Willcoxon :adw]

Attachment: 1.9.1 patch

Trunk patch does not apply cleanly, browser/base/content/test/Makefile.in is the only thing that needs updating.

Comment 62

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3f13c5f70070

Comment 63

[Axel Hecht [:Pike]]

This is pending l10n testing by the l10n drivers team, and should block Beta 4 until we're done.

Comment 64

[AKIHIRO Misaki (a.k.a Kuden)]

chrome://browser/skin/preferences/preferences.css
http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/preferences/preferences.css#177
> /**
>  * Clear Private Data
>  */
> #SanitizeDurationBox {
>   padding-bottom: 10px;
> }
> 
> #sanitizeDurationChoice {
>   margin: 0; 
> }
> 
> #sanitizeDurationLabel {
>   -moz-margin-start: 3px;
> }
> 
> #SanitizeDialogPane > groupbox {
>   margin-top: 0;
> }

I feel that we should move these to chrome://browser/skin/sanitizeDialog.css ("#SanitizeDialogPane > groupbox" might be unnecessary).

Comment 65

[Axel Hecht [:Pike]]

L10n testing passed, the resulting bugs are either fixed or not blocking. Marking fixed1.9.1 again.

Comment 66

[Marcia Knous [:marcia]]

As I am going through the various Litmus test cases, it occurred to me that we previously did allow users to clear Offline Site data using Clear Recent History. Is there a reason that was left off the list of items that can be cleared?

Comment 67

[Drew Willcoxon :adw]

(In reply to comment #66)

Second-to-last paragraph in comment 10 might shed some light.

Comment 68

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090417 Shiretoko/3.5b4pre, Windows Vista equivalent build, Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090417 Shiretoko/3.5b4pre and the Linux equivalent build. Adding keyword.

Comment 69

[Alex Faaborg [:faaborg] (Firefox UX)]

>it occurred to me that we
>previously did allow users to clear Offline Site data using Clear Recent
>History. Is there a reason that was left off the list of items that can be
>cleared?

to explain this a little more, offline site data doesn't group into history since it will include things that the user explicitly created, like to do lists, spreadsheets, email, etc.

Comment 70

[Chris Ilias [:cilias]]

user-doc-complete
<http://support.mozilla.com/kb/Clear+Recent+History?bl=n>

Comment 71

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk with builds on Windows and OS X: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090506 Minefield/3.6a1pre ID:20090506031346

Comment 72

[:Paolo Amadini]

Pushed dependent bug 1114607 to fx-team with this bug number by mistake:

https://hg.mozilla.org/integration/fx-team/rev/5ce51f227425

Comment 73

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ce51f227425

Comment 74

[[e-mail]]

Comment on attachment 372910 [details] [diff] [review]
after review v3.3

>+      <menulist id="sanitizeDurationChoice"
>+                preference="privacy.sanitize.timeSpan"
>+                onselect="gSanitizePromptDialog.selectByTimespan();"
If you'd used oncommand rather than onselect...

>+  init: function ()
>+  {
>+    // This is used by selectByTimespan() to determine if the window has loaded.
>+    this._inited = true;
... then you wouldn't have had to worry about the select events that generated when the value changes from code.