Closed Bug 405421 Opened 17 years ago Closed 16 years ago

Ensure our widget painting is transparent for themes that support it

Categories

(Core :: Widget: Gtk, defect)

Product:
Core
Component:
Widget: Gtk
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.1a2

People

(Reporter: micmon, Assigned: frnchfrgg)

References

Details

Attachments

(2 files, 3 obsolete files)

Screenshots
9.83 KB, image/png
Details
Patch v3
14.13 KB, patch
benjamin
: review+
ventnor.bugzilla
: review+
roc
: review+
dveditz
: approval1.9.0.4-
Details | Diff | Splinter Review 
The native look of widgets is great, but if the GTK theme uses rounded corners, there are problems. Basicly, the area around the widgets (and the rounded area) are drawn with a white background.

For most web sites, this does not really show as the background is white, too. For pages with darker background, widgets look a bit odd. 

This also happens inside of Firefox for example, if the widget is selected. For example, the progress bar in the download manager shows the same problem when the download is selected.
Attached image Screenshots — 
Various screenshots of the problem. For websites, you can take http://www.schierer.org/~luke/log/ as a testcase (see the login fields).
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.
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...
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.
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 ;)
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.
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.)
Oh, the checkbox thing can be considered a theme bug. Alpha blending is perfectly possible in this case.
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.
(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.
(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 to pass the cairo context trough to the engine and handle drawing to the X window correctly. (Which I imagine would get really nasty if both cairo and gdk API are mixed.)

Another way, working around the restrictions in GDK might be to attach the cairo context to the passed in GdkDrawable. However that seems like a pretty nasty hack to me. (The engine would draw on the cairo context, and somehow hint that it did.)
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.
(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.
> 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.
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.
That sounds like a good suggestion for the short term.
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.
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.
I know that entries are broken, etc. But bug 387036 suggests that buttons are broken, and I don't see any reason for them to be. They can be fully transparent in normal GTK+, so either the theme or mozilla is doing something wrong there.
The only broken buttons are those in the top and bottom of the scrollbar.
Would a patch to add a style property be acceptable? This sounds like the sanest option to me right now.

The largest chunk of code would be the code required to create the subclasses (I guess that we will need one for GtkEntry, and another one for GtkProgressBar.) The style property could just be called "transparent-bg-workaround" or something similar.
I think a part of the code already provides an opportunity for workarounds. Every single form widget that Mozilla uses is a child of a window with the name "MozillaGtkWidget". I'm wondering if that would be enough to allow themers to provide workarounds.
Michael, yes this does help. However it does *not* help at least in the GtkEntry case.

However, please read comment #18 (which references bug #415494) carefully. Mozilla itself is filling the whole are with a base[NORMAL] background. Which means that there is no way of having transparency.
(It is correct that doing this is much closer to what GTK+ does.)

So basically you have two choices:
 1. Ignore bug #415494 and do not fill in the background. Then a good way to figure this out from the engine. (The MozillaGtkWidget may be enough.)
 2. Add a style property (or similar) that will cause both Mozilla *and* the engine/theme not to fill in the background.

In the end, neither Mozilla nor the theme/engine is allowed to fill the background.

A style property can easily be checked in both the engines and mozillas drawing code. (Will need a change in many themes, but it is one line, and does not harm if a hack like this is not needed anymore.)
This patch adds a style property to GtkEntry and GtkProgressBar. Together with this patch and some changes in the sugar theme and engine, I have gotten entries to work correctly.

Note: This patch should not go in as is. It would be better to not add a style property to standard GTK+ widgets, but instead to subclass them. I was just not sure where to place the code to create the new class. It will also require some hacks for the comboboxentry.
OK. This uses a different approach. Instead of just using a style property, it uses a style property, and then attaches data to the widget to hint the engine that the background is transparent.

The approach means:
 1. The check inside the engine is trivial. Webkit can also easily use the same hint to the engine. (Checking the style property is a bit more complicated.)
 2. The style property toggles how well Mozilla emulates GTK+, so that the background can be transparent.
 3. The style property can be attached to the normal GtkEntry. There is no need to subclass GtkEntry. Subclassing GtkEntry would create a pain for the combobox as we would need to replace the normal GtkEntry with the custom subclass.
 4. Pixmap based themes can use special match lines to achieve the same thing, as they are not able to read out the hint from mozilla. (Though special cases for each of mozilla, webkit and whatever else may be needed in this case.)
I like this idea, but why the need to hint the engine that the background is transparent ? Why not make the style property mean "engine doesn't need background pre-filling", and then the engine would just paint whatever is needed to get correct rendering without assuming the background color is the base_gc one... I mean, the engine knows that it may encounter non filled backgrounds, since it set the style property.

OK, somebody could probably set the style property in the theme gtkrc or something, with an engine that doesn't support it, but hey, people doing stupid things shouldn't be rewarded...
Please note that the engine does *not* set the style property. The theme does.

First of all the patch does not subclass GtkEntry, so the engine needs to know that it is drawing an entry from mozilla, and not a normal one.
IE. depending on how the theme sets the style property, doing so will affect all GtkEntries. However, a normal GtkEntry will always need the background filled by the engine. So just getting the style properties value is not enough.

The nice thing about attaching a bit of data is that it is trivial to check in the engine. Also the same mechanism works fine for the progress bar.
(It is even easier then testing a style property as one needs to check if the style property exists first.)

(I don't care about the case where the style property is set and the engine does not support it.)
Yes, the theme sets the style property, but a theme setting this property and using an engine which gives garbled output when the bg isn't prefilled deserves to look broken in Firefox, IHMO.

And a normal GtkEntry doesn't need the background filled by the engine, it is the other way around: a normal GtkEntry fills the background (ok it's not explicitely filled, it's the inner widget default bg) before calling the engine... Some engines, like thinice, expect that and look broken if it isn't done, whereas some others (like Clearlooks) don't, since they fill with the base color the parts they need to be filled, without caring about the widget being already filled or not.

But I understand what you want to say: when there is a normal GtkEntry, themes with rounded corners need to draw the dialog color onto the base color to simulate rounded corners, and that part can be skipped if the engine knows that the bg hasn't really been filled... So then (and only then), the engine doesn't draw those pixels (or doesn't fill the widget with the dialog color), and thus can have really transparent rounded corners.

In short: an engine can hint a GtkEntry painter (Gecko/safari or vanilla GTK, I don't know of others, but why not) that it can cope with, and would prefer non-prefilled GtkEntries, but it needs to know if the hint has been obeyed to know if it needs to "pseudo-undo" the prefilling to get round corners.

Am I right ?
Yes, that is correct.
Attachment #331151 - Flags: review?(roc)
Instead of filled-bg-workaround, which I find rather vague, would it make more sense to call it honors-transparent-bg-hint?
Hm, I am not sure about honors-transparent-bg-hint. A pixmap theme is not able to read out the hint, but could still set the style property and get better theming by special casing on the "MozillaGtkWidget" name.
No, I mean it would still be a style property, but just change the name to honors-transparent-bg-hint (which would be parsed as honors-(transparent-bg-hint))
Also, I don't think it's usefull to re-set each time the info that we'll do transparent bg, if we call it "can-do-transparent-bg" or "honnors-transparent-bg-hint". Just set this data on the widget at ensure_*() time.
OK, I am fine with the style property name change.

FrnchFrgg, I decided to reset the name every time in case of a theme change. So if there is some mechanism that ensures that a theme change is picked up then that is probably better.
Attached patch Patch 2 (obsolete) — — Splinter Review 
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.
Assignee: nobody → ventnor.bugzilla
Status: NEW → ASSIGNED
Attachment #332094 - Flags: superreview?(roc)
Attachment #332094 - Flags: review?(roc)
Attachment #331151 - Flags: review?(roc)
>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.",
Attachment #332094 - Flags: superreview?(roc)
Attachment #332094 - Flags: review?(roc)
Attachment #332094 - Flags: review-
_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.
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.
(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."
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 :-)
+                                                                 "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.
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 :)
Assignee: ventnor.bugzilla → frnchfrgg
Status: ASSIGNED → NEW
(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.
> 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.
(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.
> 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 ?
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).
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.
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.
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)
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).
Attached patch Patch v3 — — Splinter Review 
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 ?
Attachment #331118 - Attachment is obsolete: true
Attachment #331151 - Attachment is obsolete: true
Attachment #332094 - Attachment is obsolete: true
Attachment #332569 - Flags: review?(benjamin)
Attachment #332569 - Flags: review?(ventnor.bugzilla)
Comment on attachment 332569 [details] [diff] [review]
Patch v3

Code-wise this is fine by me. I'll leave the decisions on the overall approach to roc/benjamin.
Attachment #332569 - Flags: review?(ventnor.bugzilla) → review+
Comment on attachment 332569 [details] [diff] [review]
Patch v3

I'm OK with this if Benjamin is.
Attachment #332569 - Flags: review+
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.
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.
Oh, I am not able to modify the attachment status, so I cannot give review+ there :-)
(In reply to comment #62)
> Oh, I am not able to modify the attachment status, so I cannot give review+
> there :-)

You can now. :)
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...

Comment on attachment 332569 [details] [diff] [review]
Patch v3

OK, so here we go.

Yeah, the same hint should also be used by other applications :-)
Attachment #332569 - Flags: review?(benjamin) → review+
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.
Keywords: checkin-needed
http://hg.mozilla.org/index.cgi/mozilla-central/rev/bb2bd5fc3efc
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Summary: Widgets with rounded corners on dark background → Ensure our widget painting is transparent for themes that support it
Target Milestone: --- → mozilla1.9.1a2
Could it be that this patch has caused bug 452385 ?
Bookmark This Page panel hangs Firefox when -moz-border-radius is used.
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.
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 :-).
Ah, I didn't see that this bug is for GTK.
Any risk taking this on the 1.9.0 branch?
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.
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)
Comment on attachment 332569 [details] [diff] [review]
Patch v3

Very well then, let's give it a go.
Attachment #332569 - Flags: approval1.9.0.3?
Attachment #332569 - Flags: approval1.9.0.3? → approval1.9.0.4?
Attachment #332569 - Flags: approval1.9.0.4? → approval1.9.0.4-
dveditz: It would be nice if you would give a reason for minusing an approval flag...
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: