488691 - Confusing warning when clearing all history

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect, P1)

Description

[Simon Bünzli]

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.)

Comment 1

[(no longer active)]

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.

Comment 2

[Drew Willcoxon :adw]

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

Comment 3

[(no longer active)]

(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.

Comment 4

[Drew Willcoxon :adw]

(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". :)

Comment 5

[Simon Bünzli]

(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.)

Comment 6

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

>(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.

Comment 7

[Axel Hecht [:Pike]]

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.

Comment 8

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

>but "re-open" doesn't reflect the fact that we're string frozen.

yeah, I meant for firefox.next

Comment 9

[Simon Bünzli]

(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.

Comment 10

[Drew Willcoxon :adw]

(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? :)

Comment 11

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

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.

Comment 12

[Simon Bünzli]

(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...

Comment 13

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

(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.

Comment 15

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

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

Comment 16

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

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.

Comment 17

[(no longer active)]

Waiting for the final string here...

Comment 18

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

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.

Comment 19

[(no longer active)]

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

Comment 20

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

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 21

[(no longer active)]

Attachment: Patch (v1)

Comment 22

[Dão Gottwald [:dao]]

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?

Comment 23

[(no longer active)]

Attachment: Patch (v2)

(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 24

[Dão Gottwald [:dao]]

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?

Comment 25

[(no longer active)]

Attachment: Patch (v3)

Addressed both comments.

Comment 26

[Dão Gottwald [:dao]]

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.

Comment 27

[(no longer active)]

Attachment: For check-in

Comment 28

[Dão Gottwald [:dao]]

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."

Comment 29

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

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

Comment 30

[(no longer active)]

Attachment: Strings patch

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

Comment 31

[(no longer active)]

Attachment: Code Patch

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.

Comment 32

[Dão Gottwald [:dao]]

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

Comment 33

[(no longer active)]

Attachment: Code Patch (v2)

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

Done.

Comment 34

[Dão Gottwald [:dao]]

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 &&"...

Comment 35

[(no longer active)]

Attachment: Code Patch (v3)

(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?  :-)

Comment 36

[(no longer active)]

Attachment: Strings patch (v2)

Comment 37

[Dão Gottwald [:dao]]

(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 38

[Dão Gottwald [:dao]]

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 39

[Dão Gottwald [:dao]]

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.

Comment 40

[Dão Gottwald [:dao]]

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

Comment 41

[(no longer active)]

Attachment: Code Patch (v4)

Addressed the review comments.

Comment 42

[(no longer active)]

Attachment: Strings patch (v3)

Comment 43

[Dão Gottwald [:dao]]

(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 44

[Dão Gottwald [:dao]]

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?

Comment 45

[(no longer active)]

Attachment: Code Patch (v5)

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 46

[(no longer active)]

Attachment: Strings patch (v4)

Comment 47

[Benjamin Smedberg]

Comment on attachment 400521 [details] [diff] [review]
Strings patch (v4)

Please do not request approval until patches have been reviewed.

Comment 48

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/aaa39ba5d852

Comment 49

[(no longer active)]

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.

Comment 50

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c0011843ae3e

Comment 51

[Marcia Knous [:marcia]]

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

Comment 52

[Marcia Knous [:marcia]]

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?

Comment 53

[Marcia Knous [:marcia]]

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?

Comment 54

[(no longer active)]

(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?

Comment 55

[Marcia Knous [:marcia]]

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