microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
162.97k stars 28.78k forks source link

New EditContext composing character(s) aren't styled well #229645

Closed Tyriar closed 6 hours ago

Tyriar commented 5 days ago

Testing #229383

On Dark Modern, this doesn't look great:

Image

This is what it looked like before:

Image

Neither looks great but I think we own this styling with EditContext? If so we should do a pass over it. Some ideas:

These are common problems with IMEs that users are used to, but we might be able to handle them better. For example Naver (SK's main search engine) has a built in IME and they have no background change or underline for that:

Image

anthonykim1 commented 5 days ago

+1 strongly agree with Screenshot 2024-09-24 at 5 43 53 PM Awkward use of underline like this can really trick people to assume some character to appear to be a different character.

I would also prefer no underline at all. I'd be happy to provide more perspective when we eventually do something about this.

Tyriar commented 5 days ago

Another reason to not go with underline is it's messy if there's a problem on the character:

Image Image

Tyriar commented 5 days ago

@rebornix @justschen thoughts on the best styling for composition?

justschen commented 4 days ago

@Tyriar iirc, there aren't that many characters in chinese that will look like having underlines (maybe 二 or 三 or 王).

apple will style the underline based on what app/background you're in (example below, in messages, it's blue)

https://github.com/user-attachments/assets/f3ab7ada-fbc2-4729-a8db-3142a386716a

I think to be language agnostic, a noticeable background change is probably the move, although it does kind of differ from how some other companies/apps/editors do composition, but it's a small text box and I'm sure people will understand if it's just the background change perhaps.

rebornix commented 4 days ago

Chinese Pinyin uses ANSII characters for composition so the Chinese characters will only show up in the editor on commit, thus it's fine if we use an underline. While Zhuyin, as @justschen showed above, is similar to Korean, it can be confusing to use underline with phonetic radicals, a background is probably better.

aiday-mar commented 4 days ago

Hi I see thanks a lot for the input. I will work on removing the underline and instead add a background color to the composition.

aiday-mar commented 4 days ago

Made the following PR to address this https://github.com/microsoft/vscode/pull/229806/files. I am not sure about the choice of background colors for composition, so pinging @daviddossett about this.

The EditContext text-format update event, sends as one of the fields an underline thickness, which can either be: none, thin or thick. From looking at the compositions, it would seem that thin corresponds to the secondary (bigger composition) and thin corresponds to the main or active sub composition within a phrase/word.

I chose the following colors:

thin -> vscode-editor-selectionBackground thick -> vscode-editor-selectionHighlightBackground

On composition in Japanese, this looks as follows:

https://github.com/user-attachments/assets/0c7c2d13-2d2e-444f-a4f9-466d8725a0d3

Perhaps the background colors need to be reworked.

Tyriar commented 4 days ago

@aiday-mar that looks too much like selection. In previous versions, at least on macOS that I was testing at the time it uses greyscale which will probably look better in this case imo:

Image

Something else I was thinking about was to use border-bottom instead of text-decoration: underline, that might look good?

aiday-mar commented 4 days ago

Hi @Tyriar so by grayscale you mean the background would be black and the composition text would be white? But if the rest of the text is colored, would this maybe look out of place?

I could add border-bottom if you think that would not be confusing with certain characters

Tyriar commented 4 days ago

@aiday-mar I mean lightish grey on Light Modern and darkish grey on Dark Modern. Similar luminance to the selection blue but it would not have color to differentiate itself.

I could add border-bottom if you think that would not be confusing with certain characters

I'd need to see it, maybe it'll look awful 😅

aiday-mar commented 3 days ago

Right I see, so I have the following without the border with greyscale colors:

https://github.com/user-attachments/assets/5df1b803-f777-4de3-b2ed-d6ac35211745

And the following with the border with greyscale colors. Here the border is just made a solid white or black depending on the theme:

https://github.com/user-attachments/assets/ac9c2157-9267-4e6f-a42c-9de7938a85ec

Just noticed one other thing I must do. For the High Contrast Light and High Contrast Dark themes, if we decide to change the background color, I will set the composition background color to a dark grey for Light Contrast and close to white for Dark Contrast, and will change the color of the text so it is contrasting, in the same way we do when we do selection.

Tyriar commented 3 days ago

I quite like border bottom actually. Also can you do it darker in Dark modern, similar to how it used to be? Thoughts @rebornix?

aiday-mar commented 3 days ago

Slightly darker color (#424242)

https://github.com/user-attachments/assets/3d2de344-30cd-4472-9bb1-4fd2c0f61cf0

Or even darker (#3d3d3d)

https://github.com/user-attachments/assets/ce55734c-3ad8-4a16-87fc-6353685ea37b

Tyriar commented 3 days ago

@aiday-mar I meant darker than the background, not lighter than it. Since it's a special state, we want to emphasize it (increase contrast), rather than make it more subtle (lower contrast) imo.

aiday-mar commented 3 days ago

Right so I made the composition background completely black on the following demo. Since the background itself is really dark, I think it's a bit hard to see the background is different, although I would agree this makes the words more contrasting.

Generally in the current proposal on the light theme the composition background is darker than the editor background so contrast is lower. I am therefore wondering if it would be better to do the same for consistency on the dark theme, that the composition background is lighter than the editor background, even if contrast is lower.

https://github.com/user-attachments/assets/4e86f7bf-b405-45e3-bc47-69e539c5d345

Tyriar commented 3 days ago

@aiday-mar hmm, maybe just border will be good enough? It fixes the issue with potentially confusing characters imo as it's lower and more readable than it was initially. We could also have the background color as themeable to leave it up to themes.

aiday-mar commented 3 days ago

What do others think on this thread? I may do a poll on the UX channel on this slack to come to a consensus

Tyriar commented 2 hours ago

@aiday-mar most theme authors wouldn't know what the theme key descriptions "The border color for the composition." means. Could you add "IME" to that?

aiday-mar commented 2 hours ago

Will do thanks.