337381 - Button on dropdown lists too skinny

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[dolphinling]

Gentoo Linux, Gnome, Firefox trunk.

The button on dropdown lists is too skinny. It's been this way for quite a while (I have the regression range narrowed down to "between Feb. 9 and Mar. 11", I'll get it narrowed down more). I'll attach a screenshot to show the problem. 

I won't be surprised if this is a dupe, since it's been here for so long, but I couldn't find it in a search...

Comment 1

[dolphinling]

Attachment: Screenshot

Here's the screenshot, regression range coming.

Comment 2

[dolphinling]

Regressed between 2006-04-06 and 2006-04-07

Bonsai link:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-04-06&maxdate=2006-04-07+06&cvsroot=%2Fcvsroot

Comment 3

[Adam Guthrie]

Nothing jumps out at me in the check-in range except cairo stuff. ccing vlad.

Comment 4

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

This has something to do with native theme handling; roc did a large chunk of that code, I'd guess that we're not handling some internal padding or somesuch and giving gtk a too-small rectangle in which to draw the element.

Comment 5

[Adam Guthrie]

So this may have something to do with theming, but the fact is that nothing theme-related landed during the regression range.

Looking at the check-ins, bug 78081 looks suspicious. There is also bug 332640, which was supposedly backed out (see bug 333051).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Guys, this just regressed when cairo was enabled in nightlies; this presumably wasn't a checkin but a configuration change on the tinderbox; checking about:buildconfig should confirm.

Anyway, the reason this regressed is that the combobox makes the dropdown marker the same width as the dropdown's scrollbar, and the implementation of nsThebesDeviceContext::GetScrollBarDimensions is just completely bogus on Linux.  For that matter, it's bogus on all non-Windows platforms; once Mac cairo is on you'll get this problem there too.

Comment 7

[Adam Guthrie]

Attachment: Use what nsDeviceContextGTK was using (for now)

Boris pointed me to where to find this on IRC; I just made the patch. It's sort of a hack, but it works.

He also said that we should possibly move this function into native theme stuff somewhere since we're going to need it for Windows, Linux, Mac, etc.

Comment 8

[Adam Guthrie]

Comment on attachment 234541 [details] [diff] [review]
Use what nsDeviceContextGTK was using (for now)

It'd make me happy if we could get this in, at least until we come up with a better solution. I miss pretty dropdown buttons. :(

Comment 9

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

Comment on attachment 234541 [details] [diff] [review]
Use what nsDeviceContextGTK was using (for now)

I guess this is ok, though I'd prefer to cache this and blow it away when the theme changes, but whatever.

Comment 10

[Adam Guthrie]

OK. I filed bug 349370 for caching the scrollbar size.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Er... we need to cache this, like non-cairo builds do.  Otherwise it'll almost certainly be a performance issue.

Comment 12

[Adam Guthrie]

Attachment: Use what nsDeviceContextGTK was using (for now) v2.0

OK. This patch caches the size just like nsDeviceContextGTK did.

Comment 13

[Adam Guthrie]

*** Bug 349370 has been marked as a duplicate of this bug. ***

Comment 14

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

Comment on attachment 235687 [details] [diff] [review]
Use what nsDeviceContextGTK was using (for now) v2.0

ifdef MOZ_ENABLE_GTK2 around the mGTKScrollbarHeight/Width members; they don't need to be there.

Also, please file a new bug for moving scrollbar width/height code into widget native theme code; this code shouldn't live on the device context.

Comment 15

[Adam Guthrie]

Attachment: Use what nsDeviceContextGTK was using (for now) v2.1

OK. I filed bug 350766 to move this code into widget/.

Comment 16

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

Checked in