Closed
Bug 491638
Opened 15 years ago
Closed 15 years ago
Clear recent history dialog should persist details expansion
Categories
(Firefox :: Private Browsing, defect)
Firefox
Private Browsing
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: adw, Assigned: adw)
References
Details
(Keywords: verified1.9.1)
Attachments
(1 file, 3 obsolete files)
4.66 KB,
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | 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)
Assignee | ||
Comment 1•15 years ago
|
||
Updates the existing CRH test.
Attachment #375939 -
Attachment is obsolete: true
Attachment #375947 -
Flags: review?(johnath)
Attachment #375939 -
Flags: review?(johnath)
Updated•15 years ago
|
Attachment #375947 -
Flags: review?(johnath)
Attachment #375947 -
Flags: review+
Attachment #375947 -
Flags: approval1.9.1?
Comment 2•15 years ago
|
||
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•15 years ago
|
||
Addressed comment 2.
Attachment #375947 -
Attachment is obsolete: true
Attachment #375947 -
Flags: approval1.9.1?
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 4•15 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?
Assignee | ||
Comment 6•15 years ago
|
||
Thanks, Dão.
Attachment #376024 -
Attachment is obsolete: true
Attachment #376024 -
Flags: approval1.9.1?
Updated•15 years ago
|
Keywords: checkin-needed
Comment 7•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/811c6bdcd6bd
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Assignee | ||
Updated•15 years ago
|
Attachment #377699 -
Flags: approval1.9.1?
Assignee | ||
Comment 8•15 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.
Comment 9•15 years ago
|
||
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 10•15 years ago
|
||
Comment on attachment 377699 [details] [diff] [review] for checkin (unbitrotted) a191=beltzner
Attachment #377699 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 11•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4bfb0df8ac17
Keywords: checkin-needed → fixed1.9.1
Comment 12•15 years ago
|
||
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.
Description
•