424074 - [Mac] Scrollbar thumb is drawn with wrong size for custom scrollbars

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Markus Stange [:mstange]]

Attachment: Fix (use pageincrement attribute for thumb size)

When creating a custom scrollbar using the XUL <scrollbar> element, the size of the scrollbar's thumb can be controlled with the pageincrement attribute.

nsNativeThemeCocoa::GetScrollbarDrawInfo does not use the pageincrement attribute when setting the thumb size; it makes the assumption that pageincrement equals the scrollbar size, which luckily works for normal scrollbars. However, it makes custom scrollbars behave very strangely.

Comment 1

[Markus Stange [:mstange]]

Attachment: Testcase

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 310730 [details] [diff] [review]
Fix (use pageincrement attribute for thumb size)

Is pageincrement guaranteed to be present?  If so, and if it has the correct value at, then this looks fine for me but get sr frm josh, please.

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 310730 [details] [diff] [review]
Fix (use pageincrement attribute for thumb size)

(In reply to comment #2)
> Is pageincrement guaranteed to be present?

The pageincrement attribute has a default value (10), so I think it is.

Comment 4

[Damon Sicore (:damons)]

+'ing w/ P2.  Josh, please reprioritize/minus if necessary.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 310730 [details] [diff] [review]
Fix (use pageincrement attribute for thumb size)

Looks fine, add a reftest?  (Compare whether a scrollbar without an explicit thumb size looks different than one with.)

Comment 6

[Markus Stange [:mstange]]

Attachment: Reftest

Do reftests need to be reviewed?

Comment 7

[Markus Stange [:mstange]]

Attachment: The real reftest

Well, I did manage to get it wrong, so maybe they should...

Comment 8

[Markus Stange [:mstange]]

I'm thinking too much. The other testcase wasn't wrong.

But the patch is wrong. We have to take care of using the right default value ourselves. CheckIntAttr just returns 0 when the attribute isn't set.

A new patch is coming. Sorry for the inconvenience.

Comment 9

[Markus Stange [:mstange]]

Attachment: Fix v2: Use correct default values

I've changed nsNativeTheme::CheckIntAttr to accept a default value as third parameter. Now we're matching the behavior of nsSliderFrame:
http://mxr.mozilla.org/firefox/source/layout/xul/base/src/nsSliderFrame.cpp#185

Again, sorry that I didn't investigate this issue the first time.

Comment 10

[Josh Aas]

// These are used by nsNativeThemeGtk

Should probably just remove this comment since it is no longer true.

Comment 11

[Markus Stange [:mstange]]

Attachment: Fix v2.1, ready for checkin

I've removed the comment and added better reftests.
The following is tested:
1. If the right default values are applied when the attributes are not set
2. If setting pageincrement to a different value changes the appearance
3. If the thumbsize is the same for the pageincrement/maxpos combinations 40/100 and 400/1000.

Comment 12

[Josh Aas]

landed on trunk, thanks Markus

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #12)
> landed on trunk, thanks Markus

You missed crediting Markus with the patch in your checkin comment.

Comment 14

[Josh Aas]

oops, sorry

Comment 15

[Samuel Sidler (old account; do not CC)]

(In reply to comment #13)
> You missed crediting Markus with the patch in your checkin comment.

Filed bug 425426 to fix the checkin comment.

Comment 16

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre and the testcase from markus

--> Verified fixed