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

The text layout code is expecting the quantity of glyphs is equal to the cells allocated for each line #3546

Closed
reli-msft opened this issue Nov 12, 2019 · 17 comments
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Milestone

Comments

@reli-msft
Copy link

The issue is found in CustomTextLayout:

Instead of carefully process the clusters input, CustomTextLayout drops nearly all the information from clusters, and only keeping how many columns a cluster occupy, and later it is expecting that the glyph quantity equals to the quantity of clusters. This may lead to incorrect rendering result for many cases (complex script, ligatures, etc.)

Bug #610, #696 may be caused by this.

@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 Nov 12, 2019
@reli-msft
Copy link
Author

reli-msft commented Nov 13, 2019

Running the following script in Node.JS shows a lot of problems in current text layout code:

console.log(`
Combining Diacritics    | [a\u0300\u0304]
Polytonic Greek         | [ᾊ] [Α\u0313\u0300\u0345]
Ideographic Variation   | [東京都葛\u{E0101}飾区] [奈良県葛\u{E0100}城市]
Composite Hangul        | [\u1100\u116A\u11B3]
Combining Dakuten       | [か\u309A]
Emoji vs Text           | [\u2602] [\u2602\uFE0E] [\u2602\uFE0F]
Emoji                   | [\u{1F469}]
Emoji with Skin Tone    | [\u{1F469}\u{1F3FB}]
Emoji Variation and ZWJ | [\u{1F469}\u{1F3FB}\u200D\u{1F4BB}]
`)

Terminal results:
image

Chrome Dev Tools results:
image

Edge F12 results:
image

Word results:
image

@skyline75489
Copy link
Collaborator

Possibly also #2191

@skyline75489
Copy link
Collaborator

Let me add Firefox result here, so that people don't forget that there's a browser out there called "Firefox":

图片

@DHowett-MSFT
Copy link
Contributor

@skyline75489 my hero

@skyline75489
Copy link
Collaborator

@DHowett-MSFT my man. I'm seeing regression on current master 306e751

图片

@skyline75489
Copy link
Collaborator

skyline75489 commented Nov 13, 2019

I've used git bisect and found that ddcc06e is the first bad commit. I've got no idea about what #3474 even is..

@zadjii-msft zadjii-msft added the Severity-Blocking We won't ship a release like this! No-siree. label Nov 13, 2019
@zadjii-msft zadjii-msft added this to the Terminal-1911 milestone Nov 13, 2019
@skyline75489
Copy link
Collaborator

I've tried this on my laptop and sadly the regression is the same:

图片

Emoji is noticeably broken. Japanese characters are different, too.

@zadjii-msft zadjii-msft added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Product-Terminal The new Windows Terminal. labels Nov 13, 2019
@zadjii-msft
Copy link
Member

@skyline75489 Thanks for the report! I'm going to move that regression to it's own thread (#3553), considering you've narrowed the regression down to a particular commit. Broadly our text rendering needs help with M chars->N glyphs scenarios like this issue describes, but we probably shouldn't have things getting worse on us.

@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Severity-Blocking We won't ship a release like this! No-siree. labels Nov 13, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 13, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 14, 2019
@reli-msft
Copy link
Author

This is heavily related to this issue: #780
There's WriteCharsLegacy and Terminal::_WriteBuffer, and they should be unified into one function that writes char stream to the back buffer with proper itemization.

@skyline75489
Copy link
Collaborator

Without #780 we can still improve the current implementation like #3578 . I'm not sure whether that's worth the effort, though. It's more like scratching the surface.

@reli-msft
Copy link
Author

reli-msft commented Nov 16, 2019

@skyline75489
We need to change the way we write buffers to perform correct line wrap and cursor movement.
For WT there are two buffers being used: one for CONHOST, which is the "Ground Truth", and one for WT, and the write subroutine is written differently.

So what's wrong with WriteCharsLegacy? Well...

  • It does not process surrogate pairs properly.
  • Combining characters are not written into the "previous" cell. They are treated separately and the cursor is moved.

And to provide a proper text layout, cells should...

  • Have some metadata to guide the client that whether they should be shaped together with adjacent cells.
  • If the real width of glyphs is different from the desired width, whar should we do.

@skyline75489
Copy link
Collaborator

@reli-msft Gotta admit, the whole Unicode experience is harder to fix than I initially anticipated. I'm not sure the exact plan Console team have. But I'm positive that we are heading to the right direction.

@reli-msft
Copy link
Author

reli-msft commented Nov 16, 2019

@skyline75489
Everybody are underestimating how complex text could be.

Let me count all the features that a proper rich text box should support:

  • Input methods, including...
    • Composing IME
    • Speech
    • Handwriting
    • Emoji picker
  • Complex text rendering, including...
    • Bidi
    • Shaping
    • Complex-script-aware editing
    • Complex-script-aware find, replace, etc.
  • Accessibility

And what they can choose to support:

  • Advanced typography, including...
    • About line breaking...
      • Hyphenation
      • Optimized line break (Knuth-plass, etc.)
    • About microtypography
      • OpenType features (ligatures, etc.)
      • OpenType variations
      • Multiple master fonts
      • Proper font fallback ← It is not a simple lookup-at-each-character process
    • Advanced Middle East features, including...
      • Kashida
    • Advanced Far East features, including...
      • Kinsoku Shori
      • Auto space insertion
      • Kumimoji
      • Warichu
      • Ruby
    • Proper vertical layout, including...
      • Yoko-in-Tate
  • Inline objects and paragraph-like objects, including...
    • Images
    • Hyperlinks
    • Math equations ← This is really hard.

@ghost ghost added the In-PR This issue has a related PR label Apr 18, 2020
@ghost ghost removed the In-PR This issue has a related PR label Jan 7, 2021
@zadjii-msft zadjii-msft modified the milestones: Terminal v2.0, 22H1 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H1, Terminal v1.14 Feb 2, 2022
@zadjii-msft zadjii-msft modified the milestones: Terminal v1.14, 22H2 Mar 10, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H2, Backlog Jul 5, 2023
@DHowett
Copy link
Member

DHowett commented Jun 27, 2024

You know, with the completion of #16916 I believe we've finally cracked this nut.

Image

@DHowett DHowett closed this as completed Jun 27, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants