489958 - New Clear recent history dialog shows unnecessary scrollbars with details expanded

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: scrollbars in expanded mode

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090423 Shiretoko/3.5b4pre ID:20090423040926

Since the landing of bug 480169 users cannot directly see the data which will be cleared. Those settings are hidden behind the details button. If you click on it to show the list you will notice that the area cannot fit all those entries and scrollbars are shown. Due to that the dialog is not re-sizable the scrollbars will always be shown. IMO we have enough space to make the window a bit higher which will result in a nice looking list without any scrollbar.

Comment 1

[Dave Garrett]

This was done intentionally.
http://people.mozilla.com/~faaborg/files/shiretoko/clearRecentHistoryi6.png

Personally, I don't think we need two levels of user interaction to show these, especially because some of the entries were merged and there are only 2 out-of-view now.

Comment 2

[(no longer active)]

Yes, this has been done intentionally.  -> WONTFIX.

Comment 3

[Dão Gottwald [:dao]]

Hiding less important items doesn't work like this. The user doesn't know that they are less important, so in order to prevent dataloss, he's tempted to scroll. That's hardly a win.

This should be reconsidered, if only because of comment 1 (6 items now vs. 8 items in the mockup).

Comment 4

[(no longer active)]

(In reply to comment #3)
> Hiding less important items doesn't work like this. The user doesn't know that
> they are less important, so in order to prevent dataloss, he's tempted to
> scroll. That's hardly a win.
> 
> This should be reconsidered, if only because of comment 1 (6 items now vs. 8
> items in the mockup).

Alex, what do you think?

Comment 5

[Johnathan Nightingale [:johnath]]

Interested in Alex's thoughts, but I'm inclined to agree with showing the whole listbox when clicked.  The user just asked us for details, in an already semi-wonkish part of the browser.  I think further progressive-disclosure doesn't really add much, and does create a sort of unpolished look.

The huge simplicity win with this dialog is that when you first open it, it is a single question with a sensible default - people I've talked to seem to really respond to that simplicity.  Once someone asks for complexity though, I don't think we should try to manipulate them into only seeing some of it.  If there were 50 elements, we would scrollbox it out of necessity, but when we're only hiding 2 off-pane, it seems like an oversight more than a design choice.

Comment 6

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

Yeah, and the fact that the dialog is now considerably simpler (since we cut the time range preview), means that we don't have to worry as much about taking up too much vertical space.

I'm expecting we'll get some negative feedback on the scrolling in beta 4, and I'm fine with removing it.

Something else that I was considering in the initial design was that the set of implementation-level aspects of history that can be removed may end up growing over time as we add more stuff, but we can deal with that case later.

Comment 7

[Luke Iliffe (Harlequin99)]

(In reply to comment #6)
> Something else that I was considering in the initial design was that the set of
> implementation-level aspects of history that can be removed may end up growing
> over time as we add more stuff, but we can deal with that case later.

This already occurs with extensions, e.g. Tab Mix Plus adds an entry to the CRH dialog to clear saved TMP sessions. Currently this is not added to the listbox but sits (rather ugly) in the simplified dialog you initially see. If/when this entry gets added to the proper listbox I assume it will trigger scrolling.

Comment 8

[Drew Willcoxon :adw]

Alex, should we go back to displaying the checkboxes in a vbox then?  It looks kind of strange to see them in a white listbox with no scrollbars.

(In reply to comment #7)
> (In reply to comment #6)
> dialog to clear saved TMP sessions. Currently this is not added to the listbox
> but sits (rather ugly) in the simplified dialog you initially see. If/when this

Hmm, yeah, extensions that add items to the dialog will have to be updated to take the details button into account...

Comment 10

[David Naylor]

I thought I'd just add my comment, as a user who hasn't been doing any thinking
around the design before using this box.

When I came across the window with the listbox I was slightly annoyed at having
to scroll to see the remaining options.

Reading the comments, I really agree with Jonathan: "Once someone asks for complexity though, I don't think we should try to manipulate them into only seeing some of it."

Even if the options grow back to 8 I still think it's better to just show them
all. Otherwise you're forcing the user to look at the first four, scroll down
to look at the last four, and then scroll back up again to get on with what he
or she really wanted to do ... (I'd bet a Euro that's how people work.)

If the options grow to more than 8 they need to be grouped or combined somehow ...

Comment 11

[Egon Knapen]

I personally don't like the new clear recent history dialog. I thought the fx3.1 was a big improvement over the fx3.0 version. I could directly see what happens (I think removing passwords from this dialog is a good improvement).

If it stays like this, it would be nice if it could remember the last setting, like I wish to always see the details... Maybe a resize option, or just show all the options in one time. And the next time I open this dialog, I would like to see it they way I left it. So the button details, will only be a one time annoyance...

I thought the fx3.1 version was very nice, much better than the way IE7 or so handles it...

Comment 12

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

>Alex, should we go back to displaying the checkboxes in a vbox then?  It looks
>kind of strange to see them in a white listbox with no scrollbars.

I kind of like how the list box groups everything together (this is a collection of sub-components as opposed to a list of disparate things), and also this makes it easier for extensions to add stuff and we'll pick up the scroll bar

Comment 13

[Drew Willcoxon :adw]

This should be a trivial fix, but setting rows="6" on the listbox causes a useless scrollbar to appear in the listbox on Pinstripe.  (There are only 6 rows.)  When I click the scrollbar it disappears.  Works fine on Winstripe and Gnomestripe.  If anyone has any insight, would appreciate it.  Found these old bugs: bug 274036, bug 424821.

Comment 14

[Drew Willcoxon :adw]

Attachment: patch v1

If I add 1px of padding to the top and bottom of the listitems (Pinstripe only), the useless listbox scrollbar goes away.  I have no idea.

Comment 15

[Drew Willcoxon :adw]

Oh, filed Bug 491788 - XUL listbox sometimes has unnecessary scrollbar.

Comment 16

[Luke Iliffe (Harlequin99)]

Is there any chance this can be ready for 3.5 RC - AFAICT it doesn't have a wanted 1.9.1 flag set.

This would nicely complete the polish on the new CRH dialog along with bug 491638 and bug 490655 which have already landed on 1.9.1

Comment 17

[Drew Willcoxon :adw]

Johnathan, any chance of review here?  I agree with comment 16.  Let me know if I should poke someone else or if you'd like a screen grab.  The fix to get rid of the useless scrollbar on Pinstripe is a little hacky, but it works and isn't noticable and IMHO is outweighed by the benefit of showing the entire list and the fact that the vast majority of users aren't on Pinstripe to begin with.

Comment 18

[Johnathan Nightingale [:johnath]]

Comment on attachment 377274 [details] [diff] [review]
patch v1

>diff --git a/browser/base/content/sanitize.xul b/browser/base/content/sanitize.xul
>-    <listbox id="itemList" rows="4" collapsed="true">
>+    <listbox id="itemList" rows="6" collapsed="true">

Yep

>diff --git a/browser/themes/pinstripe/browser/sanitizeDialog.css b/browser/themes/pinstripe/browser/sanitizeDialog.css

>+/* Without this a useless scrollbar appears in the listbox when its rows
>+   attribute is set to the total number of listitems, as it is currently.  See
>+   bug 489958 comment 14 and bug 491788. */
>+#itemList > listitem {
>+  padding: 1px 0;
>+}

I don't like a styling change we don't understand, but you filed a follow-up to track that down, so I'm okay with moving ahead with a workaround here.  However, you say in the follow-up that this occurs on all platforms, but in this bug you say it only occurs in pinstripe. Do I understand correctly that this is because the broken behaviour is row-count and OS-specific, and that we don't need this change in winstripe/gnomestripe?

r=me for a patch that has this change in all, and only, the CSS files it should be in.

Comment 19

[Drew Willcoxon :adw]

Attachment: for checkin (unbitrotted patch v1)

(In reply to comment #18)
> However, you say in the follow-up that this occurs on all platforms, but in
> this bug you say it only occurs in pinstripe. Do I understand correctly that
> this is because the broken behaviour is row-count and OS-specific, and that we
> don't need this change in winstripe/gnomestripe?

I'm able to reproduce it in isolation on all platforms, but in the CRH dialog the problem is manifested only on OS X.  I don't understand why.  I've therefore included the CSS hack only on Prinstripe.

Comment 20

[Dão Gottwald [:dao]]

Trunk is restricted, landed with a=mconnor:

http://hg.mozilla.org/mozilla-central/rev/d6a2b89e5a0c

Comment 21

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

Comment on attachment 378636 [details] [diff] [review]
for checkin (unbitrotted patch v1)

a191=beltzner

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Drew, on Linux there is still shown a scrollbar in this dialog. The slider takes the whole height and when clicking on it the scrollbar is removed. Is this really fixed?

Comment 23

[Drew Willcoxon :adw]

(In reply to comment #22)
> Drew, on Linux there is still shown a scrollbar in this dialog. The slider
> takes the whole height and when clicking on it the scrollbar is removed. Is
> this really fixed?

In my testing the uselss scrollbar appeared only on OS X, so this is a surprise to me.  The hack was only applied to OS X.  I guess it could be applied to Linux as well, but the fact that I don't see it but you do is worrisome and makes me think maybe we should not have taken this after all.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Current behavior on Linux

Screenshot just for reference. So how we should further proceed here?

Comment 25

[Drew Willcoxon :adw]

Clicking a scrollbar and having it disappear is just amateurish IMO, so I'd personally rather force the user to scroll.  In light of the fact that it's happening to you on Linux but not to me, I think we should backout this patch on trunk until bug 491788 is fixed.  Johnathan?  Or maybe someone who knows better than me can take a look at this bug.

Comment 26

[Dão Gottwald [:dao]]

I disagree. Showing all items should not depend on bug 491788. Some items being hidden is annoying interaction-wise, whereas the zombie scrollbar is more of a visual flaw.

Comment 27

[Drew Willcoxon :adw]

There's the option of displaying the items in a vbox, groupbox, or tree.  I tested a tree in isolation and it doesn't exhibit the problem, but adding checkboxes to it is a little more work.

Comment 28

[Steffen Wilberg]

Couldn't we add the pinstripe hack to gnomestripe's sanitizeDialog.css as well?

Comment 29

[Dão Gottwald [:dao]]

Yeah.
The listbox has been opted for because you wanted the overflow behavior if extensions add items, right?

Comment 30

[Dão Gottwald [:dao]]

Pushed the theme hack for gnomestripe without another review, since johnath's review comment covered this already (it made it a requirement, even):

http://hg.mozilla.org/mozilla-central/rev/04e62eaa8a13

I think this is good to go on 1.9.1 now. Drew, if you agree, please tag checkin-needed. If you want to implement the UI without a listbox, I think that's both something for a separate bug and something that's unlikely to be approved for 1.9.1 at this point.

Comment 31

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/887924b684c7

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, now it looks good on Linux too. No scrollbar anymore with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090522 Minefield/3.6a1pre ID:20090522031102

I'll check with the next available tinderbox build or tomorrows nightly build on 1.9.1 if it's fine there.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks good with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090524 Shiretoko/3.5pre ID:20090524031212

Comment 34

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

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.