Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fullscreen window level corner cases on macOS #277

Closed
wants to merge 3 commits into from

Conversation

samhed
Copy link

@samhed samhed commented Oct 5, 2021

I'll begin with my understanding of the intended window levels from the windowDidResignKey and windowDidBecomeKey functions:

  • Fullscreen non keyWindow = NSNormalWindowLevel
  • Fullscreen keyWindow = NSStatusWindowLevel

Our application listens to FL_SCREEN_CONFIGURATION_CHANGED to be able to handle when a new monitor has been added while the window is in fullscreen. When this happens we need to call FLTK's fullscreen_x() to enable fullscreen on the new monitor as well. The function needs to be called while the window already is in fullscreen.

It's possible that this happens when the fullscreen window isn't keyWindow, and in this case the proper level seems to be NSNormalWindowLevel instead of NSStatusWindowLevel.

We also want to avoid re-stacking windows within the same level, this happens when setting level to the same value again. The window who's level was re-set will appear in front. This was already mentioned in a comment in fixup_window_levels().

Lastly, when the level of a window changes we should make sure the stacking is correct for modal and non-modal windows, this is done using fixup_window_levels() on other places in the code, we should do the same in fullscreen_x().

Setting the level of a window again to the same level will put it in
front of other windows on that level. Within each level it's the ordered
index which decides the stacking.
The fullscreen_x() function can also be called for windows that are not
receiving keyboard events (i.e. not in focus). In the cases where the
window isn't keyWindow we should set the level to NSNormalWindowLevel.
For keyWindows it's proper to set NSStatusWindowLevel like before.
When changing levels we need to make sure popup menus are properly
stacked afterwards.
@samhed
Copy link
Author

samhed commented Oct 5, 2021

I don't have the possiblity to test the master branch right now, but from a quick glance, it seems that code has the same issues?

@Albrecht-S
Copy link
Member

Thanks for sending this PR. I'm not a macOS programmer (i.e. I don't know what's going on here) but anyway: I wanted to let you know that we do no longer accept patches (PR's) for branch-1.3 (with rare exceptions for really serious bugs like crashes).

Our macOS specialist @ManoloFLTK may chime in and tell you more about this topic. I can't tell if he will be able to apply your PR to 1.4 but I suggest to wait for a while before you may try to test 1.4 and maybe create another PR?

That said, I see this is related to TigerVNC. Does TigerVNC still build with 1.3.x (only) or can you use 1.4 as well?

@ManoloFLTK
Copy link
Contributor

As Albrecht explained, we no longer patch FLTK 1.3.
Your proposed changes are now in the main branch and will appear in FLTK 1.4.
Thanks for this input.
Please review this change and write here if you believe this issue can be closed.

@samhed
Copy link
Author

samhed commented Oct 6, 2021

Hi @Albrecht-S and @ManoloFLTK, thanks for your quick responses.

Ok, I understand, with regards to branch-1.3. Thanks for applying the fix to master. I would have liked to be listed as author for the changes seeing as it took me quite a while to get to the bottom of this 😅 but it's OK, don't worry about it. I'm glad you accepted the fixes.

Since FLTK 1.4 isn't available in the distros yet, I'm afraid TigerVNC has not made any attempts at building with 1.4. Until a release of 1.4 is available this will likely continue to be the case. Are you guys getting closer to a release? 🙏

@ManoloFLTK
Copy link
Contributor

Your name appears now as author of the code change.

@ManoloFLTK ManoloFLTK closed this Oct 6, 2021
@Albrecht-S
Copy link
Member

Since FLTK 1.4 isn't available in the distros yet, I'm afraid TigerVNC has not made any attempts at building with 1.4. Until a release of 1.4 is available this will likely continue to be the case.

Thanks for this insight.

Are you guys getting closer to a release?

My hope is that we can release 1.4.0 by the end of this year or in Jan/Feb 2022, but that's just a guess. There's still a lot to do and we don't have a concrete schedule yet.

@CendioOssman
Copy link
Contributor

We seem to be pushing the boundaries of FLTK with TigerVNC so a bit of a heads up would be appreciated. So some time for us to test an alpha or beta once a release is on its way would be useful. :)

@samhed samhed deleted the branch-1.3 branch October 6, 2021 11:57
@Albrecht-S
Copy link
Member

Sure, once we can "see light at the end of the tunnel" we'll post some kind of schedule somewhere (maybe in fltk.coredev, fltk.general, on our website, etc.).

I'll also try to update the GitHub Milestones accordingly (this is still experimental, currently milestones are not yet maintained actively).

@CendioOssman
Copy link
Contributor

It doesn't seem like this got included in the 1.3.8 release unfortunately. :/

Could this be added to the 1.3-branch now, in case there will be a 1.3.9? I see that there is already a commit post 1.3.8 on that branch.

@ManoloFLTK
Copy link
Contributor

ManoloFLTK commented Dec 27, 2021

@CendioOssman and To TigerVNC people: could you, please, try the attached patch for FLTK branch 1.3 that intends to fix window levels.
That patch includes the bit missing in 1.3.8 and also fixes "BUG: Long menus on MacOS running off screen" that just appeared in fltk.coredev. It also attempts to simplify things using 4 fixed levels for FLTK windows which are, in increasing stacking order :

  • normal or non-key fullscreen windows;
  • fullscreen, key windows;
  • non-modal windows;
  • modal windows.

The dock level is beween that for normal and that for fullscreen, key.

I will commit that to branches 1.3 and 1.4 if I know it's OK for TigerVNC.

winlevels.patch.txt

@ManoloFLTK ManoloFLTK self-assigned this Dec 27, 2021
@CendioOssman
Copy link
Contributor

Sure, I'll try to have a look at it this week.

@CendioOssman
Copy link
Contributor

I'm afraid this patch does not work for us. Two major regressions right off the bat:

  • Status icons from the system menu is shown on top of the full-screen window. The system menu itself is hidden though.
  • When we grab the keyboard we have to raise the window level to the shielding level. With this patch menus no longer work as they are placed behind our window. This worked previously as the code examines the actual level of windows.

Tested on macOS 12.

Some other notes:

  • Why switch from AppKit constants to Core Graphics ones? Now there's just a mix which is probably more confusing.
  • Shouldn't a full-screen window start at a normal level since it isn't key window yet?
  • Changing the levels could be a problem for TigerVNC as we currently hard code what FLTK expects so we can restore things. Do you have some details on why the levels had to be changed?

@ManoloFLTK
Copy link
Contributor

I'm afraid this patch does not work for us. Two major regressions right off the bat:
Status icons from the system menu is shown on top of the full-screen window. The system menu itself is hidden though.

That's fixed in the attached modified patch

When we grab the keyboard we have to raise the window level to the shielding level. With this patch menus no longer work as they are placed behind our window. This worked previously as the code examines the actual level of windows.

What window is mentionned? What is the 'shielding level'? Do you mean that TigerVNC uses yet other levels than those FLTK uses? Please, explain in detail what you do in TigerVNC.

Why switch from AppKit constants to Core Graphics ones? Now there's just a mix which is probably more confusing.

Yes, I should replace NSNormalWindowLevel by kCGNormalWindowLevel. I stopped using the Appkit constants because they are confusing in the sense that the doc says they are listed in ascending order but that' s incorrect when you consider each constant value.

Shouldn't a full-screen window start at a normal level since it isn't key window yet?

Because a fullscreen window must cover the dock and the menu bar.

Changing the levels could be a problem for TigerVNC as we currently hard code what FLTK expects so we can restore things. Do you have some details on why the levels had to be changed?

I changed levels to fix "BUG: Long menus on MacOS running off screen" that just appeared in fltk.coredev. Menu windows needed a higher level to cover the dock.

winlevels.patch2.txt

@CendioOssman
Copy link
Contributor

I'm afraid this patch does not work for us. Two major regressions right off the bat:
Status icons from the system menu is shown on top of the full-screen window. The system menu itself is hidden though.

That's fixed in the attached modified patch

I can verify that this works now.

When we grab the keyboard we have to raise the window level to the shielding level. With this patch menus no longer work as they are placed behind our window. This worked previously as the code examines the actual level of windows.

What window is mentionned? What is the 'shielding level'? Do you mean that TigerVNC uses yet other levels than those FLTK uses? Please, explain in detail what you do in TigerVNC.

Yes. We use CGDisplayCapture() in order to grab the keyboard so we can get things like Cmd+Tab. As a side effect of this we're put on a special level called the "shielding level". You get it via CGShieldingWindowLevel() rather than a constant. The code you removed has those calls to take that in to account. Last time I checked that level was INT_MAX.

Shouldn't a full-screen window start at a normal level since it isn't key window yet?

Because a fullscreen window must cover the dock and the menu bar.

When it is key window, yes. But if it isn't then the code is supposed to have it at a normal level. Otherwise it might obscure the window that is actually key window.

Btw, this doesn't work properly in my testing for some reason. When I switch to another window the full-screen window remains on top, obscuring the window I'm trying to use. This doesn't seem to be a recent regression as I see it in our TigerVNC 1.11.0 build as well. It did work at some point though.

Changing the levels could be a problem for TigerVNC as we currently hard code what FLTK expects so we can restore things. Do you have some details on why the levels had to be changed?

