Fix #670: Terminal selects a char when bringing window to foreground#856
Fix #670: Terminal selects a char when bringing window to foreground#856DHowett-MSFT merged 3 commits intomicrosoft:masterfrom mblowey:master
Conversation
…he terminal window from selecting text.
| std::unique_ptr<::Microsoft::Console::Render::DxEngine> _renderEngine; | ||
|
|
||
| Settings::IControlSettings _settings; | ||
| bool _focused; |
There was a problem hiding this comment.
bool _focused; [](start = 8, length = 14)
Could we use _controlRoot.FocusState()? Click here for the doc
There was a problem hiding this comment.
So I did look into _controlRoot.FocusState(), and the value returned when inside MouseClick event handler is never FocusState::Unfocused. If I had to guess, _controlRoot's focus state is probably changed sometime between the GettingFocus event and the MouseClick event.
There was a problem hiding this comment.
Well that sucks. Thanks for checking.
zadjii-msft
left a comment
zadjii-msft
left a comment
There was a problem hiding this comment.
Seems good to me. I'm no XAML wizard, but that feels like it should work. I do echo Carlos's question re: _controlRoot.FocusState - I'd rather not duplicate state we can look up
| _cursorTimer = std::nullopt; | ||
| } | ||
|
|
||
| _controlRoot.GotFocus({ this, &TermControl::_GotFocusHandler }); |
There was a problem hiding this comment.
I'd want to keep this change even if we can use _controlRoot.FocusState (as Carlos suggested)
| { | ||
| args.Handled(true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Just noticed that in CMD/Powershell (outside of WT), an unfocused window can still be scrolled w/ touch. Since we're exiting before line 604, I suspect we won't be able to do that here. Could you double check and fix if that's the case?
There was a problem hiding this comment.
It took me a bit to get the touch simulation working, but I have confirmed that this is the case. A touch and drag will scroll the original CMD and PowerShell terminals without focusing them.
With the focus check at the top of the MouseClick event handler, a touch and drag does not scroll and the window is incorrectly focused.
Moving the focus check inside the PointerDeviceType() == Mouse check does enable scrolling, but the window is still focused.
It looks like a slightly more complicated solution will be necessary. I'll start looking into it tonight.
There was a problem hiding this comment.
I started digging into the issue touch focus/scrolling issue, but I will not have time to properly fix it. I will be out of town for a week starting tomorrow and will be unable to commit any time to this PR.
I did confirm that the touch and drag issue was present before the changes of this PR, and have made a small change that returns the behavior of a touch and drag event to how it originally was while still fixing #670 (in regards to mouse click events).
I'm more than happy to open a new issue for the touch and drag problem. When I return I will be able to devote the time to resolve it. In the mean time, I would hate to see this PR remain open and stagnant for a week because of an already present bug. However it is not my repo, so I'll defer to your preferences.
There was a problem hiding this comment.
Don't worry about it. :) Thanks for looking into it. I'll put an issue in (if there isn't one already) and let's get the PR in!
There was a problem hiding this comment.
So, I have a Surface Pro and I've been comparing what your PR has with an admin Powershell instance. I think they're both acting the same way with regards to providing focus. I don't think we'll need an issue for this, then.
…se specific block of code. This lets any touch and drag events scroll the terminal's contents.
|
My primary concern here is the focus state getting out of sync across window transitions, but I have to trust that the TermControl loses focus when the window loses focus. For our UI toolkit to do otherwise would be utter insanity. |
|
Thanks for the contribution! |
4 participants
Summary of the Pull Request
This fix prevents text from being selected when the user clicks within the active terminal to re-focus the window. This brings the new terminal's behavior inline with the original powershell and cmd terminals.
Fixes #670
I tested this fix's behavior manually, but did ensure all unit tests are passing.
PR Checklist
Detailed Description of the Pull Request / Additional comments
The proposed changes may be a naive way of fixing the issue. I simply added a bool called _focused to the TermControl class which is set and unset during the respective GotFocus and LostFocus events. That bool is then checked whenever a new MouseClick event is encountered. If _focused is false during the MouseClick event, the MouseClick event handler immediately returns instead of handling the event normally.
I did try to fix this issue using the GettingFocus event, but I could not find a way to stop the MouseClick event from the GettingFocus event handler. I am pretty new to UWP events, so I am more than happy to revise this fix if a better method exists.