1478576 - System theme change doesn't affect to child content if the new window hasn't moved since its creation

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

I found this issue when I was trying to write a test case for bug 1478519.

Steps to reproduce

1. Open a new firefox instance
2. Don't touch/move the window
3. Change the system theme somehow (I am using gnome-tweaks)

After that, the appearance of the window tile bar and some such changes, but the appearance of the tab bar doesn't change at all.

What I can see in debugging is that corresponding nsWindow instance isn't created until the window is moved or something, so ThemeChanged() for the appropriate nsPresShell is not called at all.

I don't know much about nsIWidget and relevant stuff at all.  So I need you guys help. :)

Comment 1

[Karl Tomlinson (:karlt)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> What I can see in debugging is that corresponding nsWindow instance isn't
> created until the window is moved or something, so ThemeChanged() for the
> appropriate nsPresShell is not called at all.

That doesn't make sense because there would be no window to move if the nsWindow
does not exist.  MOZ_LOG=Widget:5 in the environment may be helpful.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Thank you karlt for the tip.  I just tried. 

// here I did toggle animation setting on gnome-tweak
0x7f683dcf7c00 settings_changed_cb
0x7f6831958c00 settings_changed_cb <- these lines are debug printf I put in settings_changed_cb. The pointer is the address of nsWindow*.
// here I did click the opened firefox window.

[Parent 15685: Main Thread]: D/Widget OnEnterNotify: 0x7f683dcf7c00
[Parent 15685: Main Thread]: D/Widget GetScreenBounds 591,27 | 1280x947
[Parent 15685: Main Thread]: D/Widget nsWindow::OnWindowStateEvent [0x7f683dcf7c00] changed 128 new_window_state 87168
[Parent 15685: Main Thread]: D/Widget nsWindow::OnWindowStateEvent [0x7f683dcf7c00] changed 128 new_window_state 87168
[Parent 15685: Main Thread]: D/Widget GetScreenBounds 591,27 | 1280x947
[Parent 15685: Main Thread]: D/Widget nsWindow [0x7f6822843000]
[Parent 15685: Main Thread]: D/Widget 	mShell 0x7f6822843660 mContainer 0x7f6825ab3410 mGdkWindow 0x7f6826965050 0x4400027
[Parent 15685: Main Thread]: D/Widget nsWindow::NativeMoveResize [0x7f6822843000] 841 110 229 29
[Parent 15685: Main Thread]: D/Widget nsWindow::Show [0x7f6822843000] state 1
[Parent 15685: Main Thread]: D/Widget size_allocate [0x7f6822843000] 0 0 229 29
[Parent 15685: Main Thread]: D/Widget nsWindow::OnWindowStateEvent [0x7f6822843000] changed 129 new_window_state 128
[Parent 15685: Main Thread]: D/Widget nsWindow::OnWindowStateEvent [0x7f6822843000] changed 129 new_window_state 128
[Parent 15685: Main Thread]: D/Widget configure event [0x7f6822843000] 841 110 229 29
[Parent 15685: Main Thread]: D/Widget GetScreenBounds 841,110 | 229x29
[Parent 15685: Main Thread]: D/Widget nsWindow::Show [0x7f6822843000] state 0
[Parent 15685: Main Thread]: D/Widget nsWindow::OnWindowStateEvent [0x7f6822843000] changed 1 new_window_state 129
[Parent 15685: Main Thread]: D/Widget nsWindow::OnWindowStateEvent [0x7f6822843000] changed 1 new_window_state 129
[Parent 15685: Main Thread]: D/Widget OnLeaveNotify: 0x7f683dcf7c00
[Parent 15685: Main Thread]: D/Widget nsWindow::OnWindowStateEvent [0x7f683dcf7c00] changed 128 new_window_state 87040
[Parent 15685: Main Thread]: D/Widget nsWindow::OnWindowStateEvent [0x7f683dcf7c00] changed 128 new_window_state 87040

// here I did toggle animation setting again
0x7f683dcf7c00 settings_changed_cb
0x7f6831958c00 settings_changed_cb
0x7f6822843000 settings_changed_cb  <- this nsWindow is what I am saying.

It seems to me that nsWindow(0x7f6822843000) was't create in the first place. It was created after the content is clicked?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Err, actually at that time, I might just hover over the mouse cursor instead of clicking.

Comment 4

[Karl Tomlinson (:karlt)]

229x29 is a small window (possibly a tooltip).  What makes you sure you want the notification on that window?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Yeah, I noticed that after I commented. 

(In reply to Karl Tomlinson (:karlt) from comment #4)
> 229x29 is a small window (possibly a tooltip).  What makes you sure you want
> the notification on that window?

I've been checking the corresponding nsIPresShell* (mWidgetListener->GetPresShell()), the pres shell for the tooltip is for browser.xul, there is no other nsWindow* in nsBaseWidget::NotifyThemeChanged().  All others does bail out due to GetXULWindow check.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Now I think the GetXULWindow check is an overkill.  The check was introduced in bug 743975, especially this commit [1].  Before that it seems to me that we hadn't bailed even if GetXULWindow() returns a valid nsIXULWindow pointer.

And as far as I can tell, when a new window is open, there are two widgets, one is for browser window, the other is for hidden window.  GetXULWindow() returns a valid pointer for both widgets. I don't know what the hidden window does, but we should call notification functions for the browser window one?

Neil, for what purpose does the GetXULWindow() check in ThemeChanged [1] and others?  I mean which window was supposed to be skipped for the notifications?

[1] https://hg.mozilla.org/mozilla-central/rev/4aca82dc0d41007d1fdf231d1c3be86705132d73
[2] https://hg.mozilla.org/mozilla-central/file/0be4463d2915/widget/nsBaseWidget.cpp#l1874

Comment 7

[Neil Deakin]

I looked through the code from before 743975 and the patch looks correct. When GetXULWindow would return true, nsWebShellWindow::HandleEvent ignored the theme change notification NS_THEMECHANGED:

https://hg.mozilla.org/mozilla-central/file/9c743145c60d/xpfe/appshell/src/nsWebShellWindow.cpp

Did you verify that this is actually a regression from 743975, or some later bug that changed the widget structure?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Neil Deakin from comment #7)
> I looked through the code from before 743975 and the patch looks correct.
> When GetXULWindow would return true, nsWebShellWindow::HandleEvent ignored
> the theme change notification NS_THEMECHANGED:
> 
> https://hg.mozilla.org/mozilla-central/file/9c743145c60d/xpfe/appshell/src/
> nsWebShellWindow.cpp
> 
> Did you verify that this is actually a regression from 743975, or some later
> bug that changed the widget structure?

No, I should have tried mozregression first. (I though it won't work)

Unfortunately, it couldn't narrow down enough but the result is;

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4dfe323a663d&tochange=634180132e68

I don't see any suspicious there from the commit messages.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Also I noticed that in bug 1368808 we didn't use NotifyThemeChanged(), instead we do directly call nsIPresShell::ThemeChanged() with eWindowType_toplevel check [1]

[1] https://hg.mozilla.org/integration/autoland/rev/48bb14249b06#l2.15

:mhowell, do you recall why you didn't use nsBaseWidget::NotifyThemeChanged() instead of the direct call?  I guess it didn't work as expected if you used NotifyThemeChanged?

Comment 10

[Molly Howell (she/her) (mostly inactive)]

nsBaseWidget::NotifyThemeChanged() had no effect because the check at [1] was failing. I didn't see a good way around that, so I just went straight to the presShell.

[1] https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/widget/nsBaseWidget.cpp#1850

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Thank you, mhowell, that's exactly what I am suffering from in this bug.
Ok, I will narrow the window in comment 8 down somehow.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

I did setup an environment to be able to build old revision and did bisect. It shows me that bug 809694 regressed this.  But I don't think bug 809694 regressed, I think it just unwallpaper a pre-existing bug.  Adding bug 809694 for now.  I have to check the real regression bug.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

This bug was regressed by some bugs gradually.

In bug 743975, GetXULWindow() check was introduced.  At the revision, a child widget (whose listener has no XULWindow) for browser.xul is created, thus the problem doesn't happen actually.

Since bug 784879, a popup widget whose pres shell is browser.xul had been also created, and the popup widget's listener has no XULWindow

Since bug 743975, the child widget has no longer created.  Thus, since this revision, the pres shell for the popup widget had been responsible for the theme changes notification.

Since bug 809694, the popup widget has no longer created.  Thus there has been no widgets whose listener has no XULWindow.

A change [1] in bug 743975 helped me a lot to understand what we should do here in this bug.  We should check eWindowType_toplevel and eWindowType_dialog along with the GetXULWindow check.

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/a0158d370785#l1.13

Comment 14

[Neil Deakin]

Back when I worked on 743975, the widget structure was always that there was a toplevel window (one with a XULWindow) and a child widget (one in which GetXULWindow returned null but GetView returned a view), nested within each other. I assume that is no longer the case.

It would be OK to send the ThemeChanged notification to only the toplevel window instead (eWindowType_sheet is also a top level window I believe), assuming that gets propagated down through child widgets and remote browsers. (Non-remote browsers such as the sidebar)

But all the places where the presshell is called directly should be checked as we don't need to send the notification multiple times. The System color changed notification probably should be treated the same way.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Neil Deakin from comment #14)
> Back when I worked on 743975, the widget structure was always that there was
> a toplevel window (one with a XULWindow) and a child widget (one in which
> GetXULWindow returned null but GetView returned a view), nested within each
> other. I assume that is no longer the case.
> 
> It would be OK to send the ThemeChanged notification to only the toplevel
> window instead (eWindowType_sheet is also a top level window I believe),

Good catch! That's right.  That's being said, eWindowType_sheet seems to be only for MacOSX, and on MacOSX we do still create a child widget (as per the ifdef in nsDocumentViewer::ShouldAttachToTopLevel).
I have no Mac environments so that I can check it by myself though.

> But all the places where the presshell is called directly should be checked
> as we don't need to send the notification multiple times. The System color
> changed notification probably should be treated the same way.

Yep, I have already unified the system color change function in https://hg.mozilla.org/mozilla-central/rev/f0e52163b195

Anyway I did finally manage to write a mochitest for this.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b2b8f461140de91e5177bf45a8ea2d0dced26b2

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1478576 - Drop GetXULWindow() check in nsBaseWidget::NotifyPresShell.

On some platforms we don't create any child widgets for browser.xul window when
the window is created, thus the top level widget for browser.xul had never
called the notification functions for the pres shell until another child or
popup widget is created for the browser.xul window.

Review commit: https://reviewboard.mozilla.org/r/261142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261142/

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1478576 - Use nsBaseWidget::NotifyThemeChanged for dark/liehgt theme changes on Windows 10.

Now nsBaseWidget::NotifyThemeChanged properly notifies to the pres shell.

Review commit: https://reviewboard.mozilla.org/r/261144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261144/

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1478576 - A mochitest for system font change notification.

The test case in this patch fails without the proper fix in the first patch
in this patch series.

In this patch two new nsIDOMWindowUtils APIs are introduced to change the
system font settins in tests.  Currently the APIs work only on GTK+ platform.

Also to work the test case properly we need to open a new XUL window because we
don't propagate font changes into descendant documents yet (bug 1478212).

Review commit: https://reviewboard.mozilla.org/r/261146/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261146/

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8997285 [details]
Bug 1478576 - A mochitest for system font change notification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261146/diff/1-2/

Comment 20

[Nathan Froyd [:froydnj]]

Comment on attachment 8997285 [details]
Bug 1478576 - A mochitest for system font change notification.

https://reviewboard.mozilla.org/r/261146/#review268224

r=me on the sync-messages.ini changes.  I assume that's all I was being asked to review. :)

Comment 21

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8997284 [details]
Bug 1478576 - Use nsBaseWidget::NotifyThemeChanged for dark/liehgt theme changes on Windows 10.

https://reviewboard.mozilla.org/r/261144/#review268270

Looks good; thanks for getting rid of my hacky workaround and fixing this properly.

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8997283 [details]
Bug 1478576 - Drop GetXULWindow() check in nsBaseWidget::NotifyPresShell.

https://reviewboard.mozilla.org/r/261142/#review268652

::: commit-message-1a0c5:3
(Diff revision 1)
> +On some platforms we don't create any child widgets for browser.xul window when
> +the window is created, thus the top level widget for browser.xul had never

It sounds like this is key piece of code involved in the change of behavior
that caused this regression:
https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/layout/base/nsDocumentViewer.cpp#2607

If shouldAttach is false, a child widget is created, which was always the
situation before changes for bug 743975 IIUC.
If shouldAttach is true, then, instead of creating a child widget, the view
registers mAttachedWidgetListener on the existing widget.
We would expect equivalent notifications if mAttachedWidgetListener receives the same notifications that a child widget would. 

If mWidgetListener and mAttachedWidgetListener have the same PresShell, then
it doesn't matter which listener is used to find the PresShell, and there is
no need to notify the PresShells of both listeners.

If a child widget is created, and has the same PresShell as its parent widget,
then there is no need to notify the PresShell from both widgets.
Is the GetXULWindow() test merely an optimization to avoid notifying the same
PresShell twice?

nsPresContext::SysColorChanged() and nsPresContext::ThemeChanged() are careful
to avoid doing work until a new event passes through the event queue.  i.e. it
will delay handling all such notifications that are in the event queue at one
time so that they are all handled together.

It also looks harmless to call PresShell::WindowSizeMoveDone() more often than
necessary.

So, is there any need for a GetXULWindow() test at all?

It looks like the code is careful to optimize away multiple calls.
Even if not, theme changes are not so common that we need to carefully
optimize them.  I'd prefer simpler code where it is clear that the right
notifications are happening.

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 8997285 [details]
Bug 1478576 - A mochitest for system font change notification.

https://reviewboard.mozilla.org/r/261146/#review268664

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 8997285 [details]
Bug 1478576 - A mochitest for system font change notification.

https://reviewboard.mozilla.org/r/261146/#review268670

The "gtk-font-name" code is correct.  Jim, can you find a reviewer for the
other parts, please?

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8997283 [details]
Bug 1478576 - Drop GetXULWindow() check in nsBaseWidget::NotifyPresShell.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261142/diff/1-2/

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8997284 [details]
Bug 1478576 - Use nsBaseWidget::NotifyThemeChanged for dark/liehgt theme changes on Windows 10.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261144/diff/1-2/

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8997285 [details]
Bug 1478576 - A mochitest for system font change notification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261146/diff/2-3/

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8997285 [details]
Bug 1478576 - A mochitest for system font change notification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261146/diff/3-4/

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 8997283 [details]
Bug 1478576 - Drop GetXULWindow() check in nsBaseWidget::NotifyPresShell.

https://reviewboard.mozilla.org/r/261142/#review268908

Thanks!

Comment 30

[David Durst [:ddurst]]

review hot potato to jimm

Comment 31

[Jim Mathies [:jimm]]

Comment on attachment 8997285 [details]
Bug 1478576 - A mochitest for system font change notification.

https://reviewboard.mozilla.org/r/261146/#review269580

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Thank you Jim!

A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22898cc395936977346ce91dc2e98737a758029a

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1392716e580
Drop GetXULWindow() check in nsBaseWidget::NotifyPresShell. r=karlt
https://hg.mozilla.org/integration/autoland/rev/f74ba700aff0
Use nsBaseWidget::NotifyThemeChanged for dark/liehgt theme changes on Windows 10. r=mhowell
https://hg.mozilla.org/integration/autoland/rev/fb3e5d2dd1d7
A mochitest for system font change notification. r=froydnj,jimm,karlt

Comment 34

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/e1392716e580
https://hg.mozilla.org/mozilla-central/rev/f74ba700aff0
https://hg.mozilla.org/mozilla-central/rev/fb3e5d2dd1d7

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Given the age of the bug, I think we can let this ride the trains. Feel free to nominate if you feel strongly otherwise, though.