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

Character Overlap during Korean IME Composition #4963

Closed
KennethanCeyer opened this issue Mar 17, 2020 · 12 comments · Fixed by #5135
Closed

Character Overlap during Korean IME Composition #4963

KennethanCeyer opened this issue Mar 17, 2020 · 12 comments · Fixed by #5135
Assignees
@leonMSFT
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) 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

@KennethanCeyer
Copy link

KennethanCeyer commented Mar 17, 2020 •
edited
Loading

Environment

Windows version

Microsoft Windows [Version 10.0.18362.720]

Terminal version

Version: 0.10.761.0

Steps to reproduce

  1. Open "Terminal" app
  2. Type "테스트" (It means test in Korean)
  3. You can probably see the letters overlapping

Expected behavior

Rather than a long explanation, I'd rather attach the behavior of another operating system terminal.

macos_terminal

In the case of MacOS, the cursor is positioned under the letter being combined during the Korean language composition process, and there is no collision of letters.

linux_terminal

In the Ubuntu 18.04 X Window system environment, if the same operation is performed in the terminal, You can expect that Hangul characters are under the composition process in a cursor shape. And the Hangul characters do not overlap.

Actual behavior

windows_terminal

In Version: 0.10.761.0, there is a problem that the Korean characters are overlapped as shown in the picture above.

If this problem is expected to be relatively minor by the core development team, please give a hint to the source code that is expected to be a problem so that I can give you a PR.
@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 Mar 17, 2020
@jdebp
Copy link

jdebp commented Mar 17, 2020

A data point: pasting in the same character sequence from the clipboard did not cause such an effect when I tried it, out of curiosity. I didn't try a Hangeul input method, though.

@KennethanCeyer
Copy link
Author

@jdebp Thanks for the additional comments! This problem is reproduced in the process of combining IME Hangul characters, so it is difficult to reproduce it by copying the clipboard.

@KennethanCeyer
Copy link
Author

If you test by typing the letters ㅌ, ㅔ, ㅅ, ㅡ, ㅌ, ㅡ in order, you can check the problem.

@zadjii-msft zadjii-msft added Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Mar 17, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Mar 17, 2020
@zadjii-msft zadjii-msft added this to the Terminal v1.0 milestone Mar 17, 2020
@leonMSFT
Copy link
Contributor

leonMSFT commented Mar 17, 2020 •
edited
Loading

So at first glance, it seems like the TSFInputControl is getting its set of LayoutRequested events before the Terminal manages to update its cursor, leading to the TextBlock staying in the same place when it should have moved over to where the cursor currently is. If you then type one extra character after the first overlapping keypress, the LayoutRequested will fire again, and will update the TextBlock to the correct position of the cursor.

What's funny is that while debugging this by building it in Visual Studio and running it with the debugger, my breakpoints slow the events down enough that sometimes the Terminal updates its cursor before the LayoutRequested event is received, which makes the text not overlap.

@KennethanCeyer I think the fix might be simple, depending on how difficult it is to figure out when we should be re-drawing where the TextBlock should be, instead of relying on LayoutRequested to do it. If you're willing to take a stab at it, let me know and I can answer questions if you have any! 😄

@leonMSFT leonMSFT changed the title During the Korean combination process, The chars overlapped Characters Overlap during Korean IME Composition Mar 17, 2020
@leonMSFT leonMSFT changed the title Characters Overlap during Korean IME Composition Character Overlap during Korean IME Composition Mar 17, 2020
@DHowett-MSFT DHowett-MSFT added Help Wanted We encourage anyone to jump in on these. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Mar 18, 2020
@KennethanCeyer
Copy link
Author

@leonMSFT
Thank you for a detailed description. Now it's just a matter of reading the contribution documentation and looking at the code!

@leonMSFT
Copy link
Contributor

Hi @KennethanCeyer, just checking in to see where you're at with this bug, if you haven't had too many chances to make much progress on it, I could pick up from where you left off. Otherwise if you're close to a PR disregard me! 😄 I'm just tracking this bug more closely than others because it's marked as a P1 bug.

@KennethanCeyer
Copy link
Author

@leonMSFT
Yes, I see it on my side, but it takes about 7 days to respond.
Since it's P1, I think I should not hold onto it and become a block
If you can fix it, please proceed immediately. I'm sorry I seemed to be in the way.

@leonMSFT
Copy link
Contributor

Please don’t apologize! You were definitely not a blocker, as I was working on some other bugs in the meantime anyway. I was happy to let you take a look at the bug since you expressed some interest 🙂 I just figured I would check in to see how you were doing and perhaps offer my assistance. Thank you for reporting the bug and looking into it and providing your findings in the meantime though, I really appreciate it! 👍🏻

@leonMSFT leonMSFT mentioned this issue Mar 26, 2020
Merged
3 tasks
@ghost ghost added the In-PR This issue has a related PR label Mar 26, 2020
@ghost ghost closed this as completed in #5135 Mar 30, 2020
ghost pushed a commit that referenced this issue Mar 30, 2020
@leonMSFT
Redraw TSFInputControl when Terminal cursor updates (#5135)
ca6b54e 
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
This PR will allow TSFInputControl to redraw its Canvas and TextBlock in response to when the Terminal cursor position updates. This will fix the issue where during Korean composition, the first symbol of the next composition will appear on top of the previous composed character. Since the Terminal Cursor updates a lot, I've added some checks to see if the TSFInputControl really needs to redraw. This will also decrease the number of actual redraws since we receive a bunch of `LayoutRequested` events when there's no difference between them.

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #4963
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Startup, teardown, CJK IME gibberish testing, making sure the IME block shows up in the right place.
@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 Mar 30, 2020
@ghost
Copy link

ghost commented Apr 22, 2020

🎉This issue was addressed in #5135, which has now been successfully released as Windows Terminal Preview v0.11.1121.0.:tada:

Handy links:

@sykwon66
Copy link

sykwon66 commented Nov 22, 2021 •
edited
Loading

I see this same problem in version 1.11.2921.0 when Korean typing was made in Cygwin bash.
Both windows 10 and 11 show same problem.
No issue with cmd and PowerShell.
Attached the picture when problem happened (I was typing "한글을 사용하다 보며")
windows terminal cygwin korean char input overlapping
.

@KennethanCeyer
Copy link
Author

I also found the bug reproducible in 1.11.2921.0. as mentioned by @sykwon66

image

@sykwon66
Copy link

sykwon66 commented May 9, 2022

Seems fixed in 1.12.10982.0.
However there is actually very short time being overlapped still remained.

Previous: chars overlapped and stayed there.
Now: momentarilly overlapped and splits..
Fixed but annoying to see.

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-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) 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.

Redraw TSFInputControl when Terminal cursor updates microsoft/terminal
6 participants
@KennethanCeyer @DHowett-MSFT @zadjii-msft @jdebp @leonMSFT @sykwon66