415494 - Wrong rendering of entries (moz_gtk_entry_paint)

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Julien "_FrnchFrgg_" RIVAUD]

On some themes like thinice, we don't render the textboxes as native ones, we paint the background too small. The problem is that I checked extensively the GTK source, and AFAIU we are doing the exact same thing; indeed, we get X/YTHICKNESS equal to 2 (instead of 1 to get the correct result), and when asking the position of the inner text_area X window, I also get 2 as offset, so the real widget also use a 2px thickness.

Two possibilities:
a) ThinIce uses 2px offset, but draws a bit of the background as part of the border. Then I don't understand why we don't get the same behaviour from thinice than native GTK.
b) There is a problem of initialisation (for instance, the path we use to initialise gtk could incorrectly set the default thickness - the ThinIce gtkrc seems not to define a lot of things and leaves a lot to default values - or something similar)

...

c) Both ?

Note 1: OTOH this 2px thickness instead of 1 (which causes the background to show trough) is wanted for treeviews... ThinIce seems to be broken in strange ways.

Note 2: Industrial seems also affected by this bug ? (not 100% sure, it's less visible, but pretty sure nevertheless)

Comment 1

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Paint the solid background before everything else [v1]

I figured what is going on. gtkentry.c sets the bg color of its two windows to base[GTK_STATE_...] in the realize function; we thus need to simulate this drawing prior to any other rendering. I also changed the comments since they no longer matched what is done on native gtk. Now our paint structure really is the same as the native one.

Comment 2

[Teune van Steeg]

Comment on attachment 301567 [details] [diff] [review]
Paint the solid background before everything else [v1]

> 
>+    /* gtkentry.c uses two windows, one for the entire widget and one for
>+     * the text area inside it. The background of both windows is set to
>+     * the "base" color of GTK_STATE_NORMAL (emulated by gdk_draw_rectangle
>+     * below), but only the inner textarea window uses gtk_paint_flat_box
>+     * when exposed */

This comment is not correct per bug 414748 comment #3 (paint_flat_box should be painted with GTK_STATE_INSENSITIVE when disabled, the patch in bug 414748 fixes that)

>-    TSOffsetStyleGCs(style, rect->x + x, rect->y + y);
>+    /* Draw the default window background */
>+    gdk_draw_rectangle(drawable, style->base_gc[GTK_STATE_NORMAL], TRUE,
>+                       rect->x, rect->y, rect->width, rect->height);

Maybe this should be painted with GTK_STATE_INSENSITIVE too when disabled?
And I assume this new code also fixes a glitch with the Crux Theme for the ComboBoxEntry (a small area of missing white background between the text field and button), that the patch in bug 14748 tries to hack around? Or is that hack still necessary?

Comment 3

[Julien "_FrnchFrgg_" RIVAUD]

There is

gdk_window_set_background (widget->window, &widget->style->base[GTK_WIDGET_STATE (widget)]);

in gtk_entry_realize() and the widget state at realize time is almost always GTK_STATE_NORMAL. The comment is thus right. What I didn't see is that the same code is also in gtk_entry_state_changed(). The comment (and the code) should thus be modified to take that into account.

Note that when I tried to use ConvertGtkState to get the state for the call of flat_box_paint, I got a wrong color on hover; the code in 414748 is probably better even if it seems more limited (what if some themes use more states to decide how to paint ? gtkentry.c seems to pass to the drawing functions whatever state the widget is in... yet they don't get the strange hover effect)

Comment 4

[Benjamin Berg]

GtkEntry does pass the state that it is in to the drawing functions. But this state can only be NORMAL or INSENSITIVE. The GtkEntry widget never gets a prelight, active or selected state.

Comment 5

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Paint the solid background before everything else [v2]

Addresses twanno's comments. This patch is on top of attachment 300378 [details] [diff] [review].

Comment 6

[Julien "_FrnchFrgg_" RIVAUD]

Benjamin: Since I saw a lot of your patches on gtk-engines gnome svn, could you look at what I do ? On a side note, are you on irc.gimp.net #gtk+ or freenode #gtk or something ? I have the ability and the time to tweak gecko's native looking, but I sometimes need advice on what to call. It seems that a lot of the time I spent on this bug could have been saved had I known what you know...

Comment 7

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Paint the solid background before everything else [v2.1]

Previous patch had a merge mistake resulting in reverted GTK_STATE_NORMAL --> bg_state change.

Comment 8

[Teune van Steeg]

Comment on attachment 301588 [details] [diff] [review]
Paint the solid background before everything else [v2.1]

>     bg_width = rect->width - 2*x;
> 
>     if (GTK_IS_COMBO_BOX_ENTRY(widget->parent)) {
>         bg_width += x;
>         if (direction == GTK_TEXT_DIR_RTL)
>             x = 0;
>     }
>
This hack is not needed anymore with this patch and can be removed now.

Comment 9

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Paint the solid background before everything else [v2.2]

This patch also contains the removal of now unneeded hack.

Comment 10

[Benjamin Berg]

FrnchFrgg: I am on irc.gnome.org #gtk+ and #gnome-art. My nick is "benzea". Feel free to ask questions. I am currently always online, so it may take a long time before I respond.

Oh, if you don't know about the wiki yet. I have done some documentation on live.gnome.org in the past which is available at.
http://live.gnome.org/GnomeArt/Tutorials/GtkThemes

Comment 11

[Julien "_FrnchFrgg_" RIVAUD]

Comment on attachment 301598 [details] [diff] [review]
Paint the solid background before everything else [v2.2]

This is a cheap change (that even enable us to remove some hacks) which can correct very broken look of gtk entries (text areas), especially for themes with big x/ythickness.

Comment 12

[Benjamin Berg]

Hm, this is an interesting problem. Because if you already clear the whole background (as GTK+ does), then it is impossible to get transparent corners as discussed in bug #405421.

So I guess that there are two ways:
 1. Clear the background, and live with non transparent corners
 2. Do not clear the background, which probably breaks some themes.

Comment 13

[Julien "_FrnchFrgg_" RIVAUD]

The problem is that not clearing the background breaks every theme I tested apart from Clearlooks (which seems to paint the white bg as part of the shadow); this is especially visible if x/ythickness is enlarged in the gtkrc.

The default themes which come with gtk-engines can be fixed, but what to do about the myriad of pixmap based themes (and custom engines) out there ?

Comment 14

[Damon Sicore (:damons)]

Comment on attachment 301598 [details] [diff] [review]
Paint the solid background before everything else [v2.2]

a1.9+=damons

Comment 15

[Reed Loden [:reed]]

Hey, please unbitrot the patch. Several patches landed lately that really bitrot this. :)

Comment 16

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Paint the solid background before everything else [v2.3, unbitrotted for checkin]

Ah, it bitrotted ? git completely transparently rebased it on latest trunk, so I thought is was minor.

Comment 17

[Reed Loden [:reed]]

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.78; previous revision: 1.77
done