Confusing warning when clearing all history

RESOLVED FIXED in Firefox 3.7a1

Status

()

P1
normal
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: zeniko, Assigned: Ehsan)

Tracking

({polish})

3.5 Branch
Firefox 3.7a1
polish
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.6 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, status1.9.1 wontfix)

Details

Attachments

(2 attachments, 11 obsolete attachments)

(Reporter)

Description

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

Comment 1

10 years ago
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

10 years ago
Alex discusses what makes history history at bug 464204 and bug 480169 comment 10 and probably elsewhere, too.
(Assignee)

Comment 3

10 years ago
(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

10 years ago
(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". :)
(Reporter)

Comment 5

10 years ago
(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.

Comment 7

10 years ago
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
(Reporter)

Comment 9

10 years ago
(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

10 years ago
(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.
(Reporter)

Comment 12

10 years ago
(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.
Flags: blocking-firefox3.6?
Keywords: uiwanted

Updated

9 years ago
Duplicate of this bug: 494541
Alex, we should fix this in 3.6 and soon before string freeze; what did you think of my suggestion in comment 13?
Flags: blocking-firefox3.6? → blocking-firefox3.6+
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.
(Assignee)

Comment 17

9 years ago
Waiting for the final string here...
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 19

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

Comment 21

9 years ago
Created attachment 395801 [details] [diff] [review]
Patch (v1)
Attachment #395801 - Flags: review?(dao)
(Assignee)

Updated

9 years ago
Keywords: uiwanted
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?
(Assignee)

Comment 23

9 years ago
Created attachment 395839 [details] [diff] [review]
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.
Attachment #395801 - Attachment is obsolete: true
Attachment #395839 - Flags: review?(dao)
Attachment #395801 - Flags: review?(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?
Attachment #395839 - Flags: review?(dao) → review-
(Assignee)

Comment 25

9 years ago
Created attachment 396406 [details] [diff] [review]
Patch (v3)

Addressed both comments.
Attachment #395839 - Attachment is obsolete: true
Attachment #396406 - Flags: review?(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.
Attachment #396406 - Flags: review?(dao) → review+
(Assignee)

Comment 27

9 years ago
Created attachment 396519 [details] [diff] [review]
For check-in
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
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."
Keywords: checkin-needed
This needs to land before September 14th if it's to make string freeze.
Priority: -- → P1
Whiteboard: [strings]
(Assignee)

Comment 30

9 years ago
Created attachment 400354 [details] [diff] [review]
Strings patch

Strings-only patch for landing before string freeze.  The code patch will follow.
Attachment #396519 - Attachment is obsolete: true
Attachment #400354 - Flags: ui-review?(beltzner)
Attachment #400354 - Flags: approval1.9.2?
(Assignee)

Comment 31

9 years ago
Created attachment 400357 [details] [diff] [review]
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.
Attachment #396406 - Attachment is obsolete: true
Attachment #400357 - Flags: review?(dao)
Why don't you walk the itemList child nodes to get the relevant prefs?
(Assignee)

Comment 33

9 years ago
Created attachment 400358 [details] [diff] [review]
Code Patch (v2)

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

Done.
Attachment #400357 - Attachment is obsolete: true
Attachment #400358 - Flags: review?(dao)
Attachment #400357 - Flags: review?(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 &&"...
Attachment #400358 - Flags: review?(dao) → review-

Updated

9 years ago
Attachment #400354 - Flags: review-
(Assignee)

Comment 35

9 years ago
Created attachment 400388 [details] [diff] [review]
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?  :-)
Attachment #400358 - Attachment is obsolete: true
Attachment #400388 - Flags: review?(dao)
(Assignee)

Comment 36

9 years ago
Created attachment 400389 [details] [diff] [review]
Strings patch (v2)
Attachment #400354 - Attachment is obsolete: true
Attachment #400389 - Flags: ui-review?(beltzner)
Attachment #400389 - Flags: approval1.9.2?
Attachment #400354 - Flags: ui-review?(beltzner)
Attachment #400354 - Flags: approval1.9.2?
(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.

Updated

9 years ago
Attachment #400388 - Flags: review?(dao) → review+
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.
(Assignee)

Comment 41

9 years ago
Created attachment 400481 [details] [diff] [review]
Code Patch (v4)

Addressed the review comments.
Attachment #400388 - Attachment is obsolete: true
(Assignee)

Comment 42

9 years ago
Created attachment 400482 [details] [diff] [review]
Strings patch (v3)
Attachment #400389 - Attachment is obsolete: true
Attachment #400482 - Flags: review?(dao)
Attachment #400482 - Flags: approval1.9.2?
Attachment #400389 - Flags: ui-review?(beltzner)
Attachment #400389 - Flags: approval1.9.2?
(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?
Attachment #400482 - Flags: review?(dao) → review-
(Assignee)

Comment 45

9 years ago
Created attachment 400520 [details] [diff] [review]
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.
Attachment #400481 - Attachment is obsolete: true
Attachment #400482 - Attachment is obsolete: true
Attachment #400520 - Flags: review?(dao)
Attachment #400482 - Flags: approval1.9.2?
(Assignee)

Comment 46

9 years ago
Created attachment 400521 [details] [diff] [review]
Strings patch (v4)
Attachment #400521 - Flags: review?(dao)
Attachment #400521 - Flags: approval1.9.2?

Comment 47

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

Please do not request approval until patches have been reviewed.
Attachment #400521 - Flags: approval1.9.2?

Updated

9 years ago
Attachment #400521 - Flags: review?(dao) → review+

Updated

9 years ago
Attachment #400520 - Flags: review?(dao) → review+
(Assignee)

Comment 48

9 years ago
http://hg.mozilla.org/mozilla-central/rev/aaa39ba5d852
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
status1.9.1: --- → wontfix
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
(Assignee)

Comment 49

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

Comment 50

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c0011843ae3e
status1.9.2: --- → beta1-fixed
Whiteboard: [strings] → [strings landed]
(Assignee)

Updated

9 years ago
Depends on: 516828
Flagging in litmus to make sure the existing set of test cases are updated.
Flags: in-litmus?
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?
(Assignee)

Comment 54

9 years ago
(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?
Whiteboard: [strings landed]
(Assignee)

Updated

9 years ago
Depends on: 527820
(Assignee)

Updated

5 years ago
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.