-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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 cursor hiding on input #18445
Fix cursor hiding on input #18445
Conversation
@@ -143,7 +147,7 @@ class IslandWindow : | |||
bool _minimizeToNotificationArea{ false }; | |||
|
|||
std::unordered_map<UINT, SystemMenuItemInfo> _systemMenuItems; | |||
UINT _systemMenuNextItemId; | |||
UINT _systemMenuNextItemId = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This member was left uninitialized.
|
||
void IslandWindow::ShowCursorMaybe(const UINT message) noexcept | ||
{ | ||
if (_cursorHidden && (message == WM_ACTIVATE || message == WM_POINTERUPDATE)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need one for losing focus or "switching to other window"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we hide the cursor only when we are active? (so that somebody sending us a WM_KEYDOWN remotely doesn't hide the cursor?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My assumption was that WM_ACTIVATE is a superset of WM_SETFOCUS. Is that not the case? (Not sure how to test that because I always focus a window by activating it…)
I think that hiding the cursor on an external/emulated WM_KEYDOWN is fine since the sender must've had a reason to send WM_KEYDOWN specifically and I think it should behave like any other keyboard input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough
@@ -417,6 +417,16 @@ void WindowEmperor::HandleCommandlineArgs(int nCmdShow) | |||
_dispatchSpecialKey(msg); | |||
continue; | |||
} | |||
|
|||
if (msg.message == WM_KEYDOWN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or syskeydown right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I missed this comment. I think it should not be done on SYSKEYDOWN since that's for Alt-key combinations, which also doesn't hide the cursor in classic applications.
The CoreWindow approach to implementing this has proven itself to be bug prone. This PR switches to using the much better Win32 ShowCursor API, which uses a reference count. This prevents the exact sort of race condition we have where we we disable the cursor in our code and the WinUI code then sets it to a different cursor internally which gets the system out of sync. There's no WinUI API to just hide the cursor and if it did, it would probably be a Boolean which would result in the same issue.
Closes #18400
Validation Steps Performed
It's difficult to assert the correctness of this approach, outside of just trying it out (which I did and it works). The good news is that this uses a static bool to ensure we only hide it exactly once and show it exactly once and we do the latter on every WM_ACTIVATE message which should hopefully restore the cursor when tabbing out and back in at least.