399545 - <textbox type="number"> spin buttons look wrong with some GTK themes

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Andrew]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007101104 Minefield/3.0a9pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007101104 Minefield/3.0a9pre

the widget thats used to select a number in the preferences window doesn't display properly. 

Reproducible: Always

Steps to Reproduce:
1.Open Preferences Dialog
2.Switch to "Privacy"
3.the widget in the "Remember visited pages for the last xxx days" looks like it had surgery.
Actual Results:  
The arrows on the widget are replaced with ugly white boxes. and the widget also looks like it's been cut in half.

Expected Results:  
This wasn't like this in firefox 2. Something must of happened. It shouldn't look like this. It should be fixed before the firefox 3 beta comes out.

In the URL section of the bug is the Url to the image that shows the problem.

I am running Ubuntu Linux 7.04 with the default theme, and the Firefox 3 Oct. 11th Nightly build.

Comment 1

[Reed Loden [:reed]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007101104 Minefield/3.0a9pre

Confirming. I see the problem, too.

Comment 3

[Hussam Al-Tayeb]

As per Bug 399023, this regressed between 20071005 and 20071006.

Screenshots:
https://bugzilla.mozilla.org/attachment.cgi?id=284006
and
https://bugzilla.mozilla.org/attachment.cgi?id=284007

Comment 4

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

Can you get a more specific regression range (including hours, at least)?

Comment 5

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

Does the widget you see when loading:
data:application/vnd.mozilla.xul+xml,<?xml version="1.0"?><?xml-stylesheet href="chrome://global/skin/" type="text/css"?><window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"><textbox type="number"/></window>

Show the same problem? Linux only?

Comment 6

[Andrew]

@ Gavin
How do i test that code in firefox...I kind of forgot, and also, the problem only exists on Linux.

Comment 7

[Andrew]

@Gavin, Okay, figured it out, it also happens on web pages too.

Comment 10

[Michael Monreal [:micmon]]

bug 399813 looks totally different to this one? why was this marked as a duplicate?

Comment 11

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

(In reply to comment #10)
> bug 399813 looks totally different to this one? why was this marked as a
> duplicate?

The reason it's "deformed" is because the anonymous "input" is too small. That's what the other bug is about.

Comment 12

[Michael Monreal [:micmon]]

Bug 399813 is fixed now but this bug here isn't... Gavin Sharp, is it for you? I don't think so, please reopen.

Comment 13

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

Can anyone else confirm that the URL I gave in comment 5 is still broken, or that they still see this bug?

Comment 14

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

Michi, can you attach a screenshot of what you see?

Comment 15

[Michael Monreal [:micmon]]

Gavin Sharp, It looks the same as the shots in comment #3 on today's nighly. Using the testcase of comment #5, I also see the problem there. I didn't test that one before, so I don't know if something else changed.

Comment 16

[Sylvain Pasche]

This bug was originally about two problems:
1) input too narrow, which is now fixed by bug 399813
2) <textbox type="number"> spin buttons which look wrong

I'm mutating this bug into the second issue which remains. I reported that issue in bug 398805, but that was a separate issue.

Michael explains the problem in bug 398805 comment 4:

"As for the scroll arrows that appears to be Clearlooks' fault. When I switched
my theme to a Mac OS X style the buttons came out fine. Our code does ask GTK
to draw the native spin buttons but that doesn't seem to be happening on
clearlooks-based themes. The arrow is alright..."

I did some tests with the gtk-engines available on Ubuntu 7.10:
Clearlooks, ClearlooksClassic, Glossy, Human: spin buttons painted white.

the others are ok.

Comment 17

[Benjamin Berg]

After a quick look it seems like this is not the themes fault. You should be drawing a box with the "spinbutton" detail and the correct shadow type over the area of both arrow buttons. Then for each of the arrow buttons a box as is already the case.

Comment 18

[Sylvain Pasche]

Attachment: patch, v1

This is adding support for NS_THEME_SPINNER so that we can paint a spinbutton box as mentioned on comment 17.

The transparent background in numberbox.css is to avoid seeing white pixels on the right of the numberbox. Otherwise, it inherits a white background (-moz-Field) from textbox.css in pinstripe.

There is still a minor visual issue: there should be no border rounding on side of the entry touching the spinbuttons.

Comment 19

[Sylvain Pasche]

Attachment: patch, v1.1

removed an unused parameter

Comment 20

[Benjamin Berg]

Hm, how are you drawing the entry? The corner rounding is removed from the entry part of the spinbutton, when the class is GtkSpinButton instead of GtkEntry. So you are probably not passing the gSpinWidget to the entry drawing code.

Comment 21

[Reed Loden [:reed]]

Comment on attachment 291000 [details] [diff] [review]
patch, v1.1

Don't forget to request superreview for core changes.

Comment 22

[Sylvain Pasche]

(In reply to comment #21)
> (From update of attachment 291000 [details] [diff] [review])
> Don't forget to request superreview for core changes.

ok, thanks. I'll try to remember.

(In reply to comment #20)
> Hm, how are you drawing the entry? The corner rounding is removed from the
> entry part of the spinbutton, when the class is GtkSpinButton instead of
> GtkEntry. So you are probably not passing the gSpinWidget to the entry drawing
> code.

The issue is that the entry and the spinbuttons are separate XUL elements. It means that the entry is not aware that is should be drawn differently because is sits next to a spinbuttons. I'm not sure what are the options here to solve this. 

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Might need to create a new NS_THEME_SPINNER_TEXTFIELD which behaves like NS_THEME_TEXTFIELD on most platforms but differently on GTK+.

Comment 24

[Sylvain Pasche]

Thanks for the suggestion. I'll try to have a look at this when I get some time (either here or in a followup).

Meanwhile, do you still want to review the current patch? With the exception of the minor visual glitch above, it's still a big improvement over the current situation for common engines (Clearlooks, Human, ...).

Comment 25

[Sylvain Pasche]

Attachment: patch, v1.2

unbitrot

Comment 26

[Teune van Steeg]

Sylvain, do you mind if I take this bug from you and create a patch based on your patch with the addition of NS_THEME_SPINNER_TEXTFIELD? 

Comment 27

[Sylvain Pasche]

Sure, no problem. Thanks.

Comment 28

[Teune van Steeg]

Attachment: add GTK theming for spinner and spinner-textfield

This is the same patch as Sylvain posted, updated to use NS_THEME_SPINNER_TEXTFIELD to allow the entry part of the spinner widget to be styled as such.

This does not include code to change the behaviour of NS_THEME_SPINNER_TEXTFIELD to NS_THEME_TEXTFIELD on other platforms then GTK. Comment 23 seems to suggest that this is needed, but winstripe and pinstripe will still use the current appearance with this patch.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 293943 [details] [diff] [review]
add GTK theming for spinner and spinner-textfield

treating SPINNER_TEXTFIELD as TEXTFIELD on other platforms would be nice, but we can live without it.

Comment 30

[Reed Loden [:reed]]

Comment on attachment 293943 [details] [diff] [review]
add GTK theming for spinner and spinner-textfield

Fix spin buttons with some themes.

Comment 31

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

Comment on attachment 293943 [details] [diff] [review]
add GTK theming for spinner and spinner-textfield

a=beltzner for 1.9

Comment 32

[Reed Loden [:reed]]

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.59; previous revision: 1.58
done
Checking in widget/src/gtk2/gtkdrawing.h;
/cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v  <--  gtkdrawing.h
new revision: 1.46; previous revision: 1.45
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.127; previous revision: 1.126
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.128; previous revision: 1.127
done
Checking in layout/style/nsCSSKeywordList.h;
/cvsroot/mozilla/layout/style/nsCSSKeywordList.h,v  <--  nsCSSKeywordList.h
new revision: 3.97; previous revision: 3.96
done
Checking in layout/style/nsCSSProps.cpp;
/cvsroot/mozilla/layout/style/nsCSSProps.cpp,v  <--  nsCSSProps.cpp
new revision: 3.157; previous revision: 3.156
done
Checking in toolkit/themes/gnomestripe/global/numberbox.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/numberbox.css,v  <--  numberbox.css
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/themes/winstripe/global/spinbuttons.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/spinbuttons.css,v  <--  spinbuttons.css
new revision: 1.5; previous revision: 1.4
done
Checking in gfx/public/nsThemeConstants.h;
/cvsroot/mozilla/gfx/public/nsThemeConstants.h,v  <--  nsThemeConstants.h
new revision: 1.22; previous revision: 1.21
done

Comment 33

[Michael Monreal [:micmon]]

I think there is a small bug still... using the Clearlooks theme, the bottom/top lines of the box don't reach to the right side (see screenshot) 

Comment 34

[Michael Monreal [:micmon]]

Attachment: Small bug

Comment 36

[u294409]

There's also little glitch with focus, but I can't see him now.
Fixed :D ?

Comment 37

[Sylvain Pasche]

(In reply to comment #36)
> There's also little glitch with focus, but I can't see him now.
> Fixed :D ?

Would that be bug 409388?

Comment 38

[u294409]

Maybe :) .