502292 - Minimum size of scrollbar thumb is far too small

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Glen Mailer]

Attachment: incorrectly sized thumb

The draggable "thumb" part of the scrollbar appears to have lost its setting for minimum height.

In firefox 3.0 it was always at least the size of the 3-blob gripper thing, but in 3.5 it can now be much smaller.

Comment 1

[Markus Stange [:mstange]]

On which scrollbar are you seeing this, in which part of which window?

Comment 2

[Sylvain Pasche]

I'm seeing this too on the right vertical scrollbar of a very tall page (tested on win7). For instance, open http://www.whatwg.org/specs/web-apps/current-work/

Comment 4

[Sylvain Pasche]

Build from 2008-08-12-04: 15px minimum size
Build from 2008-08-13-03: 8px minimum size

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-08-12+03%3A00&enddate=2008-08-13+04%3A00

Comment 5

[Markus Stange [:mstange]]

I'm going to blame bug 448704 :-)

Comment 6

[Sylvain Pasche]

Attachment: patch, v1

Before the gripper was removed, its size was taken into account when getting the thumb minimum size. 
The thumb size is hardcoded to be half the size of the scrollbar width/height (hardcoded in the sense that this number is not retrieved from the theme). I propose to simply make this value the same size as the thumb width/height which makes it the same minimum size as before.

Comment 7

[[e-mail]]

(In reply to comment #5)
> I'm going to blame bug 448704 :-)
Quoting:

3) This will make the Windows behavior more native, where the thumb can shrink
below a certain size and the grip will disappear--doing that before this would
not have been very feasible.

Comment 8

[Sylvain Pasche]

Attachment: Scrollbar thumb size comparison

Thanks for pointing this out. So this is an intentional regression.

I checked how the thumb behaves in a few applications. This screenshot shows the scrollbar in Firefox trunk, Chrome, IE, Notepad and a simple win32 application (that uses WS_VSCROLL to show the scrollbar).

On top is Vista and below XP. On XP the thumb size matches some applications such as Notepad, so some people may consider Firefox behavior more native.
However on Vista, the thumb size is even smaller than XP and its size doesn't match any other app (IE is the closest but it's still a few pixels larger).

Otherwise, I think I found an issue in the way the thumb minimum size is calculated in nsNativeThemeWin.cpp. The vertical scrollbar thumb minimum is evaluated to:
width = GetSystemMetrics(SM_CYVTHUMB)
height = width / 2

The documentation of SM_CYVTHUMB says:
In http://msdn.microsoft.com/en-us/library/ms724385%28VS.85%29.aspx:
The height of the thumb box in a vertical scroll bar, in pixels.
In http://msdn.microsoft.com/en-us/library/bb787527%28VS.85%29.aspx:
Height of scroll box on vertical scroll bar. This value retrieves the height of a scroll bar that has a page size of zero.

So I think this metric should be used for the height instead, and for the width we should use SM_CXVSCROLL I guess (assuming the thumb has the same width as the scrollbar), so we would have:

width = GetSystemMetrics(SM_CXVSCROLL)
height = GetSystemMetrics(SM_CYVTHUMB)

The same applies for the horizontal scrollbar.

This will result in the same thumb size as it was before bug 448704.
I think this size is better choice for Vista at least. Should we keep the current behavior for XP if that's considered to be more native? Personally I think it would also benefit from having the same size as on Vista.

Comment 9

[Sylvain Pasche]

Attachment: patch, v2

This implement what is described in comment 8.
I also made the wince minimum thumb size square on WinCE. I'm not too sure if that's wanted, but I thought that having a larger minimum thumb size on a mobile device where it's more difficult to target small areas could be nice.

The reftest is rather fragile as it depends on pixel specific metrics, but it could be useful to port it to Mac/Linux so that it doesn't regress there too.

Comment 10

[Sylvain Pasche]

Attachment: patch, v2.1

oops, bad copy paste.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 395257 [details] [diff] [review]
patch, v2.1

Great to see this get fixed. I can r+ the windows stuff but you'll need a ui review I think for these changes as well.

Comment 12

[Sylvain Pasche]

Attachment: Before/After comparison on Vista and XP

Thanks for the review.

This is the before/after comparison for the ui-review.
On left is the actual behavior and on the right with the patch applied.

I don't have a WinCE device/build to test the change there. Although the rendering could be different I guess the size should somewhat match XP/Vista behavior.

Comment 13

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

Comment on attachment 395352 [details]
Before/After comparison on Vista and XP

I think the advantage of having a minimum size that's actually grippable outweighs the subtle signals that the size of the grippy give about pagelength here.

uir=beltzner

Comment 14

[Sylvain Pasche]

Comment on attachment 395257 [details] [diff] [review]
patch, v2.1

Neil, do you want to review?

Comment 15

[[e-mail]]

(In reply to comment #8)
> I think this size is better choice for Vista at least. Should we keep the
> current behavior for XP if that's considered to be more native? Personally I
> think it would also benefit from having the same size as on Vista.
I think it should depend on whether the app is themed or not; if it is not themed then the minimum height/width should be halved as before (using the corrected metrics; bug 172751 originally introduced the height/width mixup!)

Comment 16

[Sylvain Pasche]

Attachment: patch, v3 [Checkin: See comment 17+18+20]

like so?

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 395504 [details] [diff] [review]
patch, v3
[Checkin: See comment 17+18+20]


http://hg.mozilla.org/mozilla-central/rev/0be096144e35

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 395504 [details] [diff] [review]
patch, v3
[Checkin: See comment 17+18+20]


http://hg.mozilla.org/mozilla-central/rev/af008a82454d
(Bv1) Switch "failing" test to '!='.

Based on what I understood and reproduced, I assume this was a copy&paste error.
Nontheless, I wonder why it was not detected before checkin!
Sylvain, can you explain/check/fix/confirm?

Comment 19

[Sylvain Pasche]

The issue is apparently that the Windows unit test box doesn't run with a theme (Windows 2003), but the failing tests (scroll-thumb-minimum-size-theme.html) relies on having a theming enabled.

What we would need is a way to skip the test if theming is not enabled. I don't think this is possible for the moment.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 395504 [details] [diff] [review]
patch, v3
[Checkin: See comment 17+18+20]


Per irc:
http://hg.mozilla.org/mozilla-central/rev/6d2ee131afc5
(Cv1) Disable failing test for now

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> The issue is apparently that the Windows unit test box doesn't run with a theme
> (Windows 2003), but the failing tests (scroll-thumb-minimum-size-theme.html)
> relies on having a theming enabled.

Tests like this one should really go to Try Server first:
making "unhandled assumptions" on the platform (configuration) is not good.

> What we would need is a way to skip the test if theming is not enabled. I don't
> think this is possible for the moment.

Either find a way to fix the test(s) and reenable it,
or remove the test(s). (assuming the fix is good.)

Comment 22

[Sylvain Pasche]

(In reply to comment #21)
> Tests like this one should really go to Try Server first:
> making "unhandled assumptions" on the platform (configuration) is not good.

Sorry for that, I was sure the test boxes were running with a theme and that other tests already depended on that.

> Either find a way to fix the test(s) and reenable it,
> or remove the test(s). (assuming the fix is good.)

I've filed bug 512206 for enabling that test only if theming is enabled.

Comment 23

[Sylvain Pasche]

Attachment: Patch for 1.9.2

This patch for 1.9.2 is the same as the latest trunk patch, with the test that can't run on the unit test server being commented.

This is a low risk change that should improve the usability of the scroll bar on Windows for users who have a non-Classic theme (the default on Windows >= XP).

Comment 24

[[not reading bugmail]]

(In reply to comment #7)
> (In reply to comment #5)
> > I'm going to blame bug 448704 :-)
> Quoting:
> 
> 3) This will make the Windows behavior more native, where the thumb can shrink
> below a certain size and the grip will disappear--doing that before this would
> not have been very feasible.

I noticed the after shot of Vista shows no grippy lines, I'm running the default Vista theme and I can reproduce no grippy where the thumb looks like its at the minimum size.  I cannot tell if its below the size listed in this bug.  But, is that the intended behavior?

For example, I can consistently open bug 435296 on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20090908 Minefield/3.7a1pre (.NET CLR 3.5.30729) and have no grippy lines on a 15 in monitor using 1024x768 resolution.

What is not clear to me is if the new minimum size is respected, why would we drop below that and why would the gripper disappear too?

Comment 25

[Sylvain Pasche]

(In reply to comment #24)
> I noticed the after shot of Vista shows no grippy lines, I'm running the
> default Vista theme and I can reproduce no grippy where the thumb looks like
> its at the minimum size.  I cannot tell if its below the size listed in this
> bug.  But, is that the intended behavior?

What this bug did is to ask the Windows theme about the minimum thumb size and use that value.

On the other hand, the grippy lines are only drawn if there are enough room (that check was introduced in bug 448704).

It happens that at 96 dpi the minimum thumb size returned by Windows is 15px and the check to see if there's enough room to drop the grippy lines returns false so they are not drawn.

If the content to be scrolled makes the thumb be sized to 16px, the grippy lines are drawn.

Note that if you have your system configured to more than 96dpi, the grippy lines are always visible. I personally use 110% font size and I always see the grippy lines (the thumb minimum size is 17px in that case).

But maybe we should always draw the grippy lines even if the thumb size is 15px large. That's what Chrome does apparently.

Could you file a new bug for this? I'm not sure what's the best thing to do (Windows apps are not consistent for this behavior, IE always draws the grippy lines but notepad doesn't when the thumb size is less than 17px). But that should be discussed in a new bug.

Comment 26

[Sylvain Pasche]

s/enough room to drop/enough room to draw/

Comment 27

[[not reading bugmail]]

filed bug 515535 for comment 25.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 396958 [details] [diff] [review]
Patch for 1.9.2

approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.