I changed levels to fix "BUG: Long menus on MacOS running off screen" that just appeared in fltk.coredev. Menu windows needed a higher level to cover the dock.

I would suggest changing only that in that case and keep the code that checks other windows.

@CendioOssman
Copy link
Contributor

Btw, this doesn't work properly in my testing for some reason. When I switch to another window the full-screen window remains on top, obscuring the window I'm trying to use. This doesn't seem to be a recent regression as I see it in our TigerVNC 1.11.0 build as well. It did work at some point though.

It was broken by 25fc851. Might be a regression in newer versions of macOS. The issue seems to be that you always get nil for [NSApp keyWindow] if the window belongs to another process. So the level adjustment code in windowDidResignKey() never executes.

@CendioOssman
Copy link
Contributor

Another effect of that bug is that dialogs for the full-screen window get placed behind the full-screen window when it doesn't have focus, and then magically reappear when focus is regained. Very confusing.

@ManoloFLTK
Copy link
Contributor

OK. I attach a new patch that adds to 1.3 the code proposed at the beginning of this PR and that attempts to fix "BUG: Long menus on MacOS running off screen" by giving a higner level to modal windows (NSStatusWindowLevel instead of NSModalPanelWindowLevel). No other change.
@CendioOssman : could you, please, report if it's OK for TigerVNC?

There remains a disagreement about non-key, fullscreen windows. Either:

  • they should have NSNormalWindowLevel;
    or
  • they should stay above the dock and the menu bar.

I believe the 2nd view is correct because

  • important window content can lie behind the dock or the menu bar and thus be inacessible;
  • that's what happens with the Windows and X11 FLTK platforms;
  • STR 3268 explicitly complained that fullscreen windows at NSNormalWindowLevel are not adequate.

@CendioOssman : We need to find a consensus view about non-key fullscreen windows. WIth the present code, modal and non-modal windows are located above fullscreen windows, so they are visible when shown. You mention "dialogs for the full-screen window get placed behind the full-screen window when it doesn't have focus". Does that mean that you use dialog windows that are neither modal nor non-modal? Would it be possible to set these dialogs either modal or non-modal? I would also need to know whether TigerVNC runs several fullscreen windows comtemporaneously.

winlevels.patch3.txt

@CendioOssman
Copy link
Contributor

@CendioOssman : could you, please, report if it's OK for TigerVNC?

Yes, this patch seems to work correctly with TigerVNC. At least as far as not breaking anything that isn't broken before. It also makes TigerVNC work even when I remove our workaround for this issue. So the new fix in FLTK seems to work as intended.

There remains a disagreement about non-key, fullscreen windows. Either:

  • they should have NSNormalWindowLevel;
    or

  • they should stay above the dock and the menu bar.

I believe 1 is the only practical choice. If we choose 1. then we get some issues with the window that isn't focused. I.e. the window that isn't used. That is mostly cosmetic. With 2. we get the problem that I cannot do Cmd+Tab and use another application. At all. That's a major practical issue that should trump any cosmetic issues.

Also, the code currently does 1. with a bunch of bugs. Not 2. I can still end up in situations where the full-screen window is behind the dock.

  • that's what happens with the Windows and X11 FLTK platforms;

Only partially true. I'd rather say mostly false even. On Windows and X11 focused windows will be shown in front of the full-screen window. Even the taskbar can be shown in front of the full-screen window on Windows if you explicitly select it, e.g. by pressing the Windows key.

You mention "dialogs for the full-screen window get placed behind the full-screen window when it doesn't have focus". Does that mean that you use dialog windows that are neither modal nor non-modal?

Nope. It's a modal dialog. It is shown on top of the full-screen window as long as it has focus. If I switch to another application the modal dialog is sorted behind the full-screen window.

I would also need to know whether TigerVNC runs several fullscreen windows comtemporaneously.

Not for a single connection, no. But a user can open multiple connections. Each full-screen window would then be different processes.

We might have multiple windows in the future though in order to deal with the multiple spaces setting.

ManoloFLTK added a commit that referenced this pull request Dec 29, 2021
ManoloFLTK added a commit that referenced this pull request Dec 29, 2021
@ManoloFLTK
Copy link
Contributor

OK. I have committed to branches 1.3 (bc2f1f6) and master (1.4 - eeb3e92) the changes discussed here where non-key fullscreen windows return to the normal level in order to preserve the possibility to change app with command-tab. This also fixes the issue about long menu windows being below the dock.
@CendioOssman : please, report if there are still problems. Thanks for your detailed explanations about TigerVNC expectations.

@CendioOssman
Copy link
Contributor

Great, thanks!

Everything seems to work fine now. Even the issue with the modal dialog being placed behind the full-screen window when focus was lost is resolved. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants