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

TSFInputControl alignment is slightly off on High DPI #5470

Closed
1 task
miniksa opened this issue Apr 22, 2020 · 3 comments · Fixed by #5609
Closed
1 task

TSFInputControl alignment is slightly off on High DPI #5470

miniksa opened this issue Apr 22, 2020 · 3 comments · Fixed by #5609
Assignees
@miniksa @zadjii-msft @leonMSFT
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues 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

@miniksa
Copy link
Member

miniksa commented Apr 22, 2020

  • - TSFInputControl needs to account for scaling (migrie: ✔️)
    • miniksa 4/20@5:04pm, something's still a little off here cutting off the prev text and being a bit small...
    • DHowett 4/20, seems better after it's been opened and closed once, weirdly, for me

Originally posted by @miniksa in #5345 (comment)
@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 Apr 22, 2020
@miniksa miniksa self-assigned this Apr 22, 2020
@miniksa miniksa added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. and removed Needs-Tag-Fix Doesn't match tag requirements labels Apr 22, 2020
@miniksa miniksa mentioned this issue Apr 22, 2020
Merged
9 tasks
@DHowett-MSFT DHowett-MSFT added Priority-1 A description (P1) and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 22, 2020
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone Apr 24, 2020
@zadjii-msft zadjii-msft self-assigned this Apr 24, 2020
@miniksa
Copy link
Member Author

miniksa commented Apr 24, 2020 •
edited
Loading

image
It's a little to the left, but I just checked 0.11 (without incremental) and it's like that too.

However, resizing and moving the window cause the IME popup to go out of alignment until the cursor position changes (by deleting something in the edit line)

EDIT: Also, the covering thing I think was me talking about it being slightly left of the cursor and covering some of the previously written text. Might have also been related to the tracking because I was resizing and moving the window as well as adjusting the font size (ctrl+scroll wheel) while testing High DPI. Those sound related.

@leonMSFT
Copy link
Contributor

leonMSFT commented Apr 24, 2020 •
edited
Loading

(just a thought, noting it down here) maybe the text box has a slight border thickness and where the ime shows up is actually at the beginning of the text box?

Edit: for whatever reason, bringing emoji IME up on a new tab will overlap the row no matter what

@leonMSFT leonMSFT self-assigned this Apr 27, 2020
@leonMSFT leonMSFT mentioned this issue Apr 28, 2020
Merged
3 tasks
@ghost ghost added the In-PR This issue has a related PR label Apr 28, 2020
@ghost ghost closed this as completed in #5609 Apr 28, 2020
ghost pushed a commit that referenced this issue Apr 28, 2020
@leonMSFT
Adjust TSFInputControl alignment (#5609)
64f8bbf 
This PR fixes a couple of issues with TSFInputControl alignment:

1. The emoji picker IME in particular would show up overlapping the
   current buffer row because I stupidly didn't realize that
   `TextBlock.ActualHeight` is 0 right after initialization and before
   it has any text. So, the emoji picker will show up on the bottom of
   the 0 height TextBlock, which overlaps the current buffer row. This
   isn't a problem with CJK because inputting text _causes_ the IME to
   show up, so by the time the IME shows up, the TextBlock has text and
   accordingly has a height.
2. It turns out the emoji picker IME doesn't follow the `TextBlock`
   bottom, unlike Chinese and Japanese IME, so if a user were to compose
   near the edge of the screen and let the `TextBlock` start line
   wrapping, the emoji IME doesn't follow the bottom of the `TextBlock`
   as it grows. This means that the `TextBlock` position doesn't update
   in the middle of composition, and the `LayoutRequested` event that it
   fires at the beginning of composition is the only chance we get to
   tell the emoji IME where to place itself. It turns out when we reset
   `TextBlock.Text`, the ActualHeight doesn't get immediately reset back
   to the min size. So if a user were to bring up the emoji IME before
   `ActualHeight` is reset, the IME will show up way below the current
   buffer row.
3. We don't currently `TryRedrawCanvas` when the window position changes
   (resizing, dragging the window around), so sometimes dragging or
   resizing the Terminal doesn't update the position of the IME. Ideally
   it should be listening to some "window position changed" event, but
   alas we don't have that and it would be much harder than this
   incoming fix. We'll just track the window bounds as part of
   `TryRedrawCanvas` and redraw if it changes. For the most part, this
   will allow the IME to update to where the new window position is, but
   it'll only be called if we receive a `LayoutRequested` event.

## PR Checklist
* [x] Closes #5470
* [x] CLA signed.
* [x] Tests added/passed

## Validation Steps Performed
I play with it for quite a bit.
@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 Apr 28, 2020
@ghost
Copy link

ghost commented May 5, 2020

🎉This issue was addressed in #5609, which has now been successfully released as Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1).:tada:

Handy links:

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

@miniksa miniksa

@zadjii-msft zadjii-msft

@leonMSFT leonMSFT

Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues 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.

Adjust TSFInputControl alignment microsoft/terminal
4 participants
@DHowett-MSFT @miniksa @zadjii-msft @leonMSFT