Skip to content
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

Restore and fix macOS dark text selection cursor #96947

Merged
merged 5 commits into from
May 12, 2020
Merged

Restore and fix macOS dark text selection cursor #96947

merged 5 commits into from
May 12, 2020

Conversation

kdrag0n
Copy link
Contributor

@kdrag0n kdrag0n commented May 4, 2020

This PR fixes #96946 by restoring the old cursor for all macOS versions, because even on macOS Catalina, the old special cursor is easier on the eyes when there's a dark background. The continued use of a special cursor can be seen in other apps as well, such as iTerm 2.

The cursor was originally removed in a915df1 to fix #94144, but removing it entirely isn't necessary. This PR fixes the inconsistency issues exhibited in the old implementation of the special cursor by adding a shared mouse-text class to all editor-related elements that need a text selection cursor in their entire bounding box and applying the special cursor to that class rather than just .monaco-editor .view-lines.

The custom dark cursor is now consistent in the following areas:

kdrag0n added 3 commits May 4, 2020 15:43
This reverts commit a915df1.

The special macOS dark cursor is still useful for users stuck on older
versions of macOS. Even on macOS Catalina, this cursor is helpful for
reducing visual bloat.
This reduces code duplication and makes it significantly easier to make
changes to the text cursor without having to individually modify several
files.
Now that there's a common mouse-text class for text cursors, we can
apply the special macOS dark cursor to many elements at once to improve
consistency.

Since the cursor is now applied to all relevant elements, this fixes
#94144 without needing to revert the cursor entirely.
@msftclas
Copy link

msftclas commented May 4, 2020

CLA assistant check
All CLA requirements met.

@alexdima alexdima requested a review from isidorn May 11, 2020 13:47
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved some things around, to get this working also for the standalone editor. The changes in the editor LGTM.

cc @isidorn for a small change to adopt in the repl
cc @rebornix for the change to adopt in the comments thread

@alexdima alexdima added this to the May 2020 milestone May 11, 2020
@alexdima alexdima requested a review from rebornix May 11, 2020 13:48
@@ -59,10 +59,6 @@
max-width: 150px;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should not be removed. This controls the looks of the cursor inside the debug console tree and is unrelated to this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isidorn That is the entire point of this PR, to change how the mouse cursor looks like on mac, but it goes even outside of the editor and adopts the custom cursor that the editor had also in the repl and in the comments thread.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I see that the class is set on the repl-tree.
I misunderstood the PR that it changes cursor only in the editor and all the editor instances like the repl input.
So I take my original comment back.

@isidorn
Copy link
Contributor

isidorn commented May 11, 2020

I have added one comment which I think should be tackled. Otherwise the change in repl.ts looks good.
I do not understand why is a change in textAreaHandler needed? This textArea is not visible.

@alexdima
Copy link
Member

The textarea can become visible when doing input composition, like e.g. typing Japanese text.

@isidorn
Copy link
Contributor

isidorn commented May 11, 2020

Good point, thanks.

@kdrag0n
Copy link
Contributor Author

kdrag0n commented May 12, 2020

I wasn't actually thinking of IMEs when I added the text cursor class to the textarea — it was because it's one of the elements causing #94144 (the other element being the cursor div). It's not necessarily visible, but there's still a 1 px mouse hover area that it occupies, so the cursor changes when moving over the textarea.

@alexdima alexdima mentioned this pull request May 12, 2020
@alexdima
Copy link
Member

@rebornix I have tried this locally and the changes in the comment thread look good to me, I think it makes sense to have the same I cursor everywhere, in the editor, and in the comment body/title/reply button.

I will optimistically merge, since this looks good and people are complaining (e.g. #97550) and I'd like to have a fix out in Insiders.

@alexdima alexdima merged commit 645f585 into microsoft:master May 12, 2020
@rebornix
Copy link
Member

@alexdima thanks for the ping, it looks good to me!

@kdrag0n kdrag0n deleted the macos-dark-cursor branch May 13, 2020 05:41
@github-actions github-actions bot locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants