Cosmetic bug: rectangular white outline surrounding rounded buttons

[Conn O Griofa]

Ubuntu release: Hardy (fresh install from Alpha5 desktop cd, all updates as of February 26th)
System: Dell Inspiron 510m laptop
Graphics subsystem: VGA compatible controller: Intel Corporation 82852/855GM Integrated Graphics Device (rev 02)

Rounded GTK+ buttons are rendered with a rectangular white outline, most noticeable in the new Firefox 3.0 Beta 3 release when browsing Gmail (not not limited to this site). I am attaching two small screenshots; one of the "Archive" button while using the "Ubuntulooks" theme, and the same button while using "Clearlooks". You may need to zoom the images as I cropped them in order to avoid personal information being exposed.

Steps to reproduce:
1. Ensure Ubuntulooks theme is active
2. Open Firefox 3, navigate to http://mail.google.com and log in to your account
3. Observe the "Archive" and "Report spam" buttons against the blue background.
4. Change to any other theme installed by default in Ubuntu, and repeat steps 2-3.

Expected results: Buttons have no noticeable outline, or else a thin white outline, rounded at the edges for rounded buttons.

Actual results:
1. For Ubuntulooks theme: Buttons have a white rectangular outline, creating noticeable white marks at the edges of rounded buttons.
2. For all other themes installed by default: results are as expected (above)

Note: This is most likely not a Firefox 3 or core GTK+ bug, as all other themes besides Ubuntulooks work as expected.

[Michael-monreal+moz (michael-monreal+moz)]

Created attachment 290200
Screenshots

Various screenshots of the problem. For websites, you can take http://www.schierer.org/~luke/log/ as a testcase (see the login fields).

[Roc-ocallahan (roc-ocallahan)]

There's really nothing we can do here except turn off native themes for HTML, or better, have the GTK theme authors fix their themes.

[Michael-monreal+moz (michael-monreal+moz)]

Robert: can you explain a bit more? What is "wrong" with Clearlooks? If there is something that can be done at the GTK theme level, I am sure it will be done. After posting that bug I had a look at the current WebKit/GTK and found the exact same problem there...

[Ventnor-bugzilla (ventnor-bugzilla)]

This is indeed the doing of GTK theme engines. After all the hard work that has been done it would be a big shame to turn off native HTML themes, so I think we should push this forward anyway in the hope that theme authors will fix this. Because if we don't push this forward then theme authors will have no incentive to fix this.

[Michael-monreal+moz (michael-monreal+moz)]

I fully agree with you. Even with broken themes, the widgets look great in about 95% of all cases (luckily most webpages are black-on-white or at least dark-on-bright).

I only wonder, do theme authors actually know about this issue? If they are not fully aware, I'll make sure they get their attention to this ;)

[Roc-ocallahan (roc-ocallahan)]

I agree, I was wrong to turn off HTML themes before, we just have to ship them and wait for GTK theme authors to fall into line.

Any attention you can draw to this issue would be helpful :-).

The basic issue here is that theme authors assume that widgets are being drawn on top of the theme's designated window background color (in the case of your screenshot, white). So they often draw borders and bevels using that background color around the outside of the widget. This gives the results you see.

Basically theme authors need to stop making assumptions about the window background color and try to make their themes look good no matter what that background color is. This is hard in some cases but in other cases it's simply a matter of using transparency instead of drawing the window background color as part of the widget.

[BenjaminBerg (benjamin-sipsolutions)]

Just so things are clear, I am the current maintainer of gtk-engines.

This is a lot an issue that is a lot more complex than just "using transparency instead of drawing the window background color as part of the widget". We *need* to draw the background color in a lot of cases (GtkEntry, GtkProgressBar)! And I certainly would have removed all this if it was possible.

I don't want to go into too much detail here, but I would not be suprised if this is not fixable in GTK+. (I have not investigated this possibility too much.)

So this means the only way to achieve the correct behaviour would be to somehow work together and hint the engine/theme that filling the background is not necessary.

