238854 - [gtk2] Changing GNOME2 theme doesn't apply until restarting Mozilla

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Hideo Oshima]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040325
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040325

If you change GNOME2 theme, the theme doesn't apply until you restart Mozilla
I use GTK+2 build.

Reproducible: Always
Steps to Reproduce:
1.Use Mozilla GTK+2 build
2.Change GNOME2 theme



Actual Results:  
New GNOME2 theme doesn't apply until restarting Mozilla.

Expected Results:  
New GNOME2 theme applies correctly without restarting Mozilla.

Comment 1

[Brian Ryner (not reading)]

Attachment: hook up NS_THEMECHANGED for gtk2

dispatch NS_THEMECHANGED on all windows when the toplevel window receives a
theme change notification

Comment 2

[Brian Ryner (not reading)]

checked in

Comment 3

[Frederic Bezies]

Sorry to spam this bug, but does this fix is related to bug 242768 ?

Comment 4

[Hideo Oshima]

I tried to build the source from CVS trunk.
But the new GNOME theme doesn't apply to Sidebar and page area.

Please look snapshot.

Comment 5

[Hideo Oshima]

Attachment: Screen shot

I changed GNOME theme simple to revert high contrast.

Comment 6

[Brian Ryner (not reading)]

Let's keep this bug specific to whether theme changes require a restart.  Does
Mozilla pick up theme changes as expected after restarting?

Comment 7

[R.K.Aa.]

just got this one with a fresh build:

** (Gecko:5342): CRITICAL **: file mozdrawingarea.c: line 175
(moz_drawingarea_finalize): assertion `IS_MOZ_CONTAINER(container)' failed

Comment 8

[R.K.Aa.]

filed bug 242978

Comment 9

[Ginn Chen]

There are some static color variant in nsLookAndFeel, we should refresh them
when THEMECHANGED.

Comment 10

[Ginn Chen]

patch was backed out.

Comment 11

[Ginn Chen]

Attachment: patch v1.5

Changes:
1. Use "style_set" instead of "client_event"/"_GTK_READ_RCFILES", because the
latter only works with RH9, doesn't work with JDS2 or Solaris. 
2. Add nsDeviceContextGTK:clearFontsCache, and call it when theme changed, to
realize fonts change without restart mozilla.
3. Call reflow in nsPrefsContext::ThemeChanged(). Without it, some color
settings will not apply to opened window.
4. Add gtk2/nsLookAndFeel::LookAndFeelChanged() to clear cached widget styles
and colors.

Comment 12

[Ginn Chen]

There's still a little problem after my patch.
If you open a dialog (e.g. Profile Manager, Prefs), then enlarge application
font, the dialog window will not resize automatically like gnome-application,
and the cell size of listbox or treeview will not enlarge, some text may be
overlapped.
I think it's a tiny issue. User can close and reopen the dialog to workaround it.

Comment 13

[Ginn Chen]

Comment on attachment 155152 [details] [diff] [review]
patch v1.5

review?

Comment 14

[Brian Ryner (not reading)]

Comment on attachment 155152 [details] [diff] [review]
patch v1.5

>--- gfx/src/gtk/nsDeviceContextGTK.cpp	3 Jun 2004 21:19:07 -0000	1.128
>+++ gfx/src/gtk/nsDeviceContextGTK.cpp	4 Aug 2004 05:58:11 -0000
>@@ -667,23 +667,29 @@ int nsDeviceContextGTK::prefChanged(cons
>     PRInt32 dpi;
>     nsCOMPtr<nsIPref> prefs(do_GetService(kPrefCID, &rv));
>     rv = prefs->GetIntPref(aPref, &dpi);
>     if (NS_SUCCEEDED(rv))
>       context->SetDPI(dpi);
> 
>     // If this pref changes, we have to clear our cache of stored system
>     // fonts.
>+    clearFontsCache();
>+  }
>+  
>+  return 0;
>+}
>+
>+void nsDeviceContextGTK::clearFontsCache()

please capitalize it as ClearFontsCache (I'm not sure why prefChanged isn't
that way).


>--- layout/base/src/nsPresContext.cpp	2 Aug 2004 04:52:53 -0000	3.270
>+++ layout/base/src/nsPresContext.cpp	4 Aug 2004 05:58:11 -0000
>@@ -1130,16 +1130,20 @@ nsPresContext::ThemeChanged()
>     mTheme->ThemeChanged();
> 
>   // Clear all cached nsILookAndFeel colors.
>   if (mLookAndFeel)
>     mLookAndFeel->LookAndFeelChanged();
>   
>   if (mShell)
>     mShell->ReconstructStyleData();
>+
>+#ifdef MOZ_WIDGET_GTK2
>+  ClearStyleDataAndReflow();
>+#endif

This shouldn't need to be a platform-specific #ifdef.  Also, I'm not sure if we
need to have both ReconstructStyleData and ClearStyleDataAndReflow, you should
ask dbaron or bzbarsky.


>--- widget/src/gtk2/nsWindow.cpp	27 Jul 2004 19:16:44 -0000	1.115
>+++ widget/src/gtk2/nsWindow.cpp	4 Aug 2004 05:58:13 -0000
>@@ -1738,16 +1741,56 @@ nsWindow::OnWindowStateEvent(GtkWidget *
>         event.mSizeMode = nsSizeMode_Normal;
>         mSizeState = nsSizeMode_Normal;
>     }
> 
>     nsEventStatus status;
>     DispatchEvent(&event, status);
> }
> 
>+void
>+nsWindow::ThemeChanged()
>+{
>+    nsGUIEvent event(NS_THEMECHANGED, this);
>+    nsEventStatus status = nsEventStatus_eIgnore;
>+    DispatchEvent(&event, status);
>+
>+    if (mContainer) {
>+        // Dispatch NS_THEME_CHANGED to all child windows
>+        GSList *areas = moz_container_get_drawing_areas(mContainer);

Blizzard had suggested a better way to handle this that doesn't require adding
all of the hooks to mozcontainer.  I can't find the log of my conversation with
him but here is some code I had in my tree:

+void
+nsWindow::ThemeChanged()
+{
+    nsGUIEvent event(NS_THEMECHANGED, this);
+    nsEventStatus status = nsEventStatus_eIgnore;
+    DispatchEvent(&event, status);
+
+    if (!mContainer)
+	 return;
+
+    // Dispatch NS_THEMECHANGED to all child windows
+    GList *children =
gdk_window_peek_children(GTK_WIDGET(mContainer)->window);+
+    while (children) {
+	 GdkWindow *gdkWin = GDK_WINDOW(children->data);
+
+	 nsWindow *win = (nsWindow*) g_object_get_data(G_OBJECT(gdkWin),
+						       "nsWindow");
+
+	 if (win && win != this)   // guard against infinite recursion
+	     win->ThemeChanged();
+
+	 children = children->next;
+    }
+}

Does that work for you?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 155152 [details] [diff] [review]
patch v1.5

>+    clearFontsCache();

This name also sounds too similar to the font cache, which this isn't.	Call it
|ClearCachedSystemFonts| or something like that instead.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 155152 [details] [diff] [review]
patch v1.5

>Index: layout/base/src/nsPresContext.cpp
>   if (mShell)
>     mShell->ReconstructStyleData();
>+
>+#ifdef MOZ_WIDGET_GTK2
>+  ClearStyleDataAndReflow();
>+#endif
> }

What does this fix?  I'm not sure why you're adding this so it's hard to know
what you meant.  I don't see why this should be necessary.

Comment 17

[Ginn Chen]

(In reply to comment #16)
> (From update of attachment 155152 [details] [diff] [review])
> >Index: layout/base/src/nsPresContext.cpp
> >   if (mShell)
> >     mShell->ReconstructStyleData();
I should remove this if I add below without #ifdef.
> >+
> >+#ifdef MOZ_WIDGET_GTK2
> >+  ClearStyleDataAndReflow();
> >+#endif
> > }
> 
> What does this fix?  I'm not sure why you're adding this so it's hard to know
> what you meant.  I don't see why this should be necessary.
> 
Mozilla need a reflow after theme changed.
It seems windows version don't need this.
Because nsNativeThemeWin::ThemeChanged() is different.
Without a reflow, the theme color/font change will only apply to new window, the
old ones are not changed. Because their widgets were created and applied old
theme style.

Comment 18

[Ginn Chen]

Attachment: patch v2

smaller and safer patch; keep mozcontainer/mozdrawingarea untouched.

Comment 19

[Ginn Chen]

Comment on attachment 156419 [details] [diff] [review]
patch v2

modified as your comment; just change
gdk_window_peek_children(GTK_WIDGET(mContainer)->window);
to
gdk_window_peek_children(mDrawingarea->inner_window);

it works for me.

Comment 20

[Ginn Chen]

Comment on attachment 156419 [details] [diff] [review]
patch v2

should be
+    if (!mContainer || !mDrawingarea)
+	 return;
sorry for missing protection here

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #17)
> Mozilla need a reflow after theme changed.
> It seems windows version don't need this.
> Because nsNativeThemeWin::ThemeChanged() is different.
> Without a reflow, the theme color/font change will only apply to new window, the
> old ones are not changed. Because their widgets were created and applied old
> theme style.

Why is the call to PresShell::ReconstructStyleData not sufficient?

Comment 22

[Ginn Chen]

(In reply to comment #21)
> Why is the call to PresShell::ReconstructStyleData not sufficient?

texts/backgrounds already drew won't apply new styles without reflow.
Without reflow the result is new opened windows will have new styles, but old 
ones still have partial old styles.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why is ReconstructStyleData not causing a reflow?  It should, if one is needed.

Comment 24

[Ginn Chen]

(In reply to comment #23)
> Why is ReconstructStyleData not causing a reflow?  It should, if one is needed.

I'm looking into it.
FrameManager()->ComputeStyleChangeFor always return empty changeList and 0.
I don't know the reason yet.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is the reason that no CSS properties are changing -- only the underlying meaning
of -moz-appearance?

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(If that's the case, then the patch is ok, as long as there's a comment in the
code that it's because of -moz-appearance (which is really implemented
incorrectly).)

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 156419 [details] [diff] [review]
patch v2

Well, what's in this version of the patch seems reasonable, although you don't
really need to clear style data.  Then again, clearing the style data may be
faster, since ReconstructStyleData will queue a lot of small invalidates.  But
it is worth commenting that you're doing this because of three things: system
font changes, system color changes, and -moz-appearance changes. 
ReconstructStyleData should be sufficient for the first two, but not the third.

Comment 28

[Ginn Chen]

(In reply to comment #27)
> (From update of attachment 156419 [details] [diff] [review])
> Well, what's in this version of the patch seems reasonable, although you don't
> really need to clear style data.  Then again, clearing the style data may be
> faster, since ReconstructStyleData will queue a lot of small invalidates.  But
> it is worth commenting that you're doing this because of three things: system
> font changes, system color changes, and -moz-appearance changes. 
> ReconstructStyleData should be sufficient for the first two, but not the third.
> 

I think I said something misleading in comments above.
The -moz-appearance is OK after gnome theme change.
But some colors are not matching.
e.g. Field Color, Button Color, they have new value after gnome theme changed,
but their name is unchanged.
The new values are applied to newly opened window/dialog, but not to window existed.
Font is as same as color.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #28)
> But some colors are not matching.
> e.g. Field Color, Button Color, they have new value after gnome theme changed,
> but their name is unchanged.
> The new values are applied to newly opened window/dialog, but not to window
existed.
> Font is as same as color.

Oh, I see.  We have to clear style data because the assumption of style rule
immutability has been violated since any style rule that uses system colors or
fonts (and probably -moz-appearance as well) has changed.  It's worth adding a
comment saying that.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... but given that, there's no reason to call ReconstructStyleData too, so you
should probably take that out.

Comment 31

[Ginn Chen]

(In reply to comment #30)
> ... but given that, there's no reason to call ReconstructStyleData too, so you
> should probably take that out.

The patch v2 calls ClearStyleDataAndReflow instead of ReconstructStyleData.
So I needn't re-work a patch, just need add comments in.

Please review it, bryner.

Comment 32

[Ginn Chen]

Attachment: patch v2 revised

Changes,
1. change if (!mContainer) to if(!mDrawingarea)
we don't use mContainer but mDrawingarea
2. add dbaron's comments

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 159082 [details] [diff] [review]
patch v2 revised

>+  // system colors or fonts (and probably -moz-appearance as well) has changed.

Could you wrap the word "changed" to the next line?

>+    mWidget = gtk_invisible_new();
>+    gtk_object_ref(GTK_OBJECT(mWidget));
>+    gtk_object_sink(GTK_OBJECT(mWidget));
>+    gtk_widget_ensure_style(mWidget);
>+    mStyle = gtk_widget_get_style(mWidget);

Could you put these 5 lines that you copied into their own function, perhaps
inline?

Comment 34

[Ginn Chen]

Attachment: patch to checkin

addressing dbaron's comments

Comment 35

[Kyle Yuan]

fixed.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 159168 [details] [diff] [review]
patch to checkin

Marking it inline but using it before it's defined will break some compilers. 
Please either define it in the header where it's declared (preferable) or move
the definition earlier (and probably mark it as inline as well).

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 159168 [details] [diff] [review]
patch to checkin

(I probably should have made my sr+ conditional on seeing my comments addressed
as I expected.)

Comment 38

[Ginn Chen]

Attachment: patch (move inline function to header)

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 159281 [details] [diff] [review]
patch (move inline function to header)

Yeah, although you don't need "inline" since functions *defined* (not just
declared) within a class definition are automatically inline.

Comment 40

[Ginn Chen]

(In reply to comment #39)
> (From update of attachment 159281 [details] [diff] [review])
> Yeah, although you don't need "inline" since functions *defined* (not just
> declared) within a class definition are automatically inline.

Yes, I awared this after post it.
I'll remove the inline modifier before check in.

Comment 41

[Kyle Yuan]

Comment on attachment 159281 [details] [diff] [review]
patch (move inline function to header)

checked in with |inline| modifier removed.

Comment 42

[Hideo Oshima]

I tried new build.

After changing the GNOME2 theme, I don't need to restart Mozilla
but need to reload the page or open new window/tab. Is it spec?

Anyway it's better. Thanks.

Comment 43

[Ginn Chen]

(In reply to comment #42)
> I tried new build.
> 
> After changing the GNOME2 theme, I don't need to restart Mozilla
> but need to reload the page or open new window/tab. Is it spec?
> 
> Anyway it's better. Thanks.
Hideo Oshima, 
I think you want web pages to use system colors for text and background, and you
want them get changed according to gnome-theme without reload.
It's another issue. I can also reproduce it on Windows.

Comment 44

[Hideo Oshima]

(In reply to comment #43)
> (In reply to comment #42)
> > I tried new build.
> > 
> > After changing the GNOME2 theme, I don't need to restart Mozilla
> > but need to reload the page or open new window/tab. Is it spec?
> > 
> > Anyway it's better. Thanks.
> Hideo Oshima, 
> I think you want web pages to use system colors for text and background, and you
> want them get changed according to gnome-theme without reload.
Yes. You're right.

> It's another issue. I can also reproduce it on Windows.
OK. It's not big problem.
Thanks for information. 

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This code gets called way too often with newer versions of GTK:  see bug 305970.