When selecting to clear all history, the dialog insists that "All history will be cleared", even when the "Visited pages and downloads" option isn't checked. Doesn't this effectively force the user to always click on "Details" to see what Shiretoko understands as "history" this time? (BTW: Either hiding the details per default or hiding the advanced options when displaying the details (through forcing the user to scroll down) is one too many IMHO. Even though the dialog is supposed to feel simpler, it feels more complex for me.)

Well, I agree with Simon. To take this even further: the text before the drop-down says "Time range to clear", and the first four items on the list are indeed time ranges, but the last item "Everything" is not a time range. Everything means every item stored at any time, IMO, which only happens to be the case where all of the check boxes are checked. I think changing "Everything" to something like "All times" (probably a better piece of text, of course), should resolve some of the confusion. Don't know if we can do that post string freeze, though.

Alex discusses what makes history history at bug 464204 and bug 480169 comment 10 and probably elsewhere, too.

(In reply to comment #2) > Alex discusses what makes history history at bug 464204 and bug 480169 comment > 10 and probably elsewhere, too. Yes, but please note that I'm talking about the time range, and not the history constituents.

(In reply to comment #3) > Yes, but please note that I'm talking about the time range, and not the history > constituents. Yes, I was mainly addressing Simon's comments of understanding "history". :)

(In reply to comment #2) > Alex discusses what makes history history at bug 464204 Doesn't make this one less confusing, though: I was telling Shiretoko to clear all history - and it told me that it'd do so - and found nonetheless a full History menu and History sidebar afterwards. (Of course, I had manually unchecked the "Browsing history" option a few months ago for this to happen.)

>(Of course, I had manually >unchecked the "Browsing history" option a few months ago for this to happen.) These types of UI issues inevitably come up when we drop in new interfaces at the last minute. I agree that for your case the interface is confusing, but we might not be able to fix it entirely given the string freeze. I wonder if we should try expanding the details area if the user has made any modifications (similar to how the privacy prefpane is set to display all of the prefs if the user has made any changes). When strings re-open again, we could try to adjust the message to account for situations where the user has only selected a subset of items, although that gets pretty complicated given all of the permutations. Perhaps for all items say "history" for a single item, say it's name, and for a subset just say "parts of your history"? Either way since the details area would then be expanded, the user would ideally get that all=time range, not types.

Strings are not going to re-open on 1.9.1. That doesn't exclude real bustage fixes, but "re-open" doesn't reflect the fact that we're string frozen.

>but "re-open" doesn't reflect the fact that we're string frozen. yeah, I meant for firefox.next

(In reply to comment #6) > I wonder if we should try expanding the details area if the user has made any > modifications Why not just always expand the details area and get rid of the expander? Trusting Firefox to do the right thing is good, verifying with a glance that it is indeed about to do it, is better... ;-) And it's really not like that dialog is overcomplicated.

(In reply to comment #9) > Why not just always expand the details area and get rid of the expander? Ugh, where were you guys when we were working on this thing? :)

The UI will get more complex later when we add in the control for setting the specific time range (which was removed at the last minute because we wanted to improve it and add it back in firefox.next). Overall though, we believe that clearing a specific subset of history items is a very minority use case for this dialog, and we are trying to base the design around that assumption.

(In reply to comment #11) > The UI will get more complex later Then it will get more obvious what happens. Question is, are things obvious enough with the reduced version we've got for Shiretoko? Maybe Beta 4 feedback will help answer that...

(In reply to comment #0) > When selecting to clear all history, the dialog insists that "All history will > be cleared", even when the "Visited pages and downloads" option isn't checked. That seems wrong. s/history/selected items/ perhaps? (In reply to comment #9) > Why not just always expand the details area and get rid of the expander? That doesn't feel like a good solution, though perhaps we should always expand in the case where "Everything" is selected as the time range, since it's an undo-able action. That would also allow us to not impact strings. Regardless, yes, let's get feedback.

Alex, we should fix this in 3.6 and soon before string freeze; what did you think of my suggestion in comment 13?

yeah, so in the case that both: 1) the time range is set to everything 2) the user has modified the set of items to clear we should: -make sure that the details are expanded (this will likely be the case since it remembers its state, but it could potentially have been collapsed again) -modify the string to read: All selected items will be cleared. This action cannot be undone. >though perhaps we should always expand >in the case where "Everything" is selected as the time range, since it's an >undo-able action. I think this actually distracts from the warning message displayed by adding more complexity to the window, so overall wouldn't help.

Waiting for the final string here...

Can anyone find any problems with the string: "All selected items will be cleared." Which we would only use if the set of items to clear was modified, and the range of everything was selected. Unless someone points out a problem with that string, let's go with that.

This should appear in addition to "This action cannot be undone.", right? If so, then I think it's fine.

yep, so: All history will be cleared. This action cannot be undone. Turns into: All selected items will be cleared. This action cannot be undone.

Comment on attachment 395801 [details] [diff] [review] Patch (v1) From comment 16: > 1) the time range is set to everything > 2) the user has modified the set of items to clear > > we should: > > -make sure that the details are expanded (this will likely be the case since it > remembers its state, but it could potentially have been collapsed again) > -modify the string to read: > > All selected items will be cleared. > This action cannot be undone. This patch implements only the last part, which means that said selected items may not be visible. >+sanitizeEverythingWarning2=All selected items will be cleared. Your history includes one page visit since %S.;All selected items will be cleared. Your history includes #1 page visits since %S. Is that string used somewhere?

(In reply to comment #22) > This patch implements only the last part, which means that said selected items > may not be visible. Sorry, I had completely forgot about the first part. The new patch addresses that as well. > >+sanitizeEverythingWarning2=All selected items will be cleared. Your history includes one page visit since %S.;All selected items will be cleared. Your history includes #1 page visits since %S. > > Is that string used somewhere? No, but it will be when the new CRH dialog lands.

Comment on attachment 395839 [details] [diff] [review] Patch (v2) > ensureWarningIsInited: function () > { >+ this.toggleItemList(true); I'm afraid this does more than the method name suggests. The toggle call should be moved to where ensureWarningIsInited is called or ensureWarningIsInited should be renamed, or something like that. > * Called by the item list expander button to toggle the list's visibility. >+ * >+ * @param aForceShow >+ * If true, only show the list and don't try to hide it if it's >+ * already visible. > */ >- toggleItemList: function () >+ toggleItemList: function (aForceShow) Again, the extra param makes the method behave differently from what its name suggests. Can you add showItemList and hideItemList methods which toggleItemList would call?

Addressed both comments.

Comment on attachment 396406 [details] [diff] [review] Patch (v3) >+ showItemList: function () >+ { The curly bracket should go on the function head's line. I know that file's prevailing style is different, but maintaining doesn't seem worthwhile. > toggleItemList: function () > { > var itemList = document.getElementById("itemList"); > var expanderButton = document.getElementById("detailsExpander"); expanderButton isn't used in that method anymore. > // Showing item list ... > // Hiding item list These comments are rather useless now.

Re-reading comment 16, it seems that the patch is still not quite what we want. As I understand it, if the user has not modified the set of items to clear, details should not be expanded and the text should not read "All selected items will be cleared."

This needs to land before September 14th if it's to make string freeze.

Strings-only patch for landing before string freeze. The code patch will follow.

I made the list of prefs to check for default values extensible so that extension developers can simply append tot he prefsForDefault array if they add new prefs which can be cleared from the CRH dialog. I also moved the display logic of the details pane into ensureWarningIsInited and renamed it to prepareWarning to better describe what the method does now.

Why don't you walk the itemList child nodes to get the relevant prefs?

(In reply to comment #32) > Why don't you walk the itemList child nodes to get the relevant prefs? Done.

Comment on attachment 400358 [details] [diff] [review] Code Patch (v2) >- ensureWarningIsInited: function () >+ prepareWarning: function () > { nit: while you're touching this, please move the curly bracket and the function head on the same line >+ if (this._checkDefaultValues()) I think this would be easier to follow as hasCustomizedItemSelection or something along these lines, with reversed return values... >+ warningStringID = "sanitizeEverythingNoVisitsWarning"; >+ else { >+ warningStringID = "sanitizeEverythingNoVisitsWarning2"; The latter needs a new identifier. Since it's not a revision of sanitizeEverythingNoVisitsWarning anymore, sanitizeEverythingNoVisitsWarning2 doesn't make sense. >+ _checkDefaultValues: function () { >+ let checkboxes = document.querySelectorAll("#itemList > [preference]"); >+ for (let i = 0; i < checkboxes.length; ++i) { >+ let pref = document.getElementById(checkboxes[i].getAttribute("preference")); >+ if (pref && pref.value != pref.defaultValue) pref should never be null here, right? In that case, drop "pref &&"...

(In reply to comment #34) > (From update of attachment 400358 [details] [diff] [review]) > >- ensureWarningIsInited: function () > >+ prepareWarning: function () > > { > > nit: while you're touching this, please move the curly bracket and the function > head on the same line Done. > >+ if (this._checkDefaultValues()) > > I think this would be easier to follow as hasCustomizedItemSelection or > something along these lines, with reversed return values... Done. > >+ warningStringID = "sanitizeEverythingNoVisitsWarning"; > >+ else { > >+ warningStringID = "sanitizeEverythingNoVisitsWarning2"; > > The latter needs a new identifier. Since it's not a revision of > sanitizeEverythingNoVisitsWarning anymore, sanitizeEverythingNoVisitsWarning2 > doesn't make sense. Changed it to sanitizeSelectedNoVisitsWarning. > >+ _checkDefaultValues: function () { > >+ let checkboxes = document.querySelectorAll("#itemList > [preference]"); > >+ for (let i = 0; i < checkboxes.length; ++i) { > >+ let pref = document.getElementById(checkboxes[i].getAttribute("preference")); > >+ if (pref && pref.value != pref.defaultValue) > > pref should never be null here, right? In that case, drop "pref &&"... No, it can be if an extension adds an element under #itemList without a matching <preference> element. Better safe than sorry, right? :-)

(In reply to comment #35) > > >+ let checkboxes = document.querySelectorAll("#itemList > [preference]"); > > >+ for (let i = 0; i < checkboxes.length; ++i) { > > >+ let pref = document.getElementById(checkboxes[i].getAttribute("preference")); > > >+ if (pref && pref.value != pref.defaultValue) > > > > pref should never be null here, right? In that case, drop "pref &&"... > > No, it can be if an extension adds an element under #itemList without a > matching <preference> element. Better safe than sorry, right? :-) You explicitly look for [preference], so if there's no matching <preference> element, that would be a bug.

Comment on attachment 400388 [details] [diff] [review] Code Patch (v3) >+ if (this.hasCustomizedItemSelection()) { >+ warningStringID = "sanitizeSelectedNoVisitsWarning"; >+ this.showItemList(); >+ } >+ else >+ warningStringID = "sanitizeEverythingNoVisitsWarning"; nit: } else { warningStringID = "sanitizeEverythingNoVisitsWarning"; } The "NoVisits" part of the identifier doesn't seem to reflect reality. > /** >+ * Check if all of the privacy.cpd.* preferences have their default value. >+ */ >+ hasCustomizedItemSelection: function () { The comment should refer to the item list rather than "all of the privacy.cpd.* preferences". >+ let checkboxes = document.querySelectorAll("#itemList > [preference]"); >+ for (let i = 0; i < checkboxes.length; ++i) { >+ let pref = document.getElementById(checkboxes[i].getAttribute("preference")); >+ if (pref && pref.value != pref.defaultValue) see previous comment

Comment on attachment 400389 [details] [diff] [review] Strings patch (v2) >+# LOCALIZATION NOTE (sanitizeSelectedNoVisitsWarning): Used in same context >+# as sanitizeEverythingWarning except this is shown instead when there are no >+# visits in the user's history and the user has changed the history items to be cleared. >+sanitizeSelectedNoVisitsWarning=All selected items will be cleared. Similar to "NoVisits", "when there are no visits in the user's history" seems misleading to localizers.

I guess we should remove sanitizeEverythingWarning and update the other strings accordingly.

Addressed the review comments.

(15:12:02) Pike: dao: in bug 488691, are the two strings supposed to be the same? (15:12:39) dao: Pike: nope (15:13:04) Pike: dao: I found that the second localization note has the wrong key in the (), too

Comment on attachment 400482 [details] [diff] [review] Strings patch (v3) You didn't mean to remove sanitizeButtonOK, did you? Not sure "Prompt" should be part of the id, since we don't prompt anything. Maybe sanitizeEverythingWarning2 and sanitizeSelectedWarning would be better?

Sorry for the stupid mistakes in the previous patch! ;-) I also found another problem in the code: if Everything was being selected with the item list expanded, changing the selection wouldn't update the warning text, this version of the code patch fixes that too.

Comment on attachment 400521 [details] [diff] [review] Strings patch (v4) Please do not request approval until patches have been reviewed.

This landing caused unit test oranges, which I fixed in <http://hg.mozilla.org/mozilla-central/rev/9fddec8c335d>. This changeset should have no effect on real browser usage, because when the user is actually interacting with the dialog, the item list should already be visible before clicking any of the checkboxes, so showItemList would be a no-op anyway. In the unit test, however, the click method is called on the checkboxes without making sure in advance that the item list is visible.

Flagging in litmus to make sure the existing set of test cases are updated.

While trying to verify this bug, I noticed if I have all items checked except "Site Preferences," the UI still shows "All history will be cleared. This action cannot be undone." Is this expected?

If someone could answer Comment 52 I can proceed with updating the test case. Thanks. (In reply to comment #52) > While trying to verify this bug, I noticed if I have all items checked except > "Site Preferences," the UI still shows "All history will be cleared. This > action cannot be undone." Is this expected?

(In reply to comment #52) > While trying to verify this bug, I noticed if I have all items checked except > "Site Preferences," the UI still shows "All history will be cleared. This > action cannot be undone." Is this expected? Well, I guess not. The code checks to see if the selection has been _customized_, and because by default the Site Preferences checkbox is not checked, that case is not considered for changing the prompt, but I think it should be, because in that case not all of the items are actually cleared. Can you please file a new bug for this issue?

https://bugzilla.mozilla.org/show_bug.cgi?id=527820 is the bug for the issue mentioned in Comment 54.