Clear recent history dialog should persist details expansion

VERIFIED FIXED in Firefox 3.6a1

Status

()

Firefox
Private Browsing
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

({verified1.9.1})

Trunk
Firefox 3.6a1
verified1.9.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 375939 [details] [diff] [review]
patch v1

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

Comment 1

9 years ago
Created attachment 375947 [details] [diff] [review]
patch v2 (with test)

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

Comment 3

9 years ago
Created attachment 376024 [details] [diff] [review]
for checkin

Addressed comment 2.
Attachment #375947 - Attachment is obsolete: true
Attachment #375947 - Flags: approval1.9.1?
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Comment 4

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

Comment 6

9 years ago
Created attachment 377699 [details] [diff] [review]
for checkin (unbitrotted)

Thanks, Dão.
Attachment #376024 - Attachment is obsolete: true
Attachment #376024 - Flags: approval1.9.1?

Updated

9 years ago
Keywords: checkin-needed

Comment 7

9 years ago
http://hg.mozilla.org/mozilla-central/rev/811c6bdcd6bd
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
(Assignee)

Updated

9 years ago
Attachment #377699 - Flags: approval1.9.1?
(Assignee)

Comment 8

9 years ago
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
Status: RESOLVED → VERIFIED
Comment on attachment 377699 [details] [diff] [review]
for checkin (unbitrotted)

a191=beltzner
Attachment #377699 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Updated

9 years ago
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
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.