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 issues with positioning of non-overlay popups #7071

Merged
merged 2 commits into from
Dec 6, 2021

Conversation

simonhaines
Copy link
Contributor

What does the pull request do?

Native popup windows appear 'detached' when anchored to a placement target on the main window and the main window is moved or resized. This change repositions open popups to correctly align with their expected placement.

What is the current behavior?

Native popups don't 'follow' the placement target. Similarly, popups that are anchored to other popups (sub-menus) don't follow the parent popup when it moves.

What is the updated/expected behavior with this PR?

This fix adds listeners for events that change the screen coordinates of popups and re-positions them accordingly. Window mode changes (minimize/maximize) are handled the same as window position changes.

Checklist

Breaking changes

None

Obsoletions / Deprecations

None

Fixed issues

Fixes #5890
Fixes #4225

When a native popup window is positioned relative to a placement target on
the root window and the root window is moved or resized, the popup position
should update to reflect the new position of the placement target.

If a native popup window is positioned relative to another popup, as in the
case of sub-menus, and the parent popup is moved due to a change in the
position of its placement target, the child popup should also update
relative to the new position of the parent popup.
@maxkatz6
Copy link
Member

maxkatz6 commented Dec 5, 2021

Tested with my small project to reproduce old issue (you can find here #4630)
Works way better now! Thanks!

But there is still couple of issues:

  1. When window is losing focus and goes to the background, popup is still visible topmost (keeping in mind, that this popup doesn't have Topmost=True set.
  2. When another window is moved over the popup, it goes behind it

I guess both issues are related to the fact that on windows for some reason this popup is configured as topmost anyway. Probably unrelated to this PR, because it's platform specific.

fxdvb

@simonhaines
Copy link
Contributor Author

Thanks @maxkatz6, yes this PR does not address issue #4630 (the z-order part of it anyway) but there is no reason why it shouldn't. I'll look into it and update the PR.

@maxkatz6
Copy link
Member

maxkatz6 commented Dec 5, 2021

@simonhaines I think this PR can be merged as is. It looks good to me, but I will ask for additional review.
Windows OS specific changes should be done in another PR. So far it looks like removing WS_EX_TOPMOST from popup window initialization solves this issue, but bigger question is why it was added there before. I am not familiar with deep win32 tricks.

@maxkatz6
Copy link
Member

maxkatz6 commented Dec 5, 2021

Couple of more observations.

  1. WPF is horrible at it. It has the same issues with no updating popup position on window moved. And it also sets TOPMOST if popup has StaysOpen=true. So, all non-dimissable popups are topmost, no choise. Why?
  2. In UWP it's opposite (in a better way). By default, popups are part of overlay layer, but it can be disabled with ShouldConstrainToRootBounds=false. After that popup is displayed in a new native window, moved together with parent (slighly delayed, but ok), and doesn't have WS_EX_TOPMOST style, so it's hiding and restoring with parent window. Looks good, Furthermore, they clip this popup with parent bounds, I even had to double check it with Spy++.
  3. Chromium seems to not set TOPMOST for its popups https://github.com/chromium/chromium/blob/main/ui/views/widget/widget_hwnd_utils.cc#L25-L129

I haven't tested this PR with macOS. But I remember it doesn't have this zindex/topmost problem, but has position syncronization problem (which expected to be fixed with this PR). No idea about linux.

@maxkatz6 maxkatz6 merged commit 4eaba20 into AvaloniaUI:master Dec 6, 2021
@simonhaines
Copy link
Contributor Author

Thanks @maxkatz6, I agree that platform-specific changes should be resolved in a separate PR.

I can confirm the z-order issue occurs on Linux as well. As for Windows, it looks like the WS_EX_TOPMOST has been there since its initial commit. Since the documentation states:

The window should be placed above all non-topmost windows and should stay above them, even when the window is deactivated.

I'm pretty sure this is not the intended behaviour and it was added in error. I'll take it out and submit a new PR with the Linux fix in it as well for review, and see how it goes.

@maxkatz6
Copy link
Member

maxkatz6 commented Dec 6, 2021

@simonhaines I tested it with macos, and apparently I was wrong. There also a problem with ZIndex. Created PR for that #7076

@grokys
Copy link
Member

grokys commented Feb 17, 2022

This seems to have broken positioning of DatePicker/TimePicker popups: #7633

grokys added a commit that referenced this pull request Feb 17, 2022
Was broken by #7071 - `DatePicker` was doing a manual position configuration by calling `ConfigurePosition`. When the popup is shown, `Popup` gets a position change, and then re-calls `ConfigurePosition` with the settings in the popup, which are different to those passed during the manual configuration.

Change this to set properties on the `Popup` so that when the popup position is changed, the correct parameters are passed to `ConfigurePosition` by `Popup`.

Fixes #7633
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants