[release/3.0] Context Menus are sometimes not shown in High-DPI applications #2099
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…8 in), a change was made to `Popup` that involved the destruction and recreation of the underlying `HWND`. This was done to ensure that the `HWND` was always created with the correct monitor (~=DPI) affinity.
In order to acheive this, `DestroyWindow()` and `BuildWindow()` - private methods in `Popup` - were used.
We are finding that `DestroyWindow()` has side-effects that can lead to incorrect behavior. The incorrect beahvior works as follows:
- The call into `Popup.CreateWindow` is usually a consequence `Popup.IsOpenChanged` (`false -> true`)
- Within `Popup.CreateWindow`, `DestroyWindow()` is called (when high-dpi mode is detected).
- `DestroyWindow()` destroys the underlying `HWND`, releases the capture, raises the *OnClosed* event and clears the placement-target.
- At the end of `DestroyWindow()`, we get `IsOpen == false`.
- After `DestroyWindow()`, we call into `BuildWindow()` and `CreateNewPopupRoot()` etc., which go on to build the `Popup` again (and also instnatiate a new `HWND`).
- Unfortunately, there is no mechanism here for resetting `IsOpen` back to `true` (without also leading to an undesirable infinite-recursion that calls back into `CreateWindow`).
If these calls to show the context-menu arise from `ContextMenu.OnIsOpenChanged`, and flow through `ContextMenu.HookupParentPopup -> Popup.CreateRootPopup`, then `IsOpen` gets reset (there is a direct call into `SetBinding(IsOpenProperty)`, in `Popup.CreateRootPopupInternal`) and the popup is shown correctly. Until then, the context-menu is "stuck" not being able to be shown.
The solution is to stop using `DestroyWindow()` as-is, which does more than what we need for it to accomplish. Our original intent in calling `DestroyWindow()` was simply destroy and recreate the `HWND`. This fix refactors `DestroyWindow()` to suit this need and uses the newly introduced `DestroyWindowImpl()` to destroy the `HWND`, and then recreate just that. The rest of the state is retained intact, and the `Popup/ContextMenu` continues to function well as before.
ghost
requested a review
from 
ghost
added
the
vatsan-madhavan
added
* NO MERGE *
arpitmathur
approved these changes
Oct 24, 2019
rladuca
approved these changes
Oct 24, 2019
|
We are not forward porting .NET Framework fixes to release/3.0 - this will go into release/3.1 only. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #2088
.NET 5 PR: #2097
.NET Core 3.1 PR: #2098
Description (Summary)
As part of a previous fix (that shipped originally as part of .NET 4.8 in), a change was made to
Popupthat involved the destruction and recreation of the underlyingHWND. This was done to ensure that theHWNDwas always created with the correct monitor (~=DPI) affinity.This previous fix depended upon a private helper-method (
Popup.DestroyWindow) that had side-effects that were not accounted for in the original fix. One of the side effects is that theContextMenuis not sown consistently.The solution modifies (refactors) the private method to extract the useful portion and uses it to improve the previous fix.
Customer Impact
This is a forward-port from from .NET 4.8. This was reported by a customer, and has also been discovered by Visual Studio internally.
Regression
Regression introduced by .NET 4.8. .NET Core 3.0 shipped with this bug.
Risk
Low - this has been well tested internally and validated by multiple customers. The fix is well understood and small/scoped.
Details
As part of a previous fix (that shipped originally as part of .NET 4.8 in), a change was made to
Popupthat involved the destruction and recreation of the underlyingHWND. This was done to ensure that theHWNDwas always created with the correct monitor (~=DPI) affinity.In order to acheive this,
DestroyWindow()andBuildWindow()- private methods inPopup- were used.We are finding that
DestroyWindow()has side-effects that can lead to incorrect behavior. The incorrect beahvior works as follows:Popup.CreateWindowis usually a consequencePopup.IsOpenChanged(false -> true)Popup.CreateWindow,DestroyWindow()is called (when high-dpi mode is detected).DestroyWindow()destroys the underlyingHWND, releases the capture, raises the OnClosed event and clears the placement-target.DestroyWindow(), we getIsOpen == false.DestroyWindow(), we call intoBuildWindow()andCreateNewPopupRoot()etc., which go on to build thePopupagain (and also instnatiate a newHWND).IsOpenback totrue(without also leading to an undesirable infinite-recursion that calls back intoCreateWindow).If these calls to show the context-menu arise from
ContextMenu.OnIsOpenChanged, and flow throughContextMenu.HookupParentPopup -> Popup.CreateRootPopup, thenIsOpengets reset (there is a direct call intoSetBinding(IsOpenProperty), inPopup.CreateRootPopupInternal) and the popup is shown correctly. Until then, the context-menu is "stuck" not being able to be shown.The solution is to stop using
DestroyWindow()as-is, which does more than what we need for it to accomplish. Our original intent in callingDestroyWindow()was simply destroy and recreate theHWND. This fix refactorsDestroyWindow()to suit this need and uses the newly introducedDestroyWindowImpl()to destroy theHWND, and then recreate just that. The rest of the state is retained intact, and thePopup/ContextMenucontinues to function well as before.