Fix TopMost handling for Popups on Windows #17841
Conversation
We have integration tests for Windows and macOS. But it might be challenging to setup. |
We can go safe and merge it into 11.3.0, but not backport into 11.2.x. |
|
This fixes a problem we discovered recently (did not raise an issue here yet, since we were unsure if we were using it wrongly). I would be really happy if this could find its way in the 11.2.X versions, though. |
| <TextBlock Text="'Light Dismiss' Popup" /> | ||
| <Button Content="Show Popup" Click="ButtonLightDismiss_OnClick" /> | ||
|
|
||
| <TextBlock Text="Popup that stays open" /> | ||
| <Button Content="Show Popup" Click="ButtonPopupStaysOpen_OnClick" /> | ||
|
|
||
| <TextBlock Text="TopMost popup that stays open" /> | ||
| <Button Content="Show Popup" Click="ButtonTopMostPopupStaysOpen" /> |
There was a problem hiding this comment.
These testing buttons can be moved to the IntegrationTestApp. I can try to add integration test on top of them later.
There was a problem hiding this comment.
I could use some guidance on this one. Not sure what I should do 😳
There was a problem hiding this comment.
I think the required integration tests are still blocking this from being merged. @maxkatz6, any pointers on how I can test this particular fix? I've never worked with and to be honest I can't make any sense of the existing tests to find out how I would go about this.
There was a problem hiding this comment.
@StefanKoell I just pushed popup integration tests. Based on your popup page. These work on Windows, need yet to check on macOS, but should be fine (if not, better to temporary skip them unless it's a new bug).
There was a problem hiding this comment.
@StefanKoell integrational tests are running an actual app (IntegrationTestApp project). So when there is a failing test, you usually can manually run this project and reproduce test steps.
There was a problem hiding this comment.
Just to add to this: it seems that roughly 1/3 of all the tests are green and the rest fails like shown in the screenshot.
There was a problem hiding this comment.
Interesting. Seems like some test is crashing testing app for you. Maybe you can try to isolate it?
There was a problem hiding this comment.
@maxkatz6 tried to isolate and it seems that for the popup tests, the click on the button doesn't open the popup at all. When I run the test app manually and click on the button, the popup opens up. In the test, I can confirm that the button is found and the click is executed but the popup never shows up.
I tested it on 2 different machines (both Windows 11 24H2 on ARM64). Most of the tests are behaving weird on those machines.
I tried WinAppDriver, last stable release and the "release candidate" which offers an ARM64 binary. Unfortunately both behave the same way.
As far as I can tell it is an issue with ARM64 but I can't really confirm that because I don't have access to a x64 machine at the moment.
I'm not sure what else I can do to finish up the PR. Any suggestions? Can someone else help out here?
On a positive side: I know now much more about the integration tests.
There was a problem hiding this comment.
@maxkatz6 I just pushed another commit which should fix the failed test - at least manually I can't reproduce the issue anymore with the grandchild menu item. The solution I came up with is not elegant, I guess. I'm simply delaying the popup activation which seems to be enough for the menu click handler to get processed. I'm not sure if this can be solved differently. What do you think?
There was a problem hiding this comment.
I'm still not sure what to do with the integration tests. I see also a lot of tests failing in the CI pipeline here. Is this expected?
|
You can test this PR using the following package version. |
… activate the app.
|
You can test this PR using the following package version. |
|
Hi @maxkatz6 I do see a difference in behavior though and I was wondering if there's a way to improve it:
Other than that, everything works really well. I hope this PR and the ShouldConstrainToRootBounds PR can be merged. If there's anything else from my side to do, let me know. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
@maxkatz6 You are probably swamped with the Accelerate stuff but I just wanted to check back if this is still on track for 11.3? If there's anything I can/need to do, let me know. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
| @@ -917,7 +917,7 @@ void PlatformImpl_LostFocus() | |||
| while (focused.VisualParent != null) | |||
| focused = focused.VisualParent; | |||
|
|
|||
| if (focused == this) | |||
There was a problem hiding this comment.
@maxkatz6 found another small issue with popups and input elements when the window is deactivated.
|
You can test this PR using the following package version. |
What does the pull request do?
This PR fixes how the Popup handles the TopMost property on Windows.
What is the current behavior?
All popups on Windows are created with the
WS_EX_TOPMOSTstyle. In most scenarios (withIsLightDismissEnabled = true) this is not a problem because the popup is hidden as soon as another window receives the focus.If
IsLightDismissEnabledis set tofalse(the popup needs to be closed using a user interaction on the popup), the behavior is not correct. Other windows will not cover that popup:What is the updated/expected behavior with this PR?
The Popup class has actually a property
TopMostwhich should be used to control the behavior. This PR fixes the Popup implementation on Windows to honor the value of the property:I also created a Popups page in the Control Catalog which can be used to test the change in this PR.
How was the solution implemented (if it's not obvious)?
I think the (small) changes needed to fix the behavior are self-explanatory. If not, I'm happy to elaborate.
Checklist
Not sure how this can be unit tested. Also no public APIs have been altered.
Breaking changes
The previous behavior didn't honor the
TopMostproperty at all - now it does. Since the property default value isfalse, this PR changes the default behavior and make all Popups NOT top most anymore. I can't really tell if this breaking change has an impact on built-in popups. The usual flyout/popups/tooltips I tested are working fine as far as I can tell.