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.

Sign up for GitHub

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

Jump to bottom

Card suite Unicode characters are the wrong width #5822

Closed
j4james opened this issue May 9, 2020 · 8 comments · Fixed by #5866
Closed

Card suite Unicode characters are the wrong width #5822

j4james opened this issue May 9, 2020 · 8 comments · Fixed by #5866
Assignees
@leonMSFT
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Impact-Compatibility Sure don't work like it used to. Impact-Correctness It be wrong. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.0

Comments

@j4james
Copy link
Collaborator

j4james commented May 9, 2020

Environment

Windows build number: Version 10.0.18362.657
Windows Terminal version (if applicable): Commit 7ae3433

Steps to reproduce

  1. Build commit 7ae3433
  2. Open a bash shell
  3. Execute printf "\u2660\u2663\u2665\u2666\n"

Expected behavior

This should print our the four card suite Unicode characters, each being one cell wide. Here's what it used to look like in conhost:

image

Actual behavior

Each characters occupies two cells now.

image

I wouldn't be too concerned if these were some recently invented emoji like crying kitten, or dancing monkey with umbrella. But these characters are from the default cmd shell code page (CP437), and they've been narrow characters for going on 40 years now. They're even included in the WGL4 character repertoire. Changing their width is almost guaranteed to break things.

I'm sorry I didn't bring this up before PR #5795 was merged, but I wasn't paying attention to the actual characters that were being changed. I only noticed now when I was testing some of my own code and found that this broke Vttest (it uses the diamond character in a couple of the tests).
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels May 9, 2020
@j4james
Copy link
Collaborator Author

j4james commented May 9, 2020

I've just been checking the rest of the CP437 characters, and there are few more that I think shouldn't be wide:

  • U+263A white smiling face
  • U+2640 female sign
  • U+2642 male sign

@DHowett-MSFT
Copy link
Contributor

Augh, we deserve this for our hubris. Thank you. 😁

@DHowett-MSFT
Copy link
Contributor

crying kitten, or dancing monkey with umbrella

(I'll have you know that I am still laughing at this.)

@DHowett-MSFT DHowett-MSFT added Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Impact-Compatibility Sure don't work like it used to. Impact-Correctness It be wrong. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels May 11, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 11, 2020
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone May 11, 2020
@DHowett-MSFT
Copy link
Contributor

I'm actually yanking this into the v1 milestone -- we broke it in the run-up to v1, we should darned well fix it for v1.

@leonMSFT
Copy link
Contributor

grr now I'm wondering what other glyphs have incorrect widths... like when printed inside of terminal, I'm getting the emoji presentation like so:
image

@DHowett-MSFT
Copy link
Contributor

This one looks like it's font-specific. Because Cascadia Code has those glyphs, I get:

image

@leonMSFT leonMSFT self-assigned this May 11, 2020
@j4james
Copy link
Collaborator Author

j4james commented May 11, 2020

Note that the white smiling face was supposedly added to Cascadia Code in PR 213 (microsoft/cascadia-code#213) so I'm assuming that should be rendered correctly whenever the font is next released.

However, I'm curious what fallback font is producing the color versions of all of these characters. Every fixed space font I've tried seems to have them as monochrome. It's only when the glyphs aren't included that I'm getting a fallback that's in color. Where is that coming from?

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented May 12, 2020 •
edited
Loading

So, it looks like there's an automatic fallback (in DirectWrite) to "Segoe UI Emoji" for this codepoint range.

@leonMSFT leonMSFT mentioned this issue May 12, 2020
Merged
@ghost ghost added the In-PR This issue has a related PR label May 12, 2020
@ghost ghost closed this as completed in #5866 May 12, 2020
ghost pushed a commit that referenced this issue May 12, 2020
@leonMSFT
Revert some emoji back to narrow width
cf62922 
A couple of codepoints, namely the card suites, male and female signs,
and white and black smiling faces were changed to have a two-column
width as part of #5795 since they were specified as emoji in Unicode's
emoji list v13.0[1]. 

These particular glyphs also show up in some of the most fundamental
code pages, such as CP437[2] and WGL4[3]. We should
not be touching the width of the glyphs in these codepages, as suddenly
changing a long-time-running narrow glyph to use two-columns all of a
sudden will surely break (and has already broken) things.

[1] https://www.unicode.org/Public/13.0.0/ucd/emoji/emoji-data.txt
[2] https://en.wikipedia.org/wiki/Code_page_437
[3] https://en.wikipedia.org/wiki/Windows_Glyph_List_4

Closes #5822
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels May 12, 2020
DHowett-MSFT pushed a commit that referenced this issue May 12, 2020
@leonMSFT @DHowett
Revert some emoji back to narrow width
b9483a9 
A couple of codepoints, namely the card suites, male and female signs,
and white and black smiling faces were changed to have a two-column
width as part of #5795 since they were specified as emoji in Unicode's
emoji list v13.0[1].

These particular glyphs also show up in some of the most fundamental
code pages, such as CP437[2] and WGL4[3]. We should
not be touching the width of the glyphs in these codepages, as suddenly
changing a long-time-running narrow glyph to use two-columns all of a
sudden will surely break (and has already broken) things.

[1] https://www.unicode.org/Public/13.0.0/ucd/emoji/emoji-data.txt
[2] https://en.wikipedia.org/wiki/Code_page_437
[3] https://en.wikipedia.org/wiki/Windows_Glyph_List_4

Closes #5822

(cherry picked from commit cf62922)
jelster pushed a commit to jelster/terminal that referenced this issue May 28, 2020
@leonMSFT @jelster
Revert some emoji back to narrow width
88d813f 
A couple of codepoints, namely the card suites, male and female signs,
and white and black smiling faces were changed to have a two-column
width as part of microsoft#5795 since they were specified as emoji in Unicode's
emoji list v13.0[1]. 

These particular glyphs also show up in some of the most fundamental
code pages, such as CP437[2] and WGL4[3]. We should
not be touching the width of the glyphs in these codepages, as suddenly
changing a long-time-running narrow glyph to use two-columns all of a
sudden will surely break (and has already broken) things.

[1] https://www.unicode.org/Public/13.0.0/ucd/emoji/emoji-data.txt
[2] https://en.wikipedia.org/wiki/Code_page_437
[3] https://en.wikipedia.org/wiki/Windows_Glyph_List_4

Closes microsoft#5822
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@leonMSFT leonMSFT

Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Impact-Compatibility Sure don't work like it used to. Impact-Correctness It be wrong. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.0
Development

Successfully merging a pull request may close this issue.

Revert some emoji back to narrow width microsoft/terminal
3 participants
@j4james @DHowett-MSFT @leonMSFT