Closed Bug 491638 Opened 15 years ago Closed 15 years ago

Clear recent history dialog should persist details expansion


(Firefox :: Private Browsing, defect)

Not set



Firefox 3.6a1


(Reporter: adw, Assigned: adw)



(Keywords: verified1.9.1)


(1 file, 3 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
There have been some complaints about hiding the item list behind a progressive disclosure button.  One compromise would be to remember whether the user had expanded the list when the dialog was previously closed.

In other related bugs and lists people have argued the needs of simple users vs. advanced users.  Simple users -- I think by definition -- won't expand the details list, so no big deal there.  Advanced users -- again by definition -- will, and we ought to respect their curiosity.  Some advanced users will want to toggle different items in the list fairly often, and others won't.  For the former, my proposal's pretty cool.  For the latter, we respect their curiosity initially and if they realize they'd prefer not to see details, they can hide them next time they use the dialog, no harm done.

Technically it's pretty easy: persist the appropriate attributes of the list and progressive disclosure button.
Attachment #375939 - Flags: review?(johnath)
Attached patch patch v2 (with test) (obsolete) — Splinter Review
Updates the existing CRH test.
Attachment #375939 - Attachment is obsolete: true
Attachment #375947 - Flags: review?(johnath)
Attachment #375939 - Flags: review?(johnath)
Attachment #375947 - Flags: review?(johnath)
Attachment #375947 - Flags: review+
Attachment #375947 - Flags: approval1.9.1?
Comment on attachment 375947 [details] [diff] [review]
patch v2 (with test)

>   /**
>    * Ensures that the details progressive disclosure button and the item list
>    * hidden by it match up.  Also makes sure the height of the dialog is
>    * sufficient for the item list and warning panel.
>+   *
>+   * @param aShouldBeShown
>+   *        True if you expect the details to be shown and false if hidden
>    */
>-  checkDetails: function () {
>+  checkDetails: function (aShouldBeShown) {
>     let button = this.getDetailsButton();
>     let list = this.getItemList();
>     let hidden = list.hidden || list.collapsed;
>+    is(hidden, !aShouldBeShown, "Details should be shown or hidden as expected");

Nit: I know you were trying for a generically acceptable string here, but if this test fails, it's going to take a bit more sleuthing than is ideal to figure out which direction it actually failed. Can we be more specific, e.g.

is(hidden, !aShouldBeShown, "Details section should be " + (aShouldBeShown ? "shown" : "hidden") + " but was actually " + (hidden ? "hidden" : "shown"));

Otherwise, nice clean fix with tests, yay. I think we should take it on branch once it's baked on trunk.
Attached patch for checkin (obsolete) — Splinter Review
Addressed comment 2.
Attachment #375947 - Attachment is obsolete: true
Attachment #375947 - Flags: approval1.9.1?
Keywords: checkin-needed
Comment on attachment 376024 [details] [diff] [review]
for checkin

Requesting approval, see last para of comment 2; I obsoleted patch v2 with the for checkin patch, which removed johnath's approval request on patch v2... Low risk, comes with test, a good compromise between simple and advanced users.
Attachment #376024 - Flags: approval1.9.1?
this needs an updated patch
Keywords: checkin-needed
Thanks, Dão.
Attachment #376024 - Attachment is obsolete: true
Attachment #376024 - Flags: approval1.9.1?
Keywords: checkin-needed
Closed: 15 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Attachment #377699 - Flags: approval1.9.1?
Comment on attachment 377699 [details] [diff] [review]
for checkin (unbitrotted)

Requesting approval, low risk, comes with test, makes the CRH more pleasant for advanced users, Johnathan recommends approval in comment 2.
Verified fixed on trunk and all platforms: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090517 Minefield/3.6a1pre ID:20090517031115
Comment on attachment 377699 [details] [diff] [review]
for checkin (unbitrotted)

Attachment #377699 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
Verified fixed on 1.9.1 with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090520 Shiretoko/3.5b5pre ID:20090520035357
You need to log in before you can comment on or make changes to this bug.