Currently, when Gecko asks us to draw a scrollbar, we'll tell HITheme to draw the scrollbar with all of its controls, no matter how small of a box we're given. Instead, we should do what Safari does and draw just the scrollbar track when things get too small. In my tests, anything below 61 pixels for large scrollbars and 49 pixels for small scrollbars and it starts to look bad. This is pretty easy to accomplish in widget -- we just tell HITheme to draw with no features when our height is below those numbers. I turned on my visual code debug (set SCROLLBARS_VISUAL_DEBUG to 1 in nsNativeTheme.mm) and it looks Gecko is doing some weird things below 60px as well. It's tough to describe, I'm guessing it's a Windows/Linux behavior? I would expect we would want to modify that behavior too to match what we draw. A testcase is attached that shows the minimum size I think looks good, which is also the minimum size Tiger Safari draws reasonably. The testcase also includes an example of the bug for both small and large scrollbars.

See also bug 292284, in particular attachment 182897 [details], but I also like attachment 182981 [details] ;-)

With this patch, widget now draws the scrollbar in the state it should be. All the controls still work, since I haven't touched layout.

Here's a screenshot showing the new behavior. Patched version is on the right, bug is shown on the left.

Hadn't cleared my tree last time, sorry about that.

This brings us to full parity with Safari's behavior. This adds an intermediate state where we still display the arrows, but not the thumb. This gives us an extra 7 pixels for large scrollbars and 3 for small ones. I think I can (ab)use GetMinimumWidgetSize to return 0,0 when I don't want the buttons or thumb to display, so I won't have to make changes in layout. Although I'm not entirely sure a hack like that is the way to go, it is easy, I think.

Some cleanup. I tried to abuse GetMinimumWidgetSize as per the above comment but it didn't work. Suggestions are more than welcome, at this point.

It seems that the GTK2 and Windows behaviors differ here. Loading the testcase produces different results on Windows and GTK. Windows shrinks the buttons and thumb up to a point -- GTK just hides the thumb and eventually just hides the buttons and track as well. I have yet to figure out where that behavior is implemented.

In Windows are we really squishing the buttons, or just visually? If you click just outside a shrunk button are you still really clicking on the button?

I don't actually know 100% but attachment 182902 [details] (part of bug 292284 that Neil mentioned) shows at least what it looks like on Windows, and it seems like a similar thing happens if you turn on SCROLLBAR_VISUAL_DEBUG in nsNativeThemeCocoa.mm and load the testcase on this bug. Basically, I'd say "probably." I should really get some VMs set up.

I don't see anything in layout that's shrinking the sizes of button frames. We could shrink button sizes for real if we need to, probably, but if Windows has been faking it for years that suggests you could get away with just doing it visually for now...

Here's what it looks like with the scrollbar visual debug enabled. Notice the tiny thumb in the case where the thumb should be disabled, and the weirdness of only showing the up arrow. I don't know what a similar rendering would look like on Windows. Additionally, some of the controls on the smallest scrollbar seem like they work, even though they aren't showing up properly?

Okay, the issue here is that the scrollbar is made up of multiple parts: the slider and the two buttons. It would be simple just to make the minimum size of the buttons zero; however, that would lead to the issue that the buttons wouldn't disappear when the scrollbar got too small; they would just shrink. As far as I know, though, there's no easy way in XUL to make something disappear when there isn't enough space for it without using scripting; maybe somebody else has a better idea?

Comment on attachment 264843 [details] [diff] [review] widget changes v1.2 This is pretty bad /looking/ problem, and this does actually fix it, at least visually. Since fixing it "for real" seems like it will be a lot of work, and there are other important things to do, I think we should just land this.

Comment on attachment 264843 [details] [diff] [review] widget changes v1.2 + // Only display the thumb if we have room for it to display + if ((isHorizontal ? aRect.size.width : aRect.size.height) >= + (isSmall ? MIN_SMALL_SCROLLBAR_SIZE_WITH_THUMB : MIN_SCROLLBAR_SIZE_WITH_THUMB)) { + aTdi.attributes |= kThemeTrackShowThumb; + } + + // If we don't have enough room to display *any* features, we're done creating this tdi. + if ((isHorizontal ? aRect.size.width : aRect.size.height) < + (isSmall ? MIN_SMALL_SCROLLBAR_SIZE : MIN_SCROLLBAR_SIZE)) { + aTdi.enableState = kThemeTrackNothingToScroll; + return; + } The second test can't be true if the first one was so please do an else-if. Also, please document how event handling works when we're not drawing particular parts of the scrollbar. Please document this next to the two tests above so when people see things not getting drawn they can see that event handling for them still happens.

Addresses review comments. Also wraps comments in the code at roughly 80 characters, for readability

Comment on attachment 270680 [details] [diff] [review] fix v1.3 +// These magic numbers are the minimum sizes we can draw a scrollbar and still +// have room for everything to display, including the thumb +#define MIN_SCROLLBAR_SIZE_WITH_THUMB 61 +#define MIN_SMALL_SCROLLBAR_SIZE_WITH_THUMB 49 +// And these are the minimum sizes if we don't draw the thumb You should explain how these numbers were derived, so that later we can update them if necessary. + if ((isHorizontal ? aRect.size.width : aRect.size.height) Why don't you use aTdi.trackInfo.scrollbar.viewsize here, or define a local variable that all users of this expression can use.

I landed v1.3 on trunk without seeing roc's comment #16. Colin - please post another patch on this bug addressing those. Note that since v1.3 already landed it should be against the code with that already applied.

Here's a patch addressing roc's sr comments. Should I just go ahead and land this, since I have roc's sr+?

addressing further comments from josh via irc

Comment on attachment 270769 [details] [diff] [review] patch addressing sr comments 1.2 with on-checkin nits discussed on IRC

Checking in widget/src/cocoa/nsNativeThemeCocoa.mm; /cvsroot/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm,v <-- nsNativeThemeCocoa.mm new revision: 1.48; previous revision: 1.47 done

test case doesn't include overflowed divs inside of fieldsets