Skip to content

Restore and fix macOS dark text selection cursor - #96947

Merged
Alexandru Dima (alexdima) merged 5 commits into
microsoft:masterfrom
kdrag0n:macos-dark-cursor
May 12, 2020
Merged

Alexandru Dima (alexdima) merged 5 commits into
microsoft:masterfrom
kdrag0n:macos-dark-cursor

Conversation

@kdrag0n

Copy link
Copy Markdown
Contributor

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:

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

Microsoft Contribution License Agreements (msftclas) commented May 4, 2020 •

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@alexdima Alexandru Dima (alexdima) left a comment

Copy link
Copy Markdown
Member

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 Isidor Nikolic (@isidorn) for a small change to adopt in the repl
cc Peng Lyu (@rebornix) for the change to adopt in the comments thread

@@ -59,10 +59,6 @@
max-width: 150px;
}

Copy link
Copy Markdown

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isidor Nikolic (@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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
Member

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

@isidorn

Copy link
Copy Markdown

Good point, thanks.

@kdrag0n

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

Peng Lyu (@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
Alexandru Dima (alexdima) merged commit 645f585 into microsoft:master May 12, 2020
@rebornix

Copy link
Copy Markdown
Contributor

Alexandru Dima (@alexdima) thanks for the ping, it looks good to me!

@kdrag0n
Danny Lin (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

Development

Successfully merging this pull request may close these issues.

Text selection cursor is hard to see on older versions of macOS Blinking cursor outline when selecting with mouse

5 participants