119735 - NS_THEME_WINDOW and NS_THEME_DIALOG implementations (GTK)

Status: RESOLVED FIXED

(Core Graveyard :: Skinability, enhancement)

Description

[Hixie (not reading bugmail)]

This covers the implementation of NS_THEME_WINDOW and NS_THEME_DIALOG for XUL
window and dialog backgrounds on GTK.

Comment 1

[Pierre Chanial]

taking

Comment 2

[Pierre Chanial]

Attachment: patch v1.0

this patch applies for gtk2 and it only paints the bg of a window when it's a
pixmap.
I think we should fork nsNativeThemeGTK.cpp for GTK2. I don't have an gtk1
environment that I can test, and even if I had one, I think it's wasted time.
Furthermore, gtk1 does not handle some widgets like trees. So, instead of
#ifdef'ing nsNativeThemeGTK.cpp, I copied nsNativeThemeGTK.cpp into
nsNativeThemeGTK2.cpp to make sure to not break it.

Note: the diff are against nsNativeThemeGTK.cpp, I just copied the gtk2 version
into the gtk1 one to produce them quickly.
I we agree on forking nsNativeThemeGTK.cpp, then the blame and log files should
be copied, also.

Comment 3

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

Comment on attachment 140283 [details] [diff] [review]
patch v1.0

Why did you make protoLayout global?

I'm OK with making nsNativeThemeGTK2.cpp but really we should figure out how to
share code between these two.

Why do you only paint the background if it's a pixmap?

Comment 4

[Pierre Chanial]

(In reply to comment #3)
> (From update of attachment 140283 [details] [diff] [review])
> Why did you make protoLayout global?
good point, it wasn't necessary

> I'm OK with making nsNativeThemeGTK2.cpp but really we should figure out how to
> share code between these two.
After discussing with bryner, I'll keep sharing nsNativeThemeGTK.cpp and only
#ifdef nsNativeThemeGTK::ThemeSupportsWidget
There will be a small overhead for gtk1, but the shared code will stay clean.

> Why do you only paint the background if it's a pixmap?
I checked that we don't need to paint here if the background isn't a pixmap. I
was concerned about painting the background twice. The comment should rather be:
+    /* if the window background isn't a pixmap, we don't paint here, because
+       the view manager has already initialized the back buffer with the bg
color. */
I haven't looked at the code where it happens, though, so the comment may not be
accurate.

Comment 5

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

So you're saying that GTK2 only has background colors and pixmaps, and that we
already use the GTK2 background color? I guess the color would work by virtue of
the CSS system colors.

Comment 6

[Pierre Chanial]

(In reply to comment #5)
> So you're saying that GTK2 only has background colors and pixmaps

Yes, both can be specified in the gtk theme, but the pixmap overrides the 
bg[GTK_STATE_NORMAL] color for when gtk paints windows background.

> and that we already use the GTK2 background color? 
Yes, to initialize the back buffer.

Comment 7

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

> Yes, to initialize the back buffer.

I don't know what you mean. The backbuffer is initialized to a fixed gray color
in nsViewManager. If the GTK2 background color is used at all it must be used
via CSS system colors.

Comment 8

[Pierre Chanial]

(In reply to comment #7)
> If the GTK2 background color is used at all it must be used
> via CSS system colors.

Indeed. I have investigated this issue in more details yesterday.
The painting of root elements is not straightforward: the canvas is painted
first, and it takes the nsStyleBackground of its first non transparent child.
Then the child frame background is painted but infact, it is specifically
skipped in order not to paint twice.

So here is what happens when we take the simplest xul file that contains only a
window element. The rootBoxFrame (canvas) and the boxFrame of the <window> root
element are painted one after the other.
During the first phase (painting of the rootBoxFrame background),
|FindBackground| only returns the nsStyleBackground of its first child, the
boxFrame. This struct contains no info about appearance: that's why the
background-color specified on the <window> element via CSS is painted.
During the second phase (painting of the boxFrame background), there is some
code added by hand not to skip its painting if an appearance is specified.

So that's why we paint twice if we specify an appearance for <window> elements
and once if we don't.
I propose to remove the hack in the second phase (and of course the one in my
patch), so that we paint the background color/appearance only during the first
phase. To do so, we need more information from |FindBackground|. By returning
the actual frame instead of its nsStyleBackground , we could pull from it both
the nsStyleBackground and the display data structure.
It would also spare some cycles, since not all the callers of |FindBackground|
need the nsStyleBackground.

I probably won't have time to do the patch before wednesday, unless I can find a
time slot, but comments are welcome.

Comment 9

[Pierre Chanial]

the hack in findbackground was introduced in bug 119736.

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 140283 [details] [diff] [review]
patch v1.0

Hm.  Looking at what gtk+ 2.2.4 does for GtkWindow, it looks like we could just
do:

  gtk_paint_flat_box (widget->style, widget->window, GTK_STATE_NORMAL,
		      GTK_SHADOW_NONE, area, widget, "base", 0, 0, -1, -1);

(from gtk_window_paint)

but gtk_default_draw_flat_box appears to never apply a pixmap background if you
pass in a pixmap, which we are here.  So, I think we need more or less the same
logic that's in gtk_default_draw_flat_box in the gtk source, which is
basically:

if (!style->bg_pixmap[GTK_STATE_NORMAL]) {
  gdk_draw_rectangle( ... );
} else {
  gtk_style_apply_default_background( ... );
}

Comment 11

[Pierre Chanial]

(In reply to comment #10)
>  So, I think we need more or less the same
> logic that's in gtk_default_draw_flat_box in the gtk source, which is
> basically:
> 
> if (!style->bg_pixmap[GTK_STATE_NORMAL]) {
>   gdk_draw_rectangle( ... );
> } else {
>   gtk_style_apply_default_background( ... );
> }
> 

That's indeed the logical path I followed, but then, in
gtk_style_apply_default_background, there's the same statement:
if (!style->bg_pixmap[GTK_STATE_NORMAL]) {
  gdk_draw_rectangle( ... );
} else {
  gdk_window_set_back_pixmap(...);
}

hence, it's not necessary to have it in our window paint routine.

Comment 12

[Pierre Chanial]

Attachment: patch v1.1

patch addressing Roc's concerns.
Per discussion with bryner, I also removed the calls to
gtk_widget_set_rc_style, since a style will be attached when the widget is
realized.

Comment 13

[Pierre Chanial]

Comment on attachment 140283 [details] [diff] [review]
patch v1.0

transferring r/sr

Comment 14

[Pierre Chanial]

cc'ing dbaron and bz.
I'd be glad to correct the background painting so that it would be done with the
canvas and not the Root Box Frame as described in comment 8, but before I'd like
to have an approval to change all the clients of |FindBackground| and eventually
|FindFrameBackground| if consistency is needed here.

That shouldn't prevent reviews for the last patch :).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixing FindBackground to return the frame sounds ok to me.

Comment 16

[Pierre Chanial]

(In reply to comment #15)
> Fixing FindBackground to return the frame sounds ok to me.

and nsPresContext::FindFrameBackground also?

Comment 17

[Boris Zbarsky [:bzbarsky]]

The only caller of that shouldn't be using these low-level apis and should just
use computed style, in my opinion.  I would rather remove it than anything else.

Comment 18

[Brian Ryner (not reading)]

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

> gint
> moz_gtk_get_widget_border(GtkThemeWidgetType widget, gint* xthickness,
>                           gint* ythickness)
>@@ -953,13 +977,14 @@
>         break;
>     case MOZ_GTK_CHECKBUTTON_CONTAINER:
>     case MOZ_GTK_RADIOBUTTON_CONTAINER:
>-        /* This is a hardcoded value. */
>+    /* This is a hardcoded value. */

This indenting is off.

r=bryner with that fixed.

Comment 19

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

It would be great if you can fix FindBackground.

Comment 20

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

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

This looks great. Thanks much

Comment 21

[Pierre Chanial]

(In reply to comment #19)
> It would be great if you can fix FindBackground.
follow up in bug 235189.

Marking fixed.