-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Only show cross-hairs when terminal has focus #1772
Conversation
@whydoubt This is not working for me, I dont get crosshair cursor at all now for column selection. (Ubuntu 16, tested with Firefox and Chrome) Any clue whats going on? |
No, I don't understand what the problem might be. All of these worked for me:
Using dev tools to watch the classes applied to the child of terminal-container, you should see 'focus' added if you click on the terminal and removed if you click outside. When focused, pressing and releasing the Alt key should add and remove 'column-select'. Alt-Tab should add 'column-select' on Alt, then remove 'focus' on Tab. Cross-hairs should be visible when and only when both 'focus' and 'column-select' are applied. FWIW, with Chrome on Windows 7 I did find that the cursor would not change until I moved the mouse. |
Ah yepp my bad, the css file was not refreshed. It kinda partially solves the issue #1767 - after ALT+TAB with a different focus window in between it works, but it still shows the crosshair if I tab through all windows back to Chrome. Then I have to click in Chrome outside of the terminal div to get rid of the crosshair. I think thats what was meant by the issue? Any idea how to solve this? (Might also be windowmanager dependent, using xfce here.) |
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.
Hmm, I'm not so sure we want to rely on focus here. Take VS Code as an example, if you have the editor focused and hold alt, the crosshair should still show up. I think a better fix might be to clear the class when the document blur event fires?
@@ -139,7 +139,7 @@ | |||
cursor: pointer; | |||
} | |||
|
|||
.xterm.xterm-cursor-crosshair { |
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.
xterm-cursor-crosshair
aligns with the naming of xterm-cursor-pointer
, not sure we want to change this?
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.
The current alignment of the names re-enforces the false impression that the two are strongly correlated, and is part of why the name should change. At some point xterm-cursor-pointer
should be replaced with something more semantic as well (maybe something like linkified
).
@Tyriar : not sure what your point regarding VS Code is, because if I understand you correctly, it does not act that way now, and changing it would depend on changing VS Code. Regarding clearing the |
@whydoubt Yepp the classes are applied correctly for tabbing in and out with a different focused window in between, but not if I tab through all right back to chrome. As I said Im not sure if thats fixable at all as it might be related to the way the window manager handles/steals focus. |
@jerch It appears to affect all browsers I have on Fedora—with GNOME Shell handling window management. I see that a keydown event for the first Alt is received, but no further events. The window manager ostensibly swallows all further keyups/keydowns, and no focus/blur events get fired either. There may not be a solution to this one, unless we can find some event that does get fired. |
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.
not sure what your point regarding VS Code is, because if I understand you correctly, it does not act that way now
Opps, you're right 😅. While this is certainly not perfect, this definitely improves the situation and fixes the underlying bug. Thanks @whydoubt!
@whydoubt Yeah I think the situation with the window manager cannot be improved by normal means here (means from within the browser). So the partial solution is good to go. |
Fixes #1767