Turn on nsNativeThemeGTK

RESOLVED FIXED in mozilla1.1beta

Status

RESOLVED FIXED
17 years ago
11 years ago

People

(Reporter: bryner, Assigned: bryner)

Tracking

Trunk
mozilla1.1beta
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Assignee)

Description

17 years ago
Tracking bug for turning on the GTK native theme implementation by default.
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1beta

Comment 1

17 years ago
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...

Comment 2

17 years ago
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.
(Assignee)

Comment 3

17 years ago
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.

Comment 4

17 years ago
For the record, comment #2 is referring to bug 139294.
(Assignee)

Comment 5

17 years ago
Created attachment 95929 [details] [diff] [review]
the obvious patch
Comment on attachment 95929 [details] [diff] [review]
the obvious patch

r=blizzard
Attachment #95929 - Flags: review+
(Assignee)

Comment 7

17 years ago
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.*

Updated

17 years ago
Attachment #95929 - Flags: superreview+

Comment 8

17 years ago
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.
(Assignee)

Comment 12

17 years ago
roc- what gtk theme does this use?
Created attachment 96620 [details]
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

Comment 14

17 years ago
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.

Comment 17

17 years ago
Is there a way to turn the native themes support OFF at runtime ? On 8bit
displays the Zilla now looks like a ugly patchwork.

Comment 18

17 years ago
confirmed crash with Crux GTK theme engine on startup :)
2002082521/Linux.

Comment 19

17 years ago
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.

Comment 20

17 years ago
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

Comment 29

17 years ago
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? :-)
Created attachment 100862 [details] [diff] [review]
proposed fix

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?
(Assignee)

Comment 33

17 years ago
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...

Comment 38

16 years ago
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.
(Assignee)

Comment 40

16 years ago
Comment on attachment 100862 [details] [diff] [review]
proposed fix

sr=bryner
Attachment #100862 - Flags: superreview+
(Assignee)

Comment 41

16 years ago
I checked in roc's patch to the trunk.

Comment 42

16 years ago
Created attachment 101986 [details]
Eazel aka Crux aka Mandrake GTK engine

Source of Crux/Eazel/Mandrake GTK1 engine which is causing native GTK theme
troubles...

Comment 43

16 years ago
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.

Comment 45

16 years ago
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..

Comment 46

16 years ago
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...

Comment 47

16 years ago
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.
(Assignee)

Comment 48

16 years ago
Created attachment 102699 [details] [diff] [review]
fix for the checkbox problem, for reference

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)

Comment 52

16 years ago
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. )

Comment 55

16 years ago
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).

Comment 56

16 years ago
Created attachment 102977 [details]
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.
(Assignee)

Comment 58

16 years ago
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
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 59

16 years ago
OK, filed them both. The missing left border appears to be a yet another
rounding error. bug 174637 (border) and bug 174639 (scrollbar pixmap).

Comment 60

16 years ago
bryner:
Is there a prefs to turn this feature "on"/"off" on demand ?
You can change your theme to something other than "Classic".

Comment 62

16 years ago
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.
(Assignee)

Comment 63

16 years ago
waah.

Comment 64

16 years ago
gisburn, stop being a dumbass.  if you want a stable mozilla, use a STABLE
mozilla release.  

Comment 65

16 years ago
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.
(Assignee)

Comment 66

16 years ago
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.

Comment 67

16 years ago
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.

Comment 68

16 years ago
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 ?
(Assignee)

Comment 69

16 years ago
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.

Comment 70

16 years ago
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.

Comment 71

16 years ago
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.
(Assignee)

Comment 74

16 years ago
>> 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.

Comment 75

16 years ago
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.

Comment 76

16 years ago
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.

Comment 78

16 years ago
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

Comment 82

16 years ago
It seems that buttons are not themed by GTK when Crux theme is used... anyone?

Comment 83

16 years ago
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).
(Assignee)

Comment 84

16 years ago
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.

Comment 85

16 years ago
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.