RTL text in conhost is no longer rendered correctly #12294
Comments
ghost
added
Needs-Triage
|
I'm not suggesting we revert PR #10478, since I'd hate to lose the benefits we get from that, but I think the RTL behaviour could be fixed by inserting an additional step to calculate the glyph indexes with |
|
@DHowett Don't want to nag, but note that this is a regression in conhost, and I'm a little concerned that it hasn't been triaged and may have been overlooked. |
|
Sorry, yes this was overlooked. I think mentally I kinda go "yep, I'm sure that's a real bug" when I see your name as the filer 😋 I'll toss this in 1.14. We should fix this for the OS version of conhost. |
zadjii-msft
added
Area-Rendering
DHowett
removed
the
Needs-Triage
|
Pulled triage. Sorry @j4james, I've been snowed in on e-mail as I had to leave to take care of some family stuff. Thanks for the first pass, Mike. |
|
Yes. This is very important for us to fix. /cc @alabuzhev for thoughts on how |
|
FYI, my quick hack fix for this was to replace the terminal/src/renderer/gdi/paint.cpp Line 448 in dacff61 with something like this: std::array<wchar_t, 1000> glyphs;
GCP_RESULTS results{};
results.lStructSize = sizeof(results);
results.lpGlyphs = glyphs.data();
results.nGlyphs = gsl::narrow_cast<UINT>(glyphs.size());
GetCharacterPlacementW(_hdcMemoryContext, t.lpstr, t.n, GCP_MAXEXTENT, &results, 0);
if (!ExtTextOutW(_hdcMemoryContext, t.x, t.y, t.uiFlags | ETO_GLYPH_INDEX, &t.rcl, results.lpGlyphs, results.nGlyphs, t.pdx))Obviously not intended to be production code, but you get the idea. |
I wish this was true. And also each character occupied exactly one cell. And no zero width. And no surrogates. And no clusters. And so on and so forth. Unfortunately, text processing is a PITA.
Then the new and shiny Windows Terminal is also not compatible with the original conhost and breaks such RTL-aware applications. Are there any complaints from their maintainers? Should it be fixed there too? And in conhost DX renderer? And in conemu, console2 and other similar frontends? Overall, my experience here is extremely limited, I don't work with RTL and can't say how it should be. @trexinc, I remember somewhat related discussions eons ago on the forum about how the console should behave with RTL to make life less painful. Do you have any opinion about this? |
|
Speaking about compatibility with the original conhost: as mentioned here, font fallback used to work in pre-Windows 7 days, when
|
It's been fixed in the new atlas render. |
|
@j4james I believe this doesn't work with font fallback. I think if you try to draw Japanese text for instance, it'll show just blank / whitespace glyphs. As far as I can see the only way to resolve this issue, while having both, font fallback and broken RTL support, is to use @alabuzhev Wait... Did you paste them exactly as |
Yes, this as is: Moreover:
|
While I would love to take that compliment as the author of the engine as is, I have to confess that this is unfortunately more like a side-effect from me not implementing RTL/BiDi support at all. 😕 @alabuzhev Wow! This is impressive! On one hand I think I now understand that we'll likely have to revert the |
That's kinda my point - do you already have dozens of reports like "things are broken, the world is falling apart, do something now"? |
Yeah, you're right. I've just tested and that's not working for me either. Oh well.
Yeah, that's weird. It's definitely not reordering RTL characters for me in the legacy console.
That would be because it's almost impossible to write an RTL application on terminals that don't work this way. Give it a try. See if you can write some basic RTL applications on one of those terminals that reorders RTL characters. Like a simple RTL form entry system, or something that pops up a dialog or drop-down menu over existing RTL text. Maybe I'm just an idiot, but I can't see how you can make that work, but it's fairly straightforward on terminals that leave RTL characters exactly where you put them. |
|
So I think we have 3 options here with various benefits:
Regarding 2. and 3.: I'm pretty sure that this will re-break Arabic scripts, since those heavily lean on ligatures and glyph reordering to render correctly. So basically conhost using logical order and the TUI application writing the glyphs backwards manually will only (practically at least) work for Hebrew basically as far as I can see, since Hebrew is a bit like "Latin in RTL". However I understand that we'd not want to take any chances in regressions and would thus consider opting for 2. @miniksa? |
|
As far as I can discern, no one ever actually concerned themselves with Arabic nor Hebrew support in the console host. The targeted languages were basically LTR European type character sets + the CJK trio. Beyond that... it looks like anything else that worked or didn't was a happy accident. Furthermore, when our localization team tells us what languages we can pay for in terms of translations for developer utilities today... they limit it to: German, English, Spanish, French, Italian, Japanese, Korean, Brazilian Portuguese, Russian, Simplified Chinese, and Traditional Chinese. I'd therefore have to believe to some degree that research was performed to determine that was the appropriate balance between resources and developer market was to focus on those languages. Therefore, my consideration here is happiness of those languages as primary goal with anything else being secondary. Further, one of the most popular issues filed against But with the combination of those reasons, I would have to personally opt for Option 1. I would offer to @lhecker, if he's interested, that next week is our organization's "Fix Hack Learn" week again. If he wants to spend a few days hacking Option 3 using Uniscribe to solve this problem and learn more about language processing... he would be free to do so. I think it would be better, though, long-term to focus efforts on supporting those languages fully in the Terminal and the Atlas renderer. The discussion can continue, I'm not shutting it down. This is just my opinion on the situation. |
|
I have a suggestion for another possible solution which may keep everyone happy. We carry on using |
|
How do you detect runs of RTL glyphs? If the answer is Uniscribe, I think we can just go all the way and use it for text drawing too... |
I was thinking of something simple like a range check. Haven't looked at the unicode blocks in detail, but you could start with everything from
OK ligatures I can see being a problem for Arabic text. I've only really dealt with Hebrew so I'm not sure how well that works. The other unicode stuff seems less of a problem. Does any of that stuff work now? Are we ever expecting it to work in the GDI renderer? |
Yeah it does! While Uniscribe doesn't seem to support "liga" in fonts, it does correctly handle ligatures in most languages. You gotta say, this is pretty nice to see in good old conhost, right?
I'd be quite sad if we lost that, but I'd understand that it'd be for a good cause. Given that most terminals apart from xterm seem to not draw glyphs in their logical order either, I do wonder however whether it's reasonable to merge my fix, even if I submit such a PR later. BTW for my own curiosity: Do you happen have a specific application at hand that positions Hebrew text manually? This would allow me to better test my Uniscribe- (or char-range-) experiments. |
Yeah, I saw the ligatures were working. I meant the other things you were refering to when you said "ZWJ, etc. and all the other fun Unicode stuff". And while it looks nice at first glance, it's not particularly useful as is as far I'm concerned. You've got no hope of editing the text - all it's really good for is displaying a single line of content at best.
Yeah, ideally we'd have a solution that was realistically usable and also looked pretty, but I don't know how feasible that is for languages with ligatures. I thought with something like Arabic, an application might be able to output the appropriate form of each character manually, which might make up for the loss of ligature support, but I don't know enough about the subject to know if that's nonsense.
I wouldn't have said "most", but I haven't checked recently. And for those that do draw the glyphs in RTL order, there's not a standard of any sort that they're following - they all do things differently. Thankfully some of them at least have a way to turn that functionality off.
You realise that just means we're saying we don't support TUI applications fullstop (at least for BiDi languages). If that's the route we want to take, that's fine - I seem to be in the minority in wanting support for RTL TUI apps. I'd just like to know for definite where we're going with this, so I can make my own plans accordingly.
Well there is the command line utility from fribidi, which can be used as a kind bidi-aware version of |
|
So instead of creating a Unicode standard at some point to standardize the (cell) width per grapheme cluster, applications started to straight up write "characters" in reverse. That's actually scary. 😨 Thanks for the tip with In either case I'm convinced now and will make sure to build something that restores the previous behavior as soon as I can. I mean I already planned to do it, but now I'm doing it out of conviction. 😄 I know approximately how to draw Unicode with Uniscribe, but it'll probably take me ages to integrate that into the GdiEngine... Implementing the ligature and wide glyph support in AtlasEngine was simple since it was a new project after all (which is why it handles Emojis with ZWJs for instance). |
I agree with you there. I just thought it might be better than nothing, but if there's a way to do things properly with Uniscribe, I would be thrilled. One thing you may need to watch out for when testing, is support for horizontal scrolling in conhost (disable the "wrap text output on resize" option, and make the buffer size wider than the window size). When the viewport isn't at the left margin, it can start rendering half way through the buffer, which breaks things completely in the current implement when RTL text is reordered. Hopefully it won't be a problem if we're going back to rendering in logical order again, but you may get some weird artifacts when ligatures are split across the viewport border. You'll likely have similar problems when selecting text, and cursoring over text (depending on the cursor type). But I don't think it's the end of the world if we don't have all the edge cases working perfectly to start with. Also note that the And one last tip for testing. If, like me, you don't speak any RTL languages, I've found it helpful to use nonsense Hebrew content that looks vaguely like English, so I can more easily tell when something has gone wrong. For example, the phrase below looks a bit like "young puppy won't nip on jogging pony". And the equivalent reversed text (which should look correct when the renderer doesn't do RTL reordering): |
|
@j4james Damn that was almost too easy - took like 5 minutes: https://github.com/microsoft/terminal/compare/dev/lhecker/12294-bidi-override diff --git a/src/renderer/gdi/paint.cpp b/src/renderer/gdi/paint.cpp
index 598ce3489..bc4c4e95a 100644
--- a/src/renderer/gdi/paint.cpp
+++ b/src/renderer/gdi/paint.cpp
@@ -445,9 +445,21 @@ using namespace Microsoft::Console::Render;
for (size_t i = 0; i != _cPolyText; ++i)
{
const auto& t = _pPolyText[i];
- if (!ExtTextOutW(_hdcMemoryContext, t.x, t.y, t.uiFlags, &t.rcl, t.lpstr, t.n, t.pdx))
+
+ SCRIPT_STATE ss{};
+ ss.fOverrideDirection = TRUE;
+
+ SCRIPT_STRING_ANALYSIS ssa;
+ hr = ScriptStringAnalyse(_hdcMemoryContext, t.lpstr, t.n, 0, -1, SSA_GLYPHS | SSA_FALLBACK | SSA_LINK, 0, nullptr, &ss, t.pdx, nullptr, nullptr, &ssa);
+ if (FAILED(hr))
+ {
+ break;
+ }
+
+ hr = ScriptStringOut(ssa, t.x, t.y, t.uiFlags, &t.rcl, 0, 0, FALSE);
+ ScriptStringFree(&ssa);
+ if (FAILED(hr))
{
- hr = E_FAIL;
break;
}
}I think typing this message took longer than writing that code. 😄 ScriptStringAnalyse is a handy function that calls ScriptItemize, ScriptShape, ScriptPlace, and ScriptBreak for you. If I can't make it for whatever reason though, I think this is what we could ship, since it works. |
lhecker
added
Priority-1
Some applications like `vim -H` implement their own BiDi reordering. Previously we used `PolyTextOutW` which supported such arrangements, but with a0527a1 and the switch to `ExtTextOutW` we broke such applications. This commit restores the old behavior by reimplementing the basics of `ExtTextOutW`'s internal workings while enforcing LTR ordering. ## Validation Steps Performed * Create a text file with "ץחסק פחופפסנ חס קוח ז׳חסש ץקקטק פחטסץ" Viewing the text file with `vim -H` presents the contents as expected ✅ * Printing enwik8 is as fast as before ✅ * Font fallback for various eastern scripts in enwik8 works as expected ✅ * `DECDWL` double-width sequences ✅ * Horizontal scrolling (apart from producing expected artifacts) ✅ Closes #12294
ghost
added
Resolution-Fix-Committed
Some applications like `vim -H` implement their own BiDi reordering. Previously we used `PolyTextOutW` which supported such arrangements, but with a0527a1 and the switch to `ExtTextOutW` we broke such applications. This commit restores the old behavior by reimplementing the basics of `ExtTextOutW`'s internal workings while enforcing LTR ordering. ## Validation Steps Performed * Create a text file with "ץחסק פחופפסנ חס קוח ז׳חסש ץקקטק פחטסץ" Viewing the text file with `vim -H` presents the contents as expected ✅ * Printing enwik8 is as fast as before ✅ * Font fallback for various eastern scripts in enwik8 works as expected ✅ * `DECDWL` double-width sequences ✅ * Horizontal scrolling (apart from producing expected artifacts) ✅ Closes #12294 (cherry picked from commit d97d9f0)
Windows Terminal version
Commit eb75597
Windows build number
10.0.19041.1415
Other Software
No response
Steps to reproduce
printf "\u05ea\u05d7\u05d0\n"Expected Behavior
RTL characters should be displayed in the exact order they were output, and not reversed. This is what it looks like in my inbox conhost (10.0.19041.1415):
This also matches the behaviour of XTerm.
Actual Behavior
In the current version of OpenConsole (I think since PR #10478), RTL characters are reversed, like this:
I realise that some people might consider this a good thing, since it gives the superficial appearance that it's rendering RTL languages correctly, but it is not compatible with the original conhost and breaks genuine RTL-aware applications (which rely on characters being displayed exactly where they've been positioned).
The text was updated successfully, but these errors were encountered: