512529 - Fullscreen mode (F11) does not synchronize with window manager fullscreen mode

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Martin Gräßlin]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/4.3; Linux) KHTML/4.3.0 (like Gecko)
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.13) Gecko/2009080315 Ubuntu/9.04 (jaunty) Firefox/3.0.13

The fullscreen mode entered and exited by F11 does not react on changes by the window manager. It requires to press F11 although you ended fullscreen via the window manager and _NET_WM_STATE is changed accordingly.

This has been reproduced in a KDE 4.3 environment with KWin as a window manager. As KWin is NETWM compliant it is to be expected that the same behaviour is reproducable with any other NETWM compliant window manager such as e.g. Metacity or Compiz.

Reproducible: Always

Steps to Reproduce:
1. Press F11 to enter Fullscreen mode
2. Use tasks entry (or alt+f3) -> advanced -> fullscreen to end fullscreen
Actual Results:  
Firefox is still in fullscreen mode, but the window is decorated again. You have to press F11 to end the fullscreen mode, although fullscreen has been exited. The opposite way is wrong as well: when using the window manager to enter fullscreen Firefox does not enter it's fullscreen mode

Expected Results:  
Firefox listens to changes of _NET_WM_STATE and adjusts it's visual appearance according to the set properties.

When you enter fullscreen mode via F11 _NET_WM_STATE has the following values:
_NET_WM_STATE(ATOM) = _NET_WM_STATE_FULLSCREEN
                Initial state is Normal State.

After ending fullscreen via the window manager it looks like the following:
_NET_WM_STATE(ATOM) =
                Initial state is Normal State.

_NET_WM_ALLOWED_ACTIONS does always contain _NET_WM_ACTION_FULLSCREEN

Comment 1

[Michael Mior]

I am experiencing the same problem in the current nightly build.

Comment 2

[Karl Tomlinson (:karlt)]

It's actually worse with nightlies because after trying to end fullscreen, the window returns to fullscreen again.

Comment 3

[Karl Tomlinson (:karlt)]

BTW, Alt+Menu accesses the window operations menu here.

Comment 4

[Karl Tomlinson (:karlt)]

Looks like we shouldn't return early here if GDK_WINDOW_STATE_FULLSCREEN has changed:
http://hg.mozilla.org/mozilla-central/annotate/7d0c4369c0d1/widget/src/gtk2/nsWindow.cpp#l3252

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: detect fullscreen GdkEventWindowState events

(In reply to comment #4)
> Looks like we shouldn't return early here if GDK_WINDOW_STATE_FULLSCREEN has
> changed:
> http://hg.mozilla.org/mozilla-central/annotate/7d0c4369c0d1/widget/src/gtk2/nsWindow.cpp#l3252

I tried fixing that, but it didn't help.

Only nsWebShellWindow listens for the NS_SIZEMODE event and it only calls back to the widget.

Comment 6

[echogene.alpha]

This is still present in at least versions 7, 8 and 10.

Comment 7

[Philipp Hartwig]

Attachment: Attempted fix.

This patch causes Firefox to listen to changes in GDK_WINDOW_STATE_FULLSCREEN and react to them by going into and out of its fullscreen mode. It also seems to fix the behavior described in comment #4. I've tested it with a few window managers (Notion, i3, awesome, Fluxbox) and in all cases the behavior I've observed was as desired.

Any comments and suggestions are be greatly appreciated.

Comment 8

[Philipp Hartwig]

The reference was supposed to go to comment #2 rather than 4, sorry about that.

Comment 9

[Karl Tomlinson (:karlt)]

Thanks for looking at this, Philipp.

It seems that things have changed since I last looked.
Since
http://hg.mozilla.org/mozilla-central/diff/0b6d45b6c2e6/xpfe/appshell/src/nsWebShellWindow.cpp
nsWebShellWindow does do the right thing when switching to fullscreen.

The effectiveness of that change can be seen through the following steps:
Start with a window that is not maximized.  Use the window manager to make the
window fullscreen.  (The Firefox window does not yet use fullscreen UI.)
Minimize and restore the window.  Firefox is now using the fullscreen UI.

This bug now exists on NT also (bug 580564).
Making nsWebShellWindow call SetFullScreen(false) at the right times in
http://hg.mozilla.org/mozilla-central/rev/dc1077c09540
caused bug 663586 and so that changeset was backed out.

I can't reproduce comment 2 now.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 591291 [details] [diff] [review]
Attempted fix.

There are a few things to fix here.  It would be helpful if you could fix
each issue in a separate patch, please.

>diff -r c3643d492d68 widget/gtk2/nsWindow.cpp

>-    // We don't care about anything but changes in the maximized/icon
>+    // We don't care about anything but changes in the maximized/icon/fullscreen
>     // states
>     if ((aEvent->changed_mask
>-         & (GDK_WINDOW_STATE_ICONIFIED|GDK_WINDOW_STATE_MAXIMIZED)) == 0) {
>+         & (GDK_WINDOW_STATE_ICONIFIED|GDK_WINDOW_STATE_MAXIMIZED|GDK_WINDOW_STATE_FULLSCREEN)) == 0) {
>         return;
>     }

This addresses one thing that needs fixing (comment 4) and looks good, but can
you split the series of GDK_WINDOW_STATE_* bits onto separate lines to keep
code within 80 columns, please?  i.e.

    if ((aEvent->changed_mask & (GDK_WINDOW_STATE_ICONIFIED |
                                 GDK_WINDOW_STATE_MAXIMIZED |
                                 GDK_WINDOW_STATE_FULLSCREEN)) == 0) {
        return;

> 
>-    if (aEvent->new_window_state & GDK_WINDOW_STATE_ICONIFIED) {

>+    if (aEvent->new_window_state & GDK_WINDOW_STATE_FULLSCREEN) {
>+        LOG(("\tFullscreen\n"));
>+        event.mSizeMode = nsSizeMode_Fullscreen;
>+        mSizeState = nsSizeMode_Fullscreen;
>+    }
>+    else if (aEvent->new_window_state & GDK_WINDOW_STATE_ICONIFIED) {
>         LOG(("\tIconified\n"));
>         event.mSizeMode = nsSizeMode_Minimized;
>         mSizeState = nsSizeMode_Minimized;
> #ifdef ACCESSIBILITY
>         DispatchMinimizeEventAccessible();
> #endif //ACCESSIBILITY
>     }
>     else if (aEvent->new_window_state & GDK_WINDOW_STATE_MAXIMIZED) {
>         LOG(("\tMaximized\n"));
>         event.mSizeMode = nsSizeMode_Maximized;
>         mSizeState = nsSizeMode_Maximized;
> #ifdef ACCESSIBILITY
>         DispatchMaximizeEventAccessible();
> #endif //ACCESSIBILITY
>     }
>-    else if (aEvent->new_window_state & GDK_WINDOW_STATE_FULLSCREEN) {
>-        LOG(("\tFullscreen\n"));
>-        event.mSizeMode = nsSizeMode_Fullscreen;
>-        mSizeState = nsSizeMode_Fullscreen;
>-    }
>     else {
>         LOG(("\tNormal\n"));
>         event.mSizeMode = nsSizeMode_Normal;
>         mSizeState = nsSizeMode_Normal;

Yes, another thing that needs fixing is that GDK_WINDOW_STATE_FULLSCREEN needs
to be checked before GDK_WINDOW_STATE_MAXIMIZED because the two can coexist.
In fact, _NET_WM_STATE_MAXIMIZED_VERT, _NET_WM_STATE_MAXIMIZED_HORZ,
_NET_WM_STATE_HIDDEN, and _NET_WM_STATE_FULLSCREEN can all exist concurrently
and I assume GDK also passes its states concurrently. 

However, if GDK_WINDOW_STATE_FULLSCREEN and GDK_WINDOW_STATE_ICONIFIED are
both set, I think nsSizeMode_Minimized is more important than
nsSizeMode_Fullscreen, and should be the value sent in the event.

>diff -r c3643d492d68 xpfe/appshell/src/nsWebShellWindow.cpp

>           // Let the application know if it's in fullscreen mode so it
>           // can update its UI.
>-          if (modeEvent->mSizeMode == nsSizeMode_Fullscreen) {
>-            ourWindow->SetFullScreen(true);
>+
>+          // This is an attempted fix for bug 512529. The bug only affects the 
>+          // GTK library. We therefore preserve the old behavior if 
>+          // ToggleFullscreen has not explicitly been set to either "true" or 
>+          // "false" (read "-1" as "not set") in 
>+          // widget/gtk2/nsWindow.cpp:OnWindowStateEvent. This should avoid any 
>+          // surprises with other libraries. 
>+          if (modeEvent->ToggleFullscreen == -1) {
>+            if(modeEvent->mSizeMode == nsSizeMode_Fullscreen)
>+              ourWindow->SetFullScreen(true);
>+          }
>+          // We only call SetFullscreen if the fullscreen state has actually 
>+          // changed.
>+          else if (modeEvent->ToggleFullscreen == true) {
>+            if(modeEvent->mSizeMode == nsSizeMode_Fullscreen)
>+              ourWindow->SetFullScreen(true);
>+            else
>+              ourWindow->SetFullScreen(false);
>           }

And, yes, this is the other thing that needs fixing.
We can achieve the same result (without regressing bug 663586 on NT) by
putting the "else ourWindow->SetFullScreen(false)'" within "#ifdef
MOZ_WIDGET_GTK2" or "#ifndef XP_WIN".  I'd prefer that over adding a
ToggleFullscreen member to the event, given the necessary info is available in
mSizeMode.

I'm probably not the correct reviewer for nsWebShellWindow.cpp, so perhaps try
[e-mail] for this.

Comment 11

[Philipp Hartwig]

Karl, thank you very much for looking at my patch. I've covered your first and 
your third remark in two new patches.

> Yes, another thing that needs fixing is that GDK_WINDOW_STATE_FULLSCREEN
> needs to be checked before GDK_WINDOW_STATE_MAXIMIZED because
> the two can coexist.  In fact, _NET_WM_STATE_MAXIMIZED_VERT,
> _NET_WM_STATE_MAXIMIZED_HORZ, _NET_WM_STATE_HIDDEN, and
> _NET_WM_STATE_FULLSCREEN can all exist concurrently and I assume GDK
> also passes its states concurrently.
>
> However, if GDK_WINDOW_STATE_FULLSCREEN and GDK_WINDOW_STATE_ICONIFIED
> are both set, I think nsSizeMode_Minimized is more important than
> nsSizeMode_Fullscreen, and should be the value sent in the event.

There is one problem with that: GDK_WINDOW_STATE_ICONIFIED is not equivalent to 
the window being minimized but is also set by some window managers (awesome, 
Notion, ...) when the virtual desktop the client is on is not currently 
selected. In fact this is explicitly suggested in the EWMH spec[1].

If we prefer GDK_WINDOW_STATE_ICONIFIED over GDK_WINDOW_STATE_FULLSCREEN this 
means that with the proposed change to nsWebShellWindow, Firefox will leave its 
fullscreen mode whenever we switch virtual desktops. This is not desirable. I'm
therefore still in favor of prefering GDK_WINDOW_STATE_FULLSCREEN over 
GDK_WINDOW_STATE_ICONIFIED. The only other implication of doing this that I've 
observed is that if Firefox is first minimized and then restored while in 
fullscreen mode, it will still be in fullscreen mode.  Given that this is the 
current behavior anyway and that AFAIK nobody has complained about it this 
should be fine!? Are there other implications? 

Or do you see another solution for this?

[1] http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#id2688137

Comment 12

[Philipp Hartwig]

Attachment: Listen for changes in GDK_WINDOW_STATE_FULLSCREEN

Comment 13

[Philipp Hartwig]

Attachment: Leave fullscreen mode when using GTK and GDK_WINDOW_STATE_FULLSCREEN is not set.

Comment 14

[Jim Mathies [:jimm]]

Triggering fullscreen from widget on Windows was fraught with a number of issues. Basically, we don't support it currently. Since bug 580564 was the only known side effect, I just decided to leave things as they were.

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to Philipp Hartwig from comment #11)
> > However, if GDK_WINDOW_STATE_FULLSCREEN and GDK_WINDOW_STATE_ICONIFIED
> > are both set, I think nsSizeMode_Minimized is more important than
> > nsSizeMode_Fullscreen, and should be the value sent in the event.
> 
> There is one problem with that: GDK_WINDOW_STATE_ICONIFIED is not equivalent to 
> the window being minimized but is also set by some window managers (awesome, 
> Notion, ...) when the virtual desktop the client is on is not currently 
> selected. In fact this is explicitly suggested in the EWMH spec[1].
> 
> If we prefer GDK_WINDOW_STATE_ICONIFIED over GDK_WINDOW_STATE_FULLSCREEN this 
> means that with the proposed change to nsWebShellWindow, Firefox will leave its 
> fullscreen mode whenever we switch virtual desktops. This is not desirable.

Does this lead to flicker of the wrong UI on switching desktops?

> Are there other implications? 

nsSizeMode_Minimized is important because it is an indicator that the window is not visible and so animations and events can be throttled to save cpu.

> Or do you see another solution for this?

How about calling ourWindow->SetFullScreen(false) only for visible mSizeMode states?  i.e. not on nsSizeMode_Minimized.

Comment 16

[Philipp Hartwig]

(In reply to Karl Tomlinson (:karlt) from comment #15)
> (In reply to Philipp Hartwig from comment #11)
> > If we prefer GDK_WINDOW_STATE_ICONIFIED over GDK_WINDOW_STATE_FULLSCREEN this 
> > means that with the proposed change to nsWebShellWindow, Firefox will leave its 
> > fullscreen mode whenever we switch virtual desktops. This is not desirable.
> 
> Does this lead to flicker of the wrong UI on switching desktops?

Sorry, I'm not sure what you mean by that. I only observe that when starting 
with a virtual desktop with a fullscreened Firefox on it, switching away from 
and back to that virtual desktop leads to a Firefox in normal mode. So probably 
the answer to your question is "No".

> nsSizeMode_Minimized is important because it is an indicator that the window
> is not visible and so animations and events can be throttled to save cpu.

I see, thanks.

> How about calling ourWindow->SetFullScreen(false) only for visible mSizeMode
> states?  i.e. not on nsSizeMode_Minimized.

For some reason I thought that we again needed a ToggleFullscreen variable as 
in the first patch 591291 for this. But such a variable would only be desirable 
in case the window manager removes the GDK_WINDOW_STATE_FULLSCREEN bit on 
minimizing a fullscreened window. However at least the KDE window manager and 
awesome preserve this bit on minimizing as I've just tested. So we should be 
fine with your suggestion.

I've attached two patches which give me the desired behavior with all 
window managers I've tried (awesome, i3, KDE, Notion).

Comment 17

[Philipp Hartwig]

Attachment: Prefer GDK_WINDOW_STATE_FULLSCREEN over GDK_WINDOW_STATE_MAXIMIZED

Comment 18

[Philipp Hartwig]

Attachment: Leave fullscreen mode when it seems appropriate (GTK only).

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 593004 [details] [diff] [review]
Leave fullscreen mode when it seems appropriate (GTK only).

>+          // This is an attempted fix for bug 512529. A similar change is 
>+          // known to cause problems under Windows, see bug 663586.

No need for "This is an attempted fix for bug 512529" sentence as you've
already tested that this works on four different window managers thank you
very much.  Leaving a comment to indicate that bug 663586 is the reason why
this code is not used for Windows is helpful.

(In reply to Philipp Hartwig from comment #16)
> I only observe that when starting 
> with a virtual desktop with a fullscreened Firefox on it, switching away
> from and back to that virtual desktop leads to a Firefox in normal mode. So
> probably the answer to your question is "No".

Thanks for describing.  I hadn't thought through enough to foresee that.
Unfortunately in many areas Gecko fails to distinguish between external
notifications and internal requests, so SetFullScreen(false) on minimize
notification was actually calling back to the window manager to disable
fullscreen.  Anyway the latest patch resolves that.  Also, GDK and
nsGlobalWindow::SetFullscreen both check and no-op when notified of the
existing state, preventing a repeating loop.

I'm putting my r+ on this because SetFullScreen(false) on leave fullscreen
notification is as correct as SetFullScreen(true) on enter fullscreen
notification.  We should still give Neil opportunity to comment.

Comment 20

[Philipp Hartwig]

(In reply to Karl Tomlinson (:karlt) from comment #19)
> No need for "This is an attempted fix for bug 512529" sentence as you've
> already tested that this works on four different window managers thank you
> very much.  Leaving a comment to indicate that bug 663586 is the reason why
> this code is not used for Windows is helpful.

I'll do this as soon as we've heard from Neil. So there shouldn't be any 
reference to this bug?

> I'm putting my r+ on this because SetFullScreen(false) on leave fullscreen
> notification is as correct as SetFullScreen(true) on enter fullscreen
> notification.  

Thank you very much.

Comment 21

[Karl Tomlinson (:karlt)]

(In reply to Philipp Hartwig from comment #20)
> So there shouldn't be any reference to this bug?

That's right.  People can use hg annotate to check the code history.

Comment 22

[[e-mail]]

Comment on attachment 593004 [details] [diff] [review]
Leave fullscreen mode when it seems appropriate (GTK only).

>+          // This is an attempted fix for bug 512529. A similar change is 
>+          // known to cause problems under Windows, see bug 663586.
>+#ifdef MOZ_WIDGET_GTK2
I compared this code with the code that caused bug 663586, and there is a subtle difference in the code paths, which should mean that this code won't trigger that bug; at least, I failed to reproduce it with this change. I therefore suggest that you check the code in without the #ifdef guard. (And then of course you can ditch the comment completely.)

Comment 23

[Philipp Hartwig]

Attachment: Leave fullscreen mode when it seems appropriate.

Comment 24

[Philipp Hartwig]

Karl and Neil, thank you very much for your comments and help. May I ask you to commit the patches?

Comment 25

[Philipp Hartwig]

Attachment: Prefer GDK_WINDOW_STATE_FULLSCREEN over GDK_WINDOW_STATE_MAXIMIZED

Comment 26

[Philipp Hartwig]

Attachment: Listen for changes in GDK_WINDOW_STATE_FULLSCREEN

Comment 27

[Philipp Hartwig]

Attachment: Leave fullscreen mode when it seems appropriate.

Comment 28

[Ed Morley [:emorley]]

Comment on attachment 594604 [details] [diff] [review]
Prefer GDK_WINDOW_STATE_FULLSCREEN over GDK_WINDOW_STATE_MAXIMIZED

(To keep autoland to try happy about permissions)

Comment 29

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 594604, 594605, 594606
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/48ba034b7181
Try run started, revision 48ba034b7181. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=48ba034b7181

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0267222fc4
https://hg.mozilla.org/integration/mozilla-inbound/rev/672b277a4a2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b0294c7b952

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fb0267222fc4
https://hg.mozilla.org/mozilla-central/rev/672b277a4a2c
https://hg.mozilla.org/mozilla-central/rev/1b0294c7b952

Thank you for the patch Philipp! Join us on irc (#developers on irc.mozilla.org) and we'll find some other things for you to work on if you are interested? :-D