(Hm, I shouldn't missuse this bug to complain, but I do not like mozillas GTK+ emulation stuff in some ways. For example you could at least give the GtkWindow or the GtkFixed a name with gtk_widget_set_name to make it possible to create sane special cases for mozilla.)

[BenjaminBerg (benjamin-sipsolutions)]

Oh, the checkbox thing can be considered a theme bug. Alpha blending is perfectly possible in this case.

[Ventnor-bugzilla (ventnor-bugzilla)]

Sorry Benjamin but I don't think there is a lot we can do. I'm trying to be a good player by following GTK's code as much as possible, but as a web browser with much more cases to consider than a typical application we don't really have a choice but to emulate GTK rather than use it. Even native browsers such as IE and Safari emulate their widgets rather than use the typical API's, so I'm told by Roc.

(In reply to comment #7)
> (Hm, I shouldn't missuse this bug to complain, but I do not like mozillas GTK+
> emulation stuff in some ways. For example you could at least give the GtkWindow
> or the GtkFixed a name with gtk_widget_set_name to make it possible to create
> sane special cases for mozilla.)
>

We might be able to do this though.

[Roc-ocallahan (roc-ocallahan)]

(In reply to comment #7)
> Just so things are clear, I am the current maintainer of gtk-engines.

Welcome to the party :-)

> This is a lot an issue that is a lot more complex than just "using
> transparency instead of drawing the window background color as part of the
> widget".

Sure.

> We *need* to draw the background color in a lot of cases (GtkEntry,
> GtkProgressBar)!

I believe you, but why is that?

> I don't want to go into too much detail here, but I would not be suprised if
> this is not fixable in GTK+. (I have not investigated this possibility too
> much.)

That depends on how much you are willing to change GTK+ or the themes, I guess, since it's possible to design themes that don't show these problems.

> So this means the only way to achieve the correct behaviour would be to
> somehow work together and hint the engine/theme that filling the background
> is not necessary.

Interesting. Can you say more about how that might work?

> (Hm, I shouldn't missuse this bug to complain, but I do not like mozillas GTK+
> emulation stuff in some ways. For example you could at least give the
> GtkWindow or the GtkFixed a name with gtk_widget_set_name to make it possible
> to create sane special cases for mozilla.)

That's a good idea, but I would call it something more like "HTML" because Webkit/GTK+ is using this code too.

What other suggestions do you have?

One thing I really want is the ability to pass a cairo_t directly down to a theme at least for those themes that draw using cairo. Right now when we have to draw a widget with rotation or scale, or to a non-X surface (printing), we have to use horrible hacks involving temporary pixmaps. When the widget is partially transparent the hacks are even more horrible.

[BenjaminBerg (benjamin-sipsolutions)]

(In reply to comment #10)
> > This is a lot an issue that is a lot more complex than just "using
> > transparency instead of drawing the window background color as part of the
> > widget".
>
> Sure.
>
> > We *need* to draw the background color in a lot of cases (GtkEntry,
> > GtkProgressBar)!
>
> I believe you, but why is that?

The problem with the GtkEntry is that it consists of two X windows (one around everything, and one for the text area). Both of these windows are filled with base[NORMAL], ie. the background color for the text. So to get the rounded look we need to fill the background with the correct color.

The progress bar case would may be easier to fix. The problem here is that GTK+ caches the bar (without text) in a server side pixmap. I do not know what the reason for doing this is, however it means that we need to fill the background to prevent displaying uninitilized memory.

> > I don't want to go into too much detail here, but I would not be suprised if
> > this is not fixable in GTK+. (I have not investigated this possibility too
> > much.)
>
> That depends on how much you are willing to change GTK+ or the themes, I guess,
> since it's possible to design themes that don't show these problems.

As just mentioned, the core problem is in GTK+. Both the back buffer, and the second window should not always be necessary. The problem I see is that the API/ABI stability might be broken in some cases. (Or eg. themes get broken if the background is not filled with base[NORMAL] anymore in the entry.)

> > So this means the only way to achieve the correct behaviour would be to
> > somehow work together and hint the engine/theme that filling the background
> > is not necessary.
>
> Interesting. Can you say more about how that might work?

Not sure. It may work by attaching extra data to the widget. Or having a certain widget path.

> > (Hm, I shouldn't missuse this bug to complain, but I do not like mozillas GTK+
> > emulation stuff in some ways. For example you could at least give the
> > GtkWindow or the GtkFixed a name with gtk_widget_set_name to make it possible
> > to create sane special cases for mozilla.)
>
> That's a good idea, but I would call it something more like "HTML" because
> Webkit/GTK+ is using this code too.
>
> What other suggestions do you have?

Not sure right now. I'll need to think about the problem again, and look at the mozilla code.
Oh, one thing that strikes me as rather weird is that IIRC a GtkInvisible is used to get the default colors for the HTML page. This could be handled better by using this "HTML" container widget (whatever it is).

> One thing I really want is the ability to pass a cairo_t directly down to a
> theme at least for those themes that draw using cairo. Right now when we have
> to draw a widget with rotation or scale, or to a non-X surface (printing), we
> have to use horrible hacks involving temporary pixmaps. When the widget is
> partially transparent the hacks are even more horrible.

Yeah, I remember seeing the GTK+ bug.

Not sure how this may work. I do not know that much about GDK internals. This would either require changing the engine API, or special GDK code that is able t...

[Roc-ocallahan (roc-ocallahan)]

I think the way to go would be to extend the engine API to provide additional entry points that take cairo_t parameters. I don't know how practical that is.

> The problem with the GtkEntry is that it consists of two X windows (one around
> everything, and one for the text area). Both of these windows are filled with
> base[NORMAL], ie. the background color for the text. So to get the rounded
> look we need to fill the background with the correct color.

It seems to me that in this case the outside window should be transparent instead of being prefilled with the text background color. Maybe that doesn't make sense for a GdkDrawable API but it would make sense for cairo_t API.

Another option is that using cairo, it is possible to draw with OPERATOR_SOURCE or OPERATOR_CLEAR and "knock out" some pixels, making them transparent or translucent where they're currently opaque. So it would be possible to actually make the area outside the rounded border transparent even if it started off with a solid color.

> The progress bar case would may be easier to fix. The problem here is that
> GTK+ caches the bar (without text) in a server side pixmap. I do not know
> what the reason for doing this is, however it means that we need to fill the
> background to prevent displaying uninitilized memory.

cairo would fix this again because you could cache an RGBA surface instead of a pixmap. (Actually the RGBA surface would be an XRender pixmap.)

So it seems to me that GTK+ should move towards a cairo-based API for theme engines, then these problems can be fixed on the GTK+ side and we can have a better interface for our needs too.

[BenjaminBerg (benjamin-sipsolutions)]

(In reply to comment #12)
> I think the way to go would be to extend the engine API to provide additional
> entry points that take cairo_t parameters. I don't know how practical that is.

There are some 20 darwing functions, but only 12 slots left to add more funtions to GtkStyle. So it is not possible to just add a second set of functions to engines.

> > The problem with the GtkEntry is that it consists of two X windows (one around
> > everything, and one for the text area). Both of these windows are filled with
> > base[NORMAL], ie. the background color for the text. So to get the rounded
> > look we need to fill the background with the correct color.
>
> It seems to me that in this case the outside window should be transparent
> instead of being prefilled with the text background color. Maybe that doesn't
> make sense for a GdkDrawable API but it would make sense for cairo_t API.
>
> Another option is that using cairo, it is possible to draw with OPERATOR_SOURCE
> or OPERATOR_CLEAR and "knock out" some pixels, making them transparent or
> translucent where they're currently opaque. So it would be possible to actually
> make the area outside the rounded border transparent even if it started off
> with a solid color.

GTK+ does not use windows with an alpha channel by default. Also wouldn't this need a working composition manager? What about older X versions and the different backends (directfb, windows, osx)?

> > The progress bar case would may be easier to fix. The problem here is that
> > GTK+ caches the bar (without text) in a server side pixmap. I do not know
> > what the reason for doing this is, however it means that we need to fill the
> > background to prevent displaying uninitilized memory.
>
> cairo would fix this again because you could cache an RGBA surface instead of a
> pixmap. (Actually the RGBA surface would be an XRender pixmap.)

Hm, using an RGBA pixmap should work, yes.

> So it seems to me that GTK+ should move towards a cairo-based API for theme
> engines, then these problems can be fixed on the GTK+ side and we can have a
> better interface for our needs too.

True, that would be nice (though it would not fix these kind of problems for free, would it?). However, while breaking the engine API is possible in general, I don't expect it to happen very soon.

[Roc-ocallahan (roc-ocallahan)]

> Also wouldn't this need a working composition manager?

Yeah, or else the window makeup of GtkEntry would need to change.

> though it would not fix these kind of problems for free, would it?

No...

I don't expect it to happen quickly either.

> There are some 20 darwing functions, but only 12 slots left to add more
> funtions to GtkStyle.

We could have a single entry point with a struct parameter packaging all the state that's needed to draw any widget. There are some advantages to doing that from our point of view; our theme drawing API also has a single entry point for drawing, with the widget type as a parameter.

[Reed Loden (reed)]

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

[BenjaminBerg (benjamin-sipsolutions)]

OK, lets get on the topic of the entry and progress bar issues. The current situation is that engine needs to work around GTK+ limitations. I have now opened bugs about that, but I am not sure whether these can even be fixed in the 2.x release cycle because of API compatibility (especially the progress bar issue).

http://bugzilla.gnome.org/show_bug.cgi?id=513471
http://bugzilla.gnome.org/show_bug.cgi?id=513476

(I have not talked to the GTK+ devs about this suggestion, just those bugs ...)

My suggestion is that you attach a boolean with g_object_set_data, and then engine tries to read this boolean. If it is non-zero it can leave out the background clearing. My suggestion for the name of the data would be "gtk-engine-hint-transparent-bg" or something similar.
I would modify the (most popular) engines in gtk-engines to test for the data. Other popular engines probably would pick up such a workaround pretty fast I expect. (Oh, and I'll would put the workaround in the Sugar engine :-))
Please note that this will *not* work for pixmap themes. And even when GTK+ is fixed, every single one of those would need to be updated.

I do not have a better idea to work around this problem in the short term. This solution would not be noticed by other engines, and works without fixing GTK+. In the long term fixing GTK+ is of course the right thing to do.

The nice thing for me about such a workaround is that as soon as GTK+ is fixed I could just drop the engine code again right away.

[Roc-ocallahan (roc-ocallahan)]

That sounds like a good suggestion for the short term.

[Frnchfrgg (frnchfrgg)]

I added the painting of the base[NORMAL] or base[INSENSITIVE] color behind text entries to correct the rendering in bug 415494; if we want to have the possibility to avoid drawing this background, we want to get info from the theme that it doesn't break if the background is not filled. The main problem is that engines currenlty don't expect the widget to be filled with the default background, but with the current entry background. Apart from clearlooks which always paint the background as part of the shadow, every other gtk-engine breaks if this filling hasn't happened. On a dark background it means a bug more visible that just not rounded corners.

[BenjaminBerg (benjamin-sipsolutions)]

Darn, that means that my first idea does not work out. Here just some random ways of doing it:

 1. Subclass GtkEntry (and other widgets if needed) and add a style property specifically for mozilla
 2. Let the engine attach data to the style
 3. Wait for something in GTK+

Not sure what would be best right now. It seems to me that more complex workarounds are too intrusive to be feasable.

[Conn O Griofa (psyke83)]

Ubuntu release: Hardy (fresh install from Alpha5 desktop cd, all updates as of February 26th)
System: Dell Inspiron 510m laptop
Graphics subsystem: VGA compatible controller: Intel Corporation 82852/855GM Integrated Graphics Device (rev 02)

Rounded GTK+ buttons are rendered with a rectangular white outline, most noticeable in the new Firefox 3.0 Beta 3 release when browsing Gmail (not not limited to this site). I am attaching two small screenshots; one of the "Archive" button while using the "Ubuntulooks" theme, and the same button while using "Clearlooks". You may need to zoom the images as I cropped them in order to avoid personal information being exposed.

Steps to reproduce:
1. Ensure Ubuntulooks theme is active
2. Open Firefox 3, navigate to http://mail.google.com and log in to your account
3. Observe the "Archive" and "Report spam" buttons against the blue background.
4. Change to any other theme installed by default in Ubuntu, and repeat steps 2-3.

Expected results: Buttons have no noticeable outline, or else a thin white outline, rounded at the edges for rounded buttons.

Actual results:
1. For Ubuntulooks theme: Buttons have a white rectangular outline, creating noticeable white marks at the edges of rounded buttons.
2. For all other themes installed by default: results are as expected (above)

Note: This is most likely not a Firefox 3 or core GTK+ bug, as all other themes besides Ubuntulooks work as expected.

[Conn O Griofa (psyke83)]

Update:

Since gtk2-engines-murrine is now installed by default in Hardy, I can confirm that the bug is present with this engine too.

[Andrea Cimitan (cimi)]

I have no idea why sometimes works and sometimes not.
I CC myself to this bug.

Also svn release seems affected.

[Andrea Cimitan (cimi)]

Fixed in murrine, next release will ship that fix.
http://svn.gnome.org/viewvc/murrine?view=revision&revision=24

[Murat Gunes (mgunes)]

The issue is also present with Clearlooks.

[Andrea Cimitan (cimi)]

no Murat, the issue with clearlooks is different, and can be fixed in another way, but I won't fix it for many reasons.
In any case, this is another bug

[Conn O Griofa (psyke83)]

Since hardy is at feature freeze, I'm not sure if murrine will get updated. If it doesn't, this is the patch necessary to apply against hardy's gtk2-engines-murrine 0.53.1-1ubuntu1 source. Many thanks for this, Cimi!

[Murat Gunes (mgunes)]

Andrea, thanks for the feedback; I'm closing the clearlooks task, but I'll file a separate bug in the clearlooks source package, since the issue persists, regardless of whether it will be fixed at this point. Could you cite the rationale there?

[Conn O Griofa (psyke83)]

...And here's the patch for ubuntulooks. Can the maintainer please check this patch, and include it (as long as it's ok)? On my system rounded buttons are now displaying properly after applying this patch to the source.

[Murat Gunes (mgunes)]

Conn, you can follow the process listed at https://wiki.ubuntu.com/FreezeExceptionProcess to request an exception. If the decision is to ship Murrine by default, the fix should go in.

[Conn O Griofa (psyke83)]

Murat, thanks for the information. I'll file a request, but there's still work to be done.

These patches fix buttons, but text input boxes appear to still exhibit the problem in Firefox 3. I'm investigating.

[Conn O Griofa (psyke83)]

The problem now seems to be with GtkEntry widgets. Observe the following cropped image from slashdot.org's login pane, taken while using murrine compiled with the patch I posted earlier. As you can see, "Nickname" and "Password" exhibit the same problem, but the "Log in" button does not (thanks to the patch).

I'm not certain what needs to be fixed, but *all* theme engines I have tried have this problem with GtkEntry boxes - Clearlooks, Ubuntulooks, Nodoka, Aurora... so perhaps it's a problem with Firefox. Therefore, I'm adding Firefox to the list of affected packages so it can get attention by the Mozilla folks.

From the duplicate here on launchpad, here's the upstream bug: https://bugzilla.mozilla.org/show_bug.cgi?id=329846#c45

Also, in the ubuntulooks source I noticed this, can it be related? I commented this section out, and the surrounding of GtkEntry boxes got corrupt, and active tabs became black...

File: ubuntulooks-0.9.12/engine/src/ubuntulooks_style.c, line 79

 /* I want to avoid to have to do this. I need it for GtkEntry, unless I
    find out why it doesn't behave the way I expect it to. */
 if (widget)
  ubuntulooks_get_parent_bg (widget, &params->parentbg);

[Andrea Cimitan (cimi)]

The GtkEntry is drawn in that way due to draw_gtkentry, it fills the background...

I'm not sure if that code can be removed.

[Conn O Griofa (psyke83)]

We should keep an eye on this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=405421

[Bruce Cowan (bruce89-deactivatedaccount)]

This is a duplicate of #180962.

[Conn O Griofa (psyke83)]

I'm currently creating an alternative theme for consideration in Hardy, but I want to bring your attention to a crude workaround I've included. See: http://ubuntuforums.org/showthread.php?t=715530

Basically, for GtkEntry and GtkCombo* boxes, I set the roundness to 0, in order to minimize the appearance of the ugly outline at the edges. We need a better fix.

[Bruce Cowan (bruce89-deactivatedaccount)]

I give in.

[Murat Gunes (mgunes)]

Strangely, I'm experiencing this with Murrine SVN on Hardy as well.

[Murat Gunes (mgunes)]

Cimi, could you explain why you won't fix it in Clearlooks? The outcome of the upstream bug report seems to suggest that this should be fixed in the theme engines.

[Ventnor-bugzilla (ventnor-bugzilla)]

Created attachment 332094
Patch 2

If this approach is good, then I'll take this bug and address the name change. I also added more descriptive comments about why we need this.

Since moz_gtk_entry_paint is abstract and caters for many different entry-type widgets, I found it saves a lot of code by setting the transparent-bg-hint thing everytime the widget is drawn, and I don't think the performance saving of making this only happen during theme change is enough to justify the code it would need. Setting a boolean on a GObject should be a pretty fast op anyway.

[Frnchfrgg (frnchfrgg)]

>diff -r d47ed12b70fc widget/src/gtk2/gtk2drawing.c
>--- a/widget/src/gtk2/gtk2drawing.c Sun Aug 03 00:41:32 2008 +0200
>+++ b/widget/src/gtk2/gtk2drawing.c Sun Aug 03 13:17:51 2008 +1000
>@@ -533,6 +533,8 @@ ensure_progress_widget()
> if (!gProgressWidget) {
> gProgressWidget = gtk_progress_bar_new();
> setup_widget_prototype(gProgressWidget);
>+ g_object_set_data(G_OBJECT(gProgressWidget),
>+ "transparent-bg-hint", TRUE);

Why do you set this data on the progress widget ?
And I don't agree that the tradeoff is good when setting the boolean at paint time. And even at paint time, it could/should be set unconditionnally, since it means "I obey to the transparent bg style property".

I would prefer that the boolean would be set to true at ensure() time, which means there would be 3 or 4 calls instead of one, granted, but also means it would be called 3 or 4 times, not each time we draw. The ensure() functions are called again when the objects have changed under us anyway, because the pointers are set to NULL at this time (it was needed for us not crashing at theme change).

See also comment below for naming.

>+ gtk_widget_class_install_style_property(entry_class,
>+ g_param_spec_boolean("honors-transparent-bg-hint",

You got the naming wrong, the style property is a mean for the theme to ask Gecko to not draw the bg, while the data set on the widget means that we obeyed the hint (currently) or that we can obey the hint (what I propose). My understanding is consistent with the description of the property:

>+ "Transparent BG enabling flag",
>+ "If TRUE, do not fill the background of widgets, so effects like rounded corners are done correctly.",

[BenjaminBerg (benjamin-sipsolutions)]

_FrnchFrgg_, about the progress bar. We *need* the same hint for the engine here too, as the progress bar has a similar problem.

"honors-transparent-bg-hint" was the style property name that roc suggested.

[Frnchfrgg (frnchfrgg)]

I don't want to speak for roc, but he may have confused the "style property" and the "widget property" when typing, at least, that's how I read it.

Anyway, "honors-transparent-bg-hint" makes no sense if it's the style property, which is used by the theme to ask gecko to ommit the background painting. On the contrary, it is a good name for the widget property, used by gecko to tell the theme it honored (or it can honor) the transparent bg hint.

As for the progressbar, you cannot just set the "I honor the style property" to true, if you don't really honor it, which means it has been added also to the progress bar class, and we effectively obey it.

Which brings be to the point that we probably should create a style property for a generic widget, if that's possible, and check for it every time we fill a background ourselves before calling the engine. Then we could set the "honors-transparent-bg-hint" on every of our widgets in setup_widget_prototype (and manually when we don't use it).

roc, what do you think about that ?

One last remark, I'd like to have a review by Benjamin, and a review by me before the superreview from Robert. Alternatively, I takeover the bug since I have ideas for it, and then ask for r to Benjamin and Michael, and sr from roc.

[BenjaminBerg (benjamin-sipsolutions)]

(In reply to comment #41)
> I don't want to speak for roc, but he may have confused the "style property"
> and the "widget property" when typing, at least, that's how I read it.

I understood the honor as in: "I, the theme, hereby tell you that the engine 'honors' the 'transparent-bg-hint'". With this knowledge gecko does not fill the background and everyone is happy.

The hint from gecko the engine means:
"You are drawing directly on top of the canvas. There is no need to initilize the background or undo any filling done by GTK+/gecko."

[Roc-ocallahan (roc-ocallahan)]

Comment #42 is what I was thinking.

[Ventnor-bugzilla (ventnor-bugzilla)]

FrnchFrgg, if you believe you have time to take over this bug, please feel free. I still don't really understand what approach everyone wants to take, but I can still do a code review :-)

[Roc-ocallahan (roc-ocallahan)]

+ "Transparent BG enabling flag",
+ "If TRUE, do not fill the background of widgets, so effects like rounded corners are done correctly.",

This text should be updated to reflect comment #42.

+ g_object_set_data(G_OBJECT(widget), "transparent-bg-hint", TRUE);

I think we should do this unconditionally. The style property check should only be used to control whether we call gdk_draw_rectangle to draw the background in Gecko.

[Frnchfrgg (frnchfrgg)]

So the style propery would mean "I can manage if you don't draw the background" and the widget data would mean "I won't paint the background if you can manage it". So Gecko asks for a transparent bg, and the theme tells it can handle that, for the naming I proposed it's the other way around. Either way suits me.

But I'm not really sure that the names are self-explanatory enough. Perhaps a "can-handle-transparent-background" ? The good thing is that naming doesn't affect the logic of the patch.

I'll take the bug, and write a patch along the lines of comment #41, modulo the naming which I will take from comment #42.

If that doesn't suit you, speak or be silent forever :)

[BenjaminBerg (benjamin-sipsolutions)]

(In reply to comment #45)
> + g_object_set_data(G_OBJECT(widget), "transparent-bg-hint", TRUE);
>
> I think we should do this unconditionally. The style property check should only
> be used to control whether we call gdk_draw_rectangle to draw the background in
> Gecko.

I disagree here, the "transparent-bg-hint" should only be set if the engine is drawing directly on the canvas. This is not the case if Gecko fills in the background.

(In reply to comment #46)
> I'll take the bug, and write a patch along the lines of comment #41, modulo the
> naming which I will take from comment #42.

I am not sure I understood everything from comment #41 :-)

However, I do not see any advantage in registering the style property on GtkWidget (so that it exists on all widgets). AFAIK a style property is only needed for GtkEntry.

[Frnchfrgg (frnchfrgg)]

> I disagree here, the "transparent-bg-hint" should only be set if the engine is
> drawing directly on the canvas. This is not the case if Gecko fills in the
> background.

The engine can test if the style property is present. Whatever seems to be the most sane I will do.

> (In reply to comment #46)
> > I'll take the bug, and write a patch along the lines of comment #41, modulo the
> > naming which I will take from comment #42.
>
> I am not sure I understood everything from comment #41 :-)
>
> However, I do not see any advantage in registering the style property on
> GtkWidget (so that it exists on all widgets). AFAIK a style property is only
> needed for GtkEntry.

Apart from the naming part, comment 41 was telling that we shouldn't set the data on the widget if we aren't sure that we effectively can draw directly on the canvas. And that we shouldn't set this data only on entries or progress bars, but on every widget we can draw (while ensuring that the data isn't lying). If progressbar rendering is already "transparent-safe", good (but I hope that no theme relies on the progressbar being opaque).

As for registering the style property on GtkWidget, if it's absolutely sure that we won't need it on other widgets, and that it's not a problem if a theme tries to set the property when it's non-existent, no problem with me.

[BenjaminBerg (benjamin-sipsolutions)]

(In reply to comment #48)
> The engine can test if the style property is present. Whatever seems to be the
> most sane I will do.

It is a pain to test the style property in the engine. This is because I first need to check whether it does exist (or else all GTK+ applications spew thousands of warnings).

> > (In reply to comment #46)
> Apart from the naming part, comment 41 was telling that we shouldn't set the
> data on the widget if we aren't sure that we effectively can draw directly on
> the canvas. And that we shouldn't set this data only on entries or progress
> bars, but on every widget we can draw (while ensuring that the data isn't
> lying). If progressbar rendering is already "transparent-safe", good (but I
> hope that no theme relies on the progressbar being opaque).

The engine needs the hint to know that it does not need to fill in the background. It should only be read by engines for for this one purpose in my oppinion.

> As for registering the style property on GtkWidget, if it's absolutely sure
> that we won't need it on other widgets, and that it's not a problem if a theme
> tries to set the property when it's non-existent, no problem with me.

Any style property that is set but does not exist is silently ignored.

[Frnchfrgg (frnchfrgg)]

> The engine needs the hint to know that it does not need to fill in the
> background. It should only be read by engines for for this one purpose in my
> oppinion.

I was only saying that the hint to the engine that it doesn't need to cover the already painted background mustn't be lying. And that I hope that no themes rely on the progressbar being a non transparent widget, or else we would need to prepaint the base color (unless we know by the hint that it isn't needed).

Question: Should we prepaint the background with the expected bg color for all widgets, since I think that widgets in GTK aren't transparent ? This would mean probably a prepainting done for every widget, before the calls to separate painting functions, and of course subject to the style property test. Which would mean that we would create the style property on GtkWidgetClass.

The real question is probably: currently we special-cased the text entry because I found real engines which were broken by us not pre-painting the background. But it seems too narrow, and indeed we are not sure at all that it is the only widget requiring this pre-paint. Was this special casing an error, and would it be saner to just simulate the non-transparency of gtk widgets (unless the engine tells us it can cope with non-prefilled widgets) ?

roc ?

[Roc-ocallahan (roc-ocallahan)]

I'm getting confused by these extra comments. Comment #42 is completely clear to me and exactly what I want.

(In reply to comment #47)
> I disagree here, the "transparent-bg-hint" should only be set if the engine is
> drawing directly on the canvas. This is not the case if Gecko fills in the
> background.

OK, I accept that.

Let's restrict this bug to text entries for now. Extending the approach to all widgets would have a performance cost. Let's only do it if there's a clear need (or if GTK theme people tell us it's the right thing to do).

[BenjaminBerg (benjamin-sipsolutions)]

Hrm, sorry if I helped generating some confusion. (Though I also got confused by some comments :-))

(In reply to comment #50)
> I was only saying that the hint to the engine that it doesn't need to cover the
> already painted background mustn't be lying. And that I hope that no themes
> rely on the progressbar being a non transparent widget, or else we would need
> to prepaint the base color (unless we know by the hint that it isn't needed).

Agreed. I don't think any theme is relying on the progressbar to be filled or anything.

> The real question is probably: currently we special-cased the text entry
> because I found real engines which were broken by us not pre-painting the
> background. But it seems too narrow, and indeed we are not sure at all that it
> is the only widget requiring this pre-paint. Was this special casing an error,
> and would it be saner to just simulate the non-transparency of gtk widgets
> (unless the engine tells us it can cope with non-prefilled widgets) ?

The special case for the entry is completely correct. And most GTK+ widgets are transparent.

[Roc-ocallahan (roc-ocallahan)]

If someone modifies "Patch 2" so that the description text in the call to gtk_widget_class_install_style_property is more accurate, I'm ready to r+ the patch.

[Ventnor-bugzilla (ventnor-bugzilla)]

Should "transparent-bg-hint" be unconditionally set to true in the ensure functions? If it is ignored anyway by themes that don't honour it, it shouldn't really matter right? (This has probably been suggested before)

[Roc-ocallahan (roc-ocallahan)]

No, see comment #47.

[Frnchfrgg (frnchfrgg)]

We shouldn't settle for patch 2, IMHO.
We should set the data to true on every widget we keep, (with setup_widget_prototype and the like), because we want to advertise that all our widgets are transparent (apart from the entry which will be only if the style property is set, as it is in patch 2).

As for my other questions, I agree that it would be too much of a performance cost to prepaint every widget, especially since we don't know any real life engine bitten by this transparency. I had to raise the problem, but Benjamins answer is reassuring.

I'd go with this approach: we advertise that we paint directly on the canvas for every widget but the entry (ultra cheap, and most engines would probably ignore the info), and the entry is transparent only if we know the engine can cope with it.

I'll write the patch soon (like in less than 24 hours).

[Frnchfrgg (frnchfrgg)]

Created attachment 332569
Patch v3

Essentially this patch is the same as patch 2, but with the comments adapted to match what was decided, and the indication that we don't pre-fill for each widget we may pass to the theme engine. This indication is still dynamically updated for the entry.

What's missing is that there is another widget for which we prefill, in *_tab_paint(). We currently need to prepaint behind the "gap", because in some engines the box_gap() is transparent. We probably need to give our notebook a similar treatement than our entries.

I didn't spot any other widget for which we prepaint anything.

Benjamin, what do you think ?

[Ventnor-bugzilla (ventnor-bugzilla)]

Comment on attachment 332569
Patch v3

Code-wise this is fine by me. I'll leave the decisions on the overall approach to roc/benjamin.

[Roc-ocallahan (roc-ocallahan)]

Comment on attachment 332569
Patch v3

I'm OK with this if Benjamin is.

[Frnchfrgg (frnchfrgg)]

I just noticed that Network Managed is bitten by this very problem: they put progress bars into a menu, and when the menu is selected, the rounded corners are bright visible, since they stay at the unselected color.

[BenjaminBerg (benjamin-sipsolutions)]

Sorry that I did not think about this earlier. But can moz_gtk_init be called multiple times (eg. on a theme change)? If yes, then we should check for the style property before registering, or a warning will be printed by GTK+.
(All that would be needed is a call to gtk_widget_class_find_style_property.)

There is one small thing, that I want to say, but feel free to ignore me if you think differently ;-) (ie. you get r+ anyways)
You are setting the hint on all widgets. I do see some advantages, but I also see a disadvantage. And that is that it may be abused by engine writers to special case gecko, instead of using it only in the way that it is intended.

So r+ from me, but please check the moz_gtk_init thing before landing.

[BenjaminBerg (benjamin-sipsolutions)]

Oh, I am not able to modify the attachment status, so I cannot give review+ there :-)

[Reed Loden (reed)]

(In reply to comment #62)
> Oh, I am not able to modify the attachment status, so I cannot give review+
> there :-)

You can now. :)

[Frnchfrgg (frnchfrgg)]

moz_gtk_init isn't called again on theme change.

Engine writers can special-case gecko, sure, but they can also with the
"MozillaGtkWidget" name, so I don't really see why they would choose the bad
way over the good. And I hope that when some program decides to paint a widget
itself (for example NetworkManager may want to do that to get a good look on
hover), it can use the same hint; so this hint won't stay a good indicator that
it's gecko.

It's strange that you can't even grant a review you've been asked for...

[BenjaminBerg (benjamin-sipsolutions)]

Comment on attachment 332569
Patch v3

OK, so here we go.

Yeah, the same hint should also be used by other applications :-)

[BenjaminBerg (benjamin-sipsolutions)]

In case anyone is interested, the following gtk-engines bugs are about the whole issue:

http://bugzilla.gnome.org/show_bug.cgi?id=546839
 Implement the workaround that is part of this bug (so that I don't forget)

and:
http://bugzilla.gnome.org/show_bug.cgi?id=475293
 Is a clearlooks bug about making widgets handle a transparent background better.

[Dao (dao)]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/bb2bd5fc3efc

[Alfredkayser (alfredkayser)]

Could it be that this patch has caused bug 452385 ?
Bookmark This Page panel hangs Firefox when -moz-border-radius is used.

[Frnchfrgg (frnchfrgg)]

I'd say it has nothing to do with it, but I'll test when I have time. Feel free to try with a custom build with and without this patch.

[BenjaminBerg (benjamin-sipsolutions)]

It seems rather impossible to me that the patch here creates any problems. Also if the description of bug 452385 is correct, then the bug is on Windows XO, and I do not think anyone is using GTK+ on there :-).

[Alfredkayser (alfredkayser)]

Ah, I didn't see that this bug is for GTK.

[Alexander Sack (asac)]

Any risk taking this on the 1.9.0 branch?

[Ventnor-bugzilla (ventnor-bugzilla)]

The question is whether GNOME did their bit for the 2.24 release.

If so, taking this patch shouldn't be of any risk.
If not, there's no point since 3.1 will be out before the next GNOME release anyway.

[BenjaminBerg (benjamin-sipsolutions)]

The entry workaround is in gtk-engines 2.16.0 (which is part of GNOME 2.24).

Some widgets still have a broken shadow, but the GNOME 2.24 does use the feature that is introduced by this bug.
(ie. http://bugzilla.gnome.org/show_bug.cgi?id=475293 is still open)

[Ventnor-bugzilla (ventnor-bugzilla)]

Comment on attachment 332569
Patch v3

Very well then, let's give it a go.

[Reed Loden (reed)]

dveditz: It would be nice if you would give a reason for minusing an approval flag...

[Dveditz (dveditz)]

Sorry, don't know why I didn't. "Doesn't meet new branch approval criteria -- not a security fix, topcrash, or fix for a regression from one of those". Minor polish fixes can go into the next release, and from comment 73 it wasn't clear this would have any effect anyway.