Closed Bug 142334 Opened 23 years ago Closed 22 years ago

Turn on nsNativeThemeGTK

Categories

(SeaMonkey :: Themes, defect)

Product:
SeaMonkey
Component:
Themes
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.1beta

People

(Reporter: bryner, Assigned: bryner)

References

Details

Attachments

(7 files)

the obvious patch
408 bytes, patch
blizzard
: review+
hyatt
: superreview+
Details | Diff | Splinter Review
Mandrake theme
10.48 KB, text/plain
Details
proposed fix
3.60 KB, patch
blizzard
: review+
bryner
: superreview+
Details | Diff | Splinter Review
Eazel aka Crux aka Mandrake GTK engine
165.96 KB, application/octet-stream
Details
fix for the checkbox problem, for reference
633 bytes, patch
Details | Diff | Splinter Review
fix comment style for plain old C
571 bytes, patch
Details | Diff | Splinter Review
vert vs. hori scroll bars
83.64 KB, image/png
Details
Tracking bug for turning on the GTK native theme implementation by default.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1beta
Out of interest, what are considered to be the requirements before this can happen? Obviously the patch to *do* it is trivial, and there's nothing explicitly marked as a dependency of this bug. Just curious so that I can get some idea of how soon this is likely to happen...
I do not know if this is related or not, I built yesterday's checkout of mozilla with NS_NATIVE_THEME defined and now using TestGtkEmbedNotebook or SkipStone (in tabbed mode) exhibits a weird behavior, if you switch between tabs, the page does not redraw and becomes grey - You can no longer view or load any page until you open a new tab. Same happens if you switch again.
There are a few things I'd like to see fixed before this is turned on. The most severe is that we will just crash with some theme engines. This happens if the theme engine assumes that there is a valid GdkWindow for the widget that it can manipulate (we are just passing in a pixmap since the drawing is done offscreen). Aside from that, some themes still render incorrectly, which just looks bad.
For the record, comment #2 is referring to bug 139294.
Attached patch the obvious patch — — Splinter Review
Comment on attachment 95929 [details] [diff] [review] the obvious patch r=blizzard
Attachment #95929 - Flags: review+
Just noting that as well as the trivial patch, I need r and sr to apply to all of the new code, most of which has not been reviewed: gtkdrawing.* nsNativeThemeGTK.*
Attachment #95929 - Flags: superreview+
Comment on attachment 95929 [details] [diff] [review] the obvious patch sr=hyatt
My review applies to those files as well.
> The most severe is that we will just crash with some theme engines. Did you try to fix this? I ask because, since this was turned on, my trunk builds crash with an X BadWindow when I click on any button in the profile manager. (If I select a profile from the command line, they crash trying to draw the browser window.)
PS, this is an out of the box Mandrake 8.2 install, so I imagine I'm not the only one experiencing this.
roc- what gtk theme does this use?
Attached file Mandrake theme —
This is the Mandrake theme. My .gtkrc is just this: # -- THEME AUTO-WRITTEN DO NOT EDIT include "/usr/share/themes/Mandrake/gtk/gtkrc" include "/home/roc/.gtkrc.mine" # -- THEME AUTO-WRITTEN DO NOT EDIT
FYI, "Mandrake" theme is a derivated of "Crux" theme, also called "Eazel engine" theme. This theme is available from GNOME CVS in eazel-themes module. If you want a tarball for this theme, just grab mandrake_desk SRPM from a Mandrake mirror. If you want to binary RPM, download mdk-eazel-engine package.
More datapoints: Locally commenting out NATIVE_THEME_SUPPORT in gfx/src/gtk/Makefile.in "fixed" the viewer crash on myotonic. I suspect it would clear up the other tinderboxes (palermo, balsa & worms, altoid) as well. Before I did the local back out, I changed the tinderbox display to my laptop (using X-win32) instead of costarica (using Irix X11R6). When I did that, the viewer test ran fine but bloattest timed out (due to the remote display over dsl). So the problem appears to be partially X server dependent. The lcltbld account on myotonic, which is a RH62 box btw, has no .gtkrc.
Actually, worms & balsa are hitting a separate static build issue. Ignore them for the moment.
Is there a way to turn the native themes support OFF at runtime ? On 8bit displays the Zilla now looks like a ugly patchwork.
confirmed crash with Crux GTK theme engine on startup :) 2002082521/Linux.
Error message:- Gdk-ERROR **: BadWindow (invalid Window parameter) serial 1478 error_code 3 request_code 61 minor_code 0 P.S. the theme is a fresh install from debian package, it shouldn't be the problem of the theme itself.
You don't need a broken theme to hit a Gdk-ERROR. Having a gfx card with multiple visuals+different depths is enougth to hit this issue everytimes at startup (this is now known as _smoketest_blocker_ bug 164581 ('"Gdk-ERROR **: BadMatch" at startup') ...
Strange. I just pulled the Crux theme out of gnome cvs and built it and I had no problems with it. I'm trying to find another bad theme.
I chatted with bryner about this and I think he knows what the problem is but he's not sure how to fix it. You probably want to talk to him before trying to debug this further.
Yeah, he and I had a long conversation about it. I was going to try to fix it, but I can't reproduce it to test it. :)
I could give you an account on ocallahan.org, if that would help.
Someone pointed out a local theme that will crash. I can reproduce it now.
Any progress on this bug? This would be great to have for 1.2.
OK, I had a discussion with blizzard. Here's what I think needs to be done: we need to push/pop the GDK error handler in nsNativeThemeGTK around each call to the theme engine. After each theme engine call we check the error status to see if an error occurred. If an error did occur then we set a flag to disable that widget type, so ThemeSupportsWidget returns FALSE for that widget type forever more. Because the widget may not have been painted completely, we also need to refresh the display so the widget gets drawn unthemed. You can do this by calling GetPrimaryPresShell()->GetViewManager()->UpdateAllViews(NS_VMREFRESH_NO_SYNC).
roc and I have been discussing this and he came up with a pretty good plan: push the error handler for each of the drawing operations do the drawing pop the error handler, save error condition if error condition disable this type for the rest of the life of the app force an async redraw via: GetPrimaryPresShell(frame)->GetViewManager()->UpdateAllViews(NS_VMREFRESH_NO_SYNC) endif
I'd love to get this bug fixed. I'm getting my Linux box set up and want to impl GTK themed HTML form controls, but I wanted you guys to get it working for XUL first.
What has to be done to get HTML form controls using nsITheme? BTW, blizzard, are you doing this fix, or am I? Or is it bryner? :-)
Attached patch proposed fix — — Splinter Review
Here you go. On my Mandrake theme things work pretty well with this. Buttons, Toolbarbuttons, and Dropdownbuttons get disabled automatically. There's no crash, everything looks pretty much fine. However, there is one serious problem: checkboxes/radio buttons never show their toggled-on state. I'm not sure what could be causing that.
Hmm, edit fields get disabled in some states too. They still look OK. I have no idea what the problem could be with the checkboxes. Is it possible for the theme to do something that works on a window but fails silently on a pixmap?
Sure, anything is possible. I've found it helpful to build debug versions of the theme engines so that I can step into them.
*** Bug 171867 has been marked as a duplicate of this bug. ***
Comment on attachment 100862 [details] [diff] [review] proposed fix r=blizzard This is working great here, no problems that I can see with the busted redmond theme w/ rhat 8.0. The default theme works well.
Attachment #100862 - Flags: review+
I think that patch should definitely be checked in, because it seems like the right way to protect against unknown or buggy themes crashing the browser, but I can't recommend turning native themes on by default. My build still has problems: -- checkboxes never show checked (as I mentioned above) -- I just noticed that text in some toolbar buttons isn't appearing I am using an out-of-the-box Mandrake setup. I suspect we will get a massive flood of bug reports if we ship a release with this turned on by default. I would like to debug this further but I don't have the time, nor do I have the knowhow to put together debug builds of the theme code and GTK, which I think would be pretty necessary to make progress.
Maybe the mandrake folks can provide some insight...
Thanks Chris, I was already CC: to this bug :)) Currently, what is called "Mandrake" theme is only "Crux" engine with different default colors and values.. It is Crux engine which is somehow broken :I remember it doesn't like having widgets with width or height equals to 0 (I've fixed that in GNOME CVS, before it was crashing, now, it only outputs a warning)..
I was hoping that you might be able to build and run a copy of the engine, no matter what it's called, and let us know why it's not working properly. I don't have a copy of mandrake here and I don't have nearly enough bandwith to download and install it.
Comment on attachment 100862 [details] [diff] [review] proposed fix sr=bryner
Attachment #100862 - Flags: superreview+
I checked in roc's patch to the trunk.
Source of Crux/Eazel/Mandrake GTK1 engine which is causing native GTK theme troubles...
I've done some check using Chris patch against Mozilla 1.1 and I was able to find what is causing X Errors : when drawing rounded buttons, Eazel engine tries to clear the edge of the button area, to really have a rounded aspect.. It seems these gdk_window_clear_area calls (lines 343->347 in src/eazel-theme-draw.c) are causing the X Window errors.. If I comment these lines, Mozilla no longer fails to render the widgets (well, there are still some problems with checkboxes and radio buttons which are not drawn correctly..). This behaviour is only seen in Mozilla, which make me thing it is a problem on the mozilla side but I'm not a GTK theme expert..
Frederic, you might want to fix that in your theme so that Mozilla will work with the next Mandrake release. I don't know who else is using this kind of code in their themes, but I think that yours is one of the most public. I guess we need to figure out the checkbox issues before we turn this on by default.
I've checked GTK2 port of Crux engine (it has been done by Seth Nickell <snickell@stanford.edu>) and there is the same "trick" for widget rounding.. Since mozilla is the only application to have this kind of problem, I'm more incline to think it is a mozilla bug than a theme bug.. And again, it is not "our" theme : this engine was written by Eazel folks and we just happen to be the only one to use it (and since Eazel is dead, we are pretty stuck with it).. I've checked again the theme code and the error is caused because the window used by the theme (ie given by mozilla) is not yet mapped.. This is causing gdk_window_clear_area to fail..
Maybe unrelated, but is there any way to apply the techniques used by -moz-border-radius to this problem? Presumably that code has something that has a similar kind of effect, although it might be being called in a completely different scenario so it might not apply...
Instead of doing gdk_clear_area, perhps you could do gdk_draw_rectangle (window, bg_gc, TRUE, x, y, width, height); I think that way it will work on GdkPixmaps.
I debugged this and found that the eazel engine checks the toggle button's active state itself. Making sure it's in sync fixes the problem. I'll go ahead and check this in since it's not part of the build.
Awesome. Just one thing: gtkdrawing.c:207:3: warning: C++ style comments are not allowed in ISO C89
a=dbaron (,asa,roc,brendan,blizzard) for flipping the switch for 1.2b (I'm assuming it's uncommenting the one line in the makefile)
Is there a bug for native theme support in the GTK2 port?
things looked fine when i ran the browser smoketests (http://www.mozilla.org/quality/smoketests/index.html#browser --didn't do java or plugins, though). i fiddled with the radiobuttons and checkboxes (eg, pref panels), and those seemed fine, too. tested with the 2002-10-14-13-NATIVE_THEME build on linux rh7.2.
( For those interested I opened bug 174471 to get this ported over to gtk2. )
Linux 2002101508 - two issues you may be aware of - the first (left-most) tab in classic has no left border, and HTML form elements now look completely different than XUL ones (particularly the radio buttons in prefs).
Attached image vert vs. hori scroll bars —
One more thing. Vertical scroll bars are having the gtk theme's pixmap applied to them. Horizontal ones don't seem to. Attaching screenshot of mozilla window, with another sample GTK app on top of it, showing the difference.
> HTML form elements now look completely different than XUL ones The plan is to change the HTML form elements to look native too.
There is already bug 174629 about tabs missing the _bottom_ border. Please file a new bug if your left-border problem is different from that, and also a new bug for the horizontal scrollbar issue. I'm going to go ahead and mark this bug fixed.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
OK, filed them both. The missing left border appears to be a yet another rounding error. bug 174637 (border) and bug 174639 (scrollbar pixmap).
bryner: Is there a prefs to turn this feature "on"/"off" on demand ?
You can change your theme to something other than "Classic".
Robert O'Callahan wrote: > You can change your theme to something other than "Classic". This is not sufficient. Some people want to use "Classic" and want to have a _stable_ Mozilla. This feature is still causing crashes and seems to have regressed remote X11 performance a lot.
waah.
gisburn, stop being a dumbass. if you want a stable mozilla, use a STABLE mozilla release.
Stuart Parmenter wrote: > if you want a stable mozilla, use a STABLE mozilla release. This isn't the point. The nativetheme feature is causing lots of problems and there should be a way to turn this off (e.g. via prefs) without the need of recompiling mozilla. BTW: Are you sure that we find all nativetheme bugs until 1.2final ships ? I doubt it.
The remote X performance regression is filed as bug 174585, and that will be fixed for 1.2. Any remaining crashes are due to buggy theme engines, and I don't think it affects the default themes of any distribution. I only know of one engine that currently has problems, and the latest version of that engine fixes it (see bug 175306). I'd like to remove the theme CSS rules for unix that are discarded due to the nsITheme functionality, to reduce footprint and improve startup/new window time. Having a way to disable nsITheme means that we have to keep CSS rules to make it look right.
Brian Ryner wrote: > Any remaining crashes are due to buggy theme engines, and I don't think it > affects the default themes of any distribution. What about people without any themes installed ? Somehow people hit X errors without having any themes installed, too.
Brian Ryner wrote: > I'd like to remove the theme CSS rules for unix that are discarded due to the > nsITheme functionality, to reduce footprint and improve startup/new window > time. Having a way to disable nsITheme means that we have to keep CSS rules > to make it look right. Erm... don't we need these rules for BeOS/Photon/Qt/Xlib toolkits ?
I don't really care about those toolkits. If someone wants to fork the CSS to create a theme for those toolkits, they're welcome to.
Photon/Qt/Xlib are all considered to be dead ports. They should probably be removed from the tree. BeOS is a dead platform, but seems to at least be watched. If those people want to continue using "classic," then they need to fork the css and maintain them. If you want to step up and fork the css and maintain it, then do it. You are the only person who cares about those toolkits, so if you don't step up, don't expect anyone else to.
Is bug#175775 caused by this native theme support then?
> if you want a stable mozilla, use a STABLE mozilla release. Mozilla 1.2 will be a stable release and we don't want to ship it with unnecessary problems. And please lay off the name calling. > This feature is still causing crashes If that's true, we need to figure out what's going on and fix them if it's our fault, or I will push for this to be disabled again. Please give us the bug numbers. > Any remaining crashes are due to buggy theme engines I don't think we know that for sure until we've diagnosed the particular crashes. > I'd like to remove the theme CSS rules for unix that are discarded due to the > nsITheme functionality We won't be able to do that for a long time. We need those rules for the cases when an X error is detected and we disable GTK rendering for a control. So I'm in favour of allowing a pref here, for now. However, eventually we will disable these rules. My suggestion is that someone (gisburn?) implement nsITheme for xlib. In fact, there could be an XP implementation that any toolkit could fall back to. What would be really cool is if gfx/xlib could dynamically detect the desktop in use and plug in an appropriate native theme renderer. In particular, if it could detect and use Qt themes I think a lot of people would go for it.
> This isn't the point. The nativetheme feature is causing lots of problems and > there should be a way to turn this off (e.g. via prefs) without the need of > recompiling mozilla. What crashes? What themes? What ports? This is vague and specious.
>> Any remaining crashes are due to buggy theme engines > >I don't think we know that for sure until we've diagnosed the particular crashes. I'm speaking from the crashes I _have_ looked into, such as the Xenophilia 0.7 crash.
Robert O'Callahan wrote: > > This feature is still causing crashes > > If that's true, we need to figure out what's going on and fix them if it's our > fault, or I will push for this to be disabled again. Please give us the bug > numbers. Quick search shows this list: - bug 175306 ("mozilla 1.2b (classic theme) crashes with some GTK themes") - bug 175755 ("GTK native themes crash on Solaris 2.6"; two reporters) - bug 175989 ("Browser dies on startup (X11 related) [opcode of failed request: 70 (X_PolyFillRectangle)]") ...and I guess there a more in bugzilla. > > Any remaining crashes are due to buggy theme engines > > I don't think we know that for sure until we've diagnosed the particular > crashes. I am getting crashes on Solaris without having _any_ themes installed. I only build GDK/GTK+/libIDL from source, installed it - and that's all. > > I'd like to remove the theme CSS rules for unix that are discarded due to > > the nsITheme functionality > > We won't be able to do that for a long time. We need those rules for the cases > when an X error is detected and we disable GTK rendering for a control. So I'm > in favour of allowing a pref here, for now. A bunch of related questions: What about BiDi (required for Arabic/Hebrew; smontagu is currently looking at that) ? What about CTL (required for Thai/Hindi/Tamil etc.) ? What about X input methods (japanese/chinese/etc.; bug 175755 seems to be a problem between native theme code and input methods) ? Did anyone check whether this works properly with the native theme code turned-"on" ? > However, eventually we will disable these rules. My suggestion is that someone > (gisburn?) implement nsITheme for xlib. In fact, there could be an XP > implementation that any toolkit could fall back to. The Xlib toolkit should work without depending on any external libraries and it should work with a minimum footprint (I'd like to avoid the unneccesary ~6MB extra footprint caused by GDK/GTK+ libraries). Newer Motif2 has theme support - but that's not available in all versions - and it would be another unneccesary library I'd like to avoid. And this still does not cover QNX/Photon, Qt and BeOS toolkits. > What would be really cool is if gfx/xlib could dynamically detect the desktop > in use and plug in an appropriate native theme renderer. > In particular, if it could detect and use Qt themes > I think a lot of people would go for it. That would only work if there would be a way to detect such libraries at runtime.
Christopher Blizzard wrote: > > This isn't the point. The nativetheme feature is causing lots of problems > > and > > there should be a way to turn this off (e.g. via prefs) without the need of > > recompiling mozilla. > > What crashes? Some bugids are listed in comment #75 ... > What themes? I don't have any themes installed on my test machine. See comment #75 ... > What ports? Default GTK+ toolkit on Solaris/SPARC and SuSE Linux 6.4.
Last time I checked CTL was pretty badly broken on the trunk anyway, so I don't think there's much worry that this will make it worse... Can't say about the other issues though.
Ian 'Hixie' Hickson wrote: > Last time I checked CTL was pretty badly broken on the trunk anyway, so I > don't think there's much worry that this will make it worse... 1. CTL support is not "on" by default, you have to use --enable-ctl at build time 2. You need the matching fonts installed, otherwise CTL support won't work properly either
Bug 175989 is a dup of bug 175755. This is a bug that we can and should fix by modifying the XIM error handler. [Why are X clients set up to crash by default when an error is detected, anyway? That seems ludicrously wrong.] So, so far the only "bad theme" we have is Xenophilia 0.7. > The Xlib toolkit should work without depending on any external libraries and > it should work with a minimum footprint Maybe I wasn't clear. Here's what you do: write an nsITheme implementation that uses GFX calls to paint a default, fixed theme of your choice. This could be used by Xlib, Qt, Photon and BeOS. > That would only work if there would be a way to detect such libraries at > runtime. There is, using dlopen and friends.
> 1. CTL support is not "on" by default, you have to use --enable-ctl at build > time > 2. You need the matching fonts installed, otherwise CTL support won't work > properly either Yes, I'm not stupid, I am aware of both these facts. My comment stands.
>> What themes? > > I don't have any themes installed on my test machine. See comment #75 ... > If you've installed Gtk there's a default theme. [blizzard@dead blizzard]$ rpm -ql gtk+ | grep theme /usr/share/themes/Default /usr/share/themes/Default/gtk /usr/share/themes/Default/gtk/gtkrc
It seems that buttons are not themed by GTK when Crux theme is used... anyone?
Furthermore, there are many inconsistencies between Mozilla's GTK implementation and other GTK Apps, some inconsistencies can be observed when using themes other than the default, for example, scrollbar not highlighted when Mozilla window is focused (bug 176456), tabs does not look quite right (bug 174629), text box does not hace proper GTK focus behavior (the cursor should not blink (by default?) and it should have some focus behavior (e.g. outline) in some theme, e.g. Crux).
Crux (the Eazel theme engine, actually) has some incompatibilities which cause some widgets not to be themed. I plan on posting a list of what a theme engine needs to do to ensure Mozilla compatibility to the netscape.public.mozilla.gtk newsgroup. In the meantime, this bug isn't the right place to discuss these problems. I'd suggest taking it to the newsgroup.
blizzard wrote: > If you've installed Gtk there's a default theme. > [blizzard@dead blizzard]$ rpm -ql gtk+ | grep theme > /usr/share/themes/Default > /usr/share/themes/Default/gtk > /usr/share/themes/Default/gtk/gtkrc These files do not exist after installing GDK/GTK+ libs on Solaris 2.7 and SuSE 6.4.
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: