527820 - Checking all prefs except "Site Preferences" shows confusing UI

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect, P2)

Description

[Marcia Knous [:marcia]]

Seen while running Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2b3pre) Gecko/20091110 Namoroka/3.6b3pre

STR:

1. Open Tools | Clear Recent History and have all items checked except "Site Preferences.
2. Note that the UI shows "All history will be cleared. This action cannot be undone." 

Bug 488691#c54 indicates that this is not correct

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Patch + unit test.

Comment 2

[(no longer active)]

Requesting blocking because bug 488691 was actually a 3.6 blocker, but did not implement the desired behavior 100% correctly.

Comment 3

[(no longer active)]

Oh, and I should add that the fix is very trivial (actually one line of code change + a method rename) and it comes with an automated test, so it's relatively safe to take.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 411846 [details] [diff] [review]
Patch (v1)

I think it's intentional that the list doesn't show when selecting "everything" with the default items selected. Apparently "Site Preferences" have a special role and aren't quite considered history, or something.

You might try to get this ui-reviewed, but r- until then.

Comment 5

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

Intended UI described here: https://bugzilla.mozilla.org/show_bug.cgi?id=488691#c16

>I think it's intentional that the list doesn't show when selecting "everything"
>with the default items selected.

That's right, but I actually can't remember if Site Preferences is one of the default selected items.

Ehsan: can you describe the functionality of the patch for a quick ui-review.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> >I think it's intentional that the list doesn't show when selecting "everything"
> >with the default items selected.
> 
> That's right, but I actually can't remember if Site Preferences is one of the
> default selected items.

It's not.

Comment 7

[(no longer active)]

Here is an overview of this bug and its fix for ui-r:

In bug 488691, we started showing a prompt in the CRH dialog when the user selected "Everything" in the timeline, and changed the default selection of which items to clear.  The prompt was: "All selected items will be cleared".

Please note the difference between what we tell the user in the UI and the logic that we use to display that prompt: we tell the user that all the selected items will be cleared if the selection *is different* with the default selection, not when there any unselected items.

Now, as it turns out, the default for Site Preferences is unchecked, so if the user checks all of the checkboxes, we show the "All history" prompt, not the "All selected items" prompt, which is the wrong behavior.

The problem was that actually the logic that we used to determine whether we need to show the "selected items" prompt was wrong: we should show that prompt when there are unselected items, without regard to the default values of those preferences.  This patch fixes that logic to come in sync with the prompt we show in the UI.

Comment 8

[Dão Gottwald [:dao]]

> Now, as it turns out, the default for Site Preferences is unchecked, so if the
> user checks all of the checkboxes, we show the "All history" prompt, not the
> "All selected items" prompt, which is the wrong behavior.

It's quite the opposite.

Comment 9

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

Comment on attachment 411846 [details] [diff] [review]
Patch (v1)

(In reply to comment #8)
> > Now, as it turns out, the default for Site Preferences is unchecked, so if the
> > user checks all of the checkboxes, we show the "All history" prompt, not the
> > "All selected items" prompt, which is the wrong behavior.
> 
> It's quite the opposite.

Ehsan was saying that his patch makes it so that when the user checks all of the checkboxes, the message is "All history will be cleared", fixing the problem that existed previously.

As Ehsan mentions, the previous dialog is misleading, as it gives the user the impression that their entire history will be cleared, when in fact it will not. I understand that "Site Preferences" aren't considered part of "History", but in that case they should probably be removed from this list or that other bug shouldn't have been fixed.

This fixes things to a point where we're consistent for Firefox 3.6. We can fix otherwise in a future release.

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #5)
> >I think it's intentional that the list doesn't show when selecting "everything"
> >with the default items selected.
> 
> That's right, 

So changing this is ok?

Comment 11

[(no longer active)]

(In reply to comment #10)
> (In reply to comment #5)
> > >I think it's intentional that the list doesn't show when selecting "everything"
> > >with the default items selected.
> > 
> > That's right, 
> 
> So changing this is ok?

Yes, I presented a patched build to him, and he said that this is the right behavior.

Comment 12

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

(It's true, I did!)

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 411846 [details] [diff] [review]
Patch (v1)

>   /**
>    * Resets the checkboxes to their default state.
>    */
>   resetCheckboxes: function () {
>     var cb = this.win.document.querySelectorAll("#itemList > [preference]");
>     ok(cb.length > 1, "found checkboxes for preferences");
>     for (var i = 0; i < cb.length; ++i) {
>       var pref = this.win.document.getElementById(cb[i].getAttribute("preference"));
>-      if (pref.value != pref.defaultValue)
>+      if (!pref.value)
>         cb[i].click();

This change looks wrong.

Comment 14

[(no longer active)]

Attachment: Patch (v2)

Maybe I should have renamed the function, because its purpose is to make sure that all of the items are checked.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 413361 [details] [diff] [review]
Patch (v2)

>     wh.onload = function () {
>       // Reset the check boxes and select "Everything"
>-      this.resetCheckboxes();
>+      this.checkAllCheckboxes();

You need to adjust that comment as well.

>+      // Modify the Site Preferences item state (bug 527820)
>+      this.checkAllCheckboxes();
>+      this.checkPrefCheckbox("siteSettings", false);
>+      this.acceptDialog();
>+    };
>+    wh.open();
>+  },
>+  function () {
>+    let wh = new WindowHelper();
>+    wh.onload = function () {
>+      // Details should be open because the items selection is not the same
>+      // as the default state.
>+      this.checkDetails(true);

That comment seems wrong... this.checkAllCheckboxes(); and this.checkPrefCheckbox("siteSettings", false); actually makes the selection match the default state.

Comment 16

[(no longer active)]

Attachment: Fix all comments

This patch fixes all the comments in the test to match the new logic.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 413368 [details] [diff] [review]
Fix all comments

>-      // Details should remain closed because the items selection is the same
>-      // as the default state.
>+      // Details should be open because not all items are checked.
>       this.checkDetails(false);

That comment is not in sync with the function call.

Comment 18

[(no longer active)]

Attachment: Fixed the comment

Fixed the comment.

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/647e08eed75b

Comment 20

[(no longer active)]

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

Comment 21

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.2 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2b6pre) Gecko/20100104 Namoroka/3.6b6pre. I verified using the STR In Comment 0.