254002 - <toolbarbutton> borders not stylable on linux; checking not working

Status: RESOLVED DUPLICATE of bug 404514

(Core :: XUL, defect)

Description

[Alex Fritze]

See testcase: http://www.croczilla.com/~alex/testcases/toolbar-bug.xul
The first toolbar button has attribute checked="true" and should be displayed
with an inset-type border. It is under Windows but not under Linux. 

More generally, styling of borders doesn't work for toolbarbuttons under Linux
at all: The second button should appear with a 2px solid red border. Again, it
doesn't under Linux.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

what happens if you also style it with -moz-appearance:none !important; ?

Comment 2

[Alex Fritze]

(In reply to comment #1)
> what happens if you also style it with -moz-appearance:none !important; ?

See http://www.croczilla.com/~alex/testcases/toolbar-bug2.xul
Interestingly, this makes the 'checked' case work, but not the non-checked one...

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

hm... what if you make the border !important, too?

Comment 4

[Alex Fritze]

Good point, but doesn't work either unfortunately. I've modified
http://www.croczilla.com/~alex/testcases/toolbar-bug2.xul accordingly.

Comment 5

[Pawel Chmielowski]

Attachment: patch ver 1

This patch adds native rendering of checked buttons in gtk2. First testcase is solved by this change, second works for me even without this patch.

Who can review this?

Comment 6

[jan klopper]

FYI,

The first testcase gives me two default buttons, no styles what so ever.

The seccond testcase gives me two styled buttons, the first "selected/checked" and The seccond with a 2px(i think) red border all around.

No patches aplied, plain vanilla 1.5;
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

Comment 7

[Pawel Chmielowski]

Attachment: ver 2

Remove gtk warnings and some indentation fixes.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 209226 [details] [diff] [review]
ver 2

>--- .pc/gtk-toolbarbutton-checked-fix.diff/widget/src/gtk2/gtk2drawing.c	2006-01-21 18:44:26.000000000 +0100
>+++ widget/src/gtk2/gtk2drawing.c	2006-01-21 19:01:33.000000000 +0100
>@@ -940,17 +945,17 @@ moz_gtk_dropdown_arrow_paint(GdkDrawable
> {
>     GdkRectangle arrow_rect, real_arrow_rect;
>     GtkStateType state_type = ConvertGtkState(state);
>     GtkShadowType shadow_type = state->active ? GTK_SHADOW_IN : GTK_SHADOW_OUT;
>     GtkStyle* style;
> 
>     ensure_arrow_widget();
>     moz_gtk_button_paint(drawable, rect, cliprect, state, GTK_RELIEF_NORMAL,
>-                         gDropdownButtonWidget);
>+                         gDropdownButtonWidget, 0);

Use FALSE here instead of 0 to be consistent with the gboolean type.

>@@ -1567,17 +1572,18 @@ gint
> moz_gtk_widget_paint(GtkThemeWidgetType widget, GdkDrawable* drawable,
>                      GdkRectangle* rect, GdkRectangle* cliprect,
>                      GtkWidgetState* state, gint flags)
> {
>     switch (widget) {
>     case MOZ_GTK_BUTTON:
>         ensure_button_widget();
>         return moz_gtk_button_paint(drawable, rect, cliprect, state,
>-                                    (GtkReliefStyle) flags, gButtonWidget);
>+                                    (GtkReliefStyle) flags&MOZ_GTK_BUTTON_RELIEF_MASK,
>+                                    gButtonWidget, flags&MOZ_GTK_BUTTON_CHECKED);

Space on both sides of '&'

Looks good otherwise.

Comment 9

[Pawel Chmielowski]

Attachment: ver. 3

Updated to reflect review comments.

Comment 10

[David Baron :dbaron:]

Comment on attachment 210033 [details] [diff] [review]
ver. 3

>-    GtkStateType button_state = ConvertGtkState(state);
>+    GtkStateType button_state =
>+        (checked && !state->inHover) ? GTK_STATE_ACTIVE : ConvertGtkState(state);

Might you not want to do this if state->disabled ?

sr=dbaron, although this isn't really an area of code I'm particularly strong in

Comment 11

[Pawel Chmielowski]

Attachment: ver. 4

This update fix handling of disabled state buttons. Brian do you wan't to review it again?

Comment 12

[Pawel Chmielowski]

Attachment: CSS tweak

To fully mimmic gtk look small CSS tweak is needed. It changes label color for disabled and checked toolbar buttons.

Comment 13

[Pawel Chmielowski]

Attachment: New testcase

Updated testcase, which shows also buttons in disabled state.

Comment 14

[Pawel Chmielowski]

Attachment: ver. 5

Don't add trailing whitespaces...

Comment 15

[Pawel Chmielowski]

Attachment: v6

Updated to tip.

Comment 16

[Marek Stępień [:marcoos, inactive]]

Any chances for getting this in for Fx 2.0?

Comment 17

[Marek Stępień [:marcoos, inactive]]

Pawe³, a "version 7" of this patch should probably include reverting the changes caused by the so-called "fix" for bug 334596.

Comment 18

[Marek Stępień [:marcoos, inactive]]

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

Comment 19

[Marek Stępień [:marcoos, inactive]]

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

Comment 20

[Prasad Sunkari [:prasad]]

Attachment: Patch updated to the trunk

Updated the patch to the trunk as of 2007-12-27.

Comment 21

[Prasad Sunkari [:prasad]]

Comment on attachment 294663 [details] [diff] [review]
Patch updated to the trunk

Marking this patch obsolete because the trunk as on 2007-12-28 has proper support to this.

Comment 22

[Prasad Sunkari [:prasad]]

Marking it as a duplicate of a much later bug because the later one got committed to CVS.