Skip to content

Fix bug of some IMEs cannot input in terminal#3251

Merged
Tyriar merged 4 commits intoxtermjs:masterfrom
Python-37:Fix-bug-of-some-IMEs-cannot-input-in-terminal
Mar 18, 2021
Merged

Fix bug of some IMEs cannot input in terminal#3251
Tyriar merged 4 commits intoxtermjs:masterfrom
Python-37:Fix-bug-of-some-IMEs-cannot-input-in-terminal

Conversation

@Python-37
Copy link
Copy Markdown
Contributor

@Python-37 Python-37 commented Mar 10, 2021

Fix bug of QQ pinyin and Rime IME cannot input characters in VS Code's terminal.
microsoft/vscode#115814
This bug behaves like this microsoft/vscode#115814 (comment)
Here we found the reason microsoft/vscode#115814 (comment)
Here we found a solution microsoft/vscode#115814 (comment)
The css padding: 0 caused this bug.
Thanks to @ccloli and @ZhyMC

Fix bug of QQ pinyin and Rime IME cannot input characters in terminal.
@Kingwl
Copy link
Copy Markdown

Kingwl commented Mar 11, 2021

Cool! :XD

Copy link
Copy Markdown
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work all!

The fix seems a little strange, I can understand if the IME sees a 0x0 element and doesn't work, but the textarea should not be 0x0 but the size of the cursor:

image

Can you check the size of the textarea on your machine? It's seems really strange that padding would affect an IME like this.

cc @rebornix

@Tyriar Tyriar added this to the 4.12.0 milestone Mar 11, 2021
@ccloli
Copy link
Copy Markdown

ccloli commented Mar 11, 2021

@Tyriar As our previous test, when the padding is 0, the textarea's width is also 0 when typing; but when it finishes, the input words are held on the textarea (can be verified by logging its value), and it's width is not 0, but the text width.

I just record its behavior from a VM, if you need it.

textarea-style.mp4

BTW I also record a video logging its value when keydown event is fired (so the logging value is its old value), seems that when typing with the IME, it'll put a space into the program UI (a regular whitespace , keycode is 32 or 0x20), and at that time, the textarea's width become 0. So maybe Xterm drops the whitespace, like value = value.trim()?

keydown-event.mp4

As our previous test, the IME will hold inputs until it finishes, then the IME will send the final result to the program. But it's unsure that when it finishes, the final result will be kept in the textarea, and Xterm doesn't put them out.

Say if you type abc first, the textarea value will be abc; then when you start typing next word like defg, the value will be abc (with a white space); once you finish it, the value becomes defg, but because of this rule, only g will be put out to Xterm output, and the result will be abcg.


Updated: When I remove the style padding: 0, seems that the textarea will hold all the inputs.

keydown-event-default-padding.mp4

(Sorry the VM's performance is not good, so the video is not that smooth)

Updated: reorganize my word.

@kena0ki
Copy link
Copy Markdown
Contributor

kena0ki commented Mar 14, 2021

Sorry for the regression, I didn't noticed the issue is happening until now.

The PR simply reverts padding:0 but it would cause another regression, the issue microsoft/vscode#104876.

IME input issue is caused from padding-left:0 and padding-right:0, so I think we should replace padding:0 to padding-top:0 and padding-bottom:0 instead of reverting whole padding:0.

Note 1: To me, this fix looks kind of a workaround, since I think css should not affect functional behavior. We might need further investigation for the issue as an another task. (I don't have time to do that right now though.)
Note 2: In the xtermjs demo (not in VS Code), I tried switching off padding:0 using QQ pinyin with Game Compatible Mode, but the issue was not gone. With Single-line Input Mode, the issue was gone regardless of padding:0 being enabled or not. So if we support third party IME with the classical (or normal?) mode not only for VS Code, I don't know we should, we need another PR.

@ccloli
Copy link
Copy Markdown

ccloli commented Mar 14, 2021

@kena0ki Maybe another option is setting min-width? At least by setting min-width: 1px both QQ Pinyin and Rime are working.

min-width-rime.mp4

@kena0ki
Copy link
Copy Markdown
Contributor

kena0ki commented Mar 14, 2021

@ccloli I confirmed min-width:1px fixes the issue in VS Code and does not fix the issue in the xtermjs demo, i.e. the result is exactly the same. Maybe min-width with a comment is better in terms of readability.
The interesting thing is that in both cases removing padding:0 or adding min-width:1px, textarea temporarily becomes width:0 while using IME. (Internal value different from the value showed in DevTools?)

Edit:
I found padding:0 and min-width:1px are not related to width. width is probably modified somewhere in js.
I guess third party IMEs find whether textarea is available or not by using size of textarea width including padding and border. So even if width is set to 0, if padding or border is greater than 0 (or is default), the IMEs can find textarea is avalable. This is also the case with min-width greater than 0.
So the available solutions are change css (padding, border or min-width) or fixing js. I think easy and reasonable solution is adding min-width after all.

@wyattzheng
Copy link
Copy Markdown

wyattzheng commented Mar 15, 2021

@kena0ki some css properties are calculated and overridden by javascript methods, see : /src/browser/input/CompositionHelper.ts.

this._textarea.style.left = cursorLeft + 'px';
this._textarea.style.top = cursorTop + 'px';
this._textarea.style.width = compositionViewBounds.width + 'px';
this._textarea.style.height = compositionViewBounds.height + 'px';
this._textarea.style.lineHeight = compositionViewBounds.height + 'px';

we should notice this.

@ccloli
Copy link
Copy Markdown

ccloli commented Mar 15, 2021

I found padding:0 and min-width:1px are not related to width. width is probably modified somewhere in js.

Yep, the width is probably calculated by JS, it looks like the width is calculated by trimmed text's width, so when it only has a whitespace, its width becomes 0.

I confirmed min-width:1px fixes the issue in VS Code and does not fix the issue in the xtermjs demo, i.e. the result is exactly the same

When I was testing, I encountered this, too. However I can't reproduce it now, but I guess it's due to the position of the textarea is out of the browser's viewport.

I guess third party IMEs find whether textarea is available or not by using size of textarea width including padding and border. So even if width is set to 0, if padding or border is greater than 0 (or is default), the IMEs can find textarea is avalable.

It behaves like this, but from my test it might not be IME's issue, since you can typing text in a zero-width textarea with these IMEs and having no issue, the inputs goes into the textarea and doesn't loss any characters.

(However if you click the textarea to input, if you've already type some text on it, some of text may be replaced, but clicking Focus button won't have the issue. Maybe when width goes zero, some of text is selected, so when typing it'll be replaced)

<body>
<textarea placeholder="type here" style="border: 0px; padding: 0px; margin: 0px; width: auto; height: 89px;" onfocus="this.style.width = 0" onblur="console.log(this.value); this.style.width = 'auto';"></textarea>
<button onclick="document.querySelector('textarea').focus()">Focus</button>
</body>

@kena0ki
Copy link
Copy Markdown
Contributor

kena0ki commented Mar 15, 2021

Ah, I understand. Losing focus cause the issue.
I confirmed characters were unable to be entered correctly if textarea width (including padding and border) was changed to 0 while typing characters using IME.
https://codepen.io/kena0ki/pen/YzpgOej?editors=1111

@Tyriar
Copy link
Copy Markdown
Member

Tyriar commented Mar 18, 2021

What if we did this instead?

// Ensure the text area is at least 1x1, otherwise certain IMEs may break 
this._textarea.style.width = Math.max(compositionViewBounds.width, 1) + 'px';
this._textarea.style.height = Math.max(compositionViewBounds.height, 1) + 'px';

If someone could test that before the coming Monday I can push this in for VS Code 1.55

@ccloli
Copy link
Copy Markdown

ccloli commented Mar 18, 2021

I change the built code directly, having no issue.

math-max-1.mp4

@wyattzheng
Copy link
Copy Markdown

wyattzheng commented Mar 18, 2021

opacity property makes textarea invisible, width > 0 solved some issues which zero width cause, so in my opinion, it's a good final solution.

should we start a new PR?

@Tyriar
Copy link
Copy Markdown
Member

Tyriar commented Mar 18, 2021

Thanks for the quick tests! I made the change in this PR

@Tyriar Tyriar enabled auto-merge March 18, 2021 15:15
@wyattzheng
Copy link
Copy Markdown

@Tyriar Hooray!

Comment thread src/browser/input/CompositionHelper.ts Outdated
Comment on lines 228 to 229
this._textarea.style.width = compositionViewBounds.width + 'px';
this._textarea.style.height = compositionViewBounds.height + 'px';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this._textarea.style.width = compositionViewBounds.width + 'px';
this._textarea.style.height = compositionViewBounds.height + 'px';

Shouldn't this 2 lines be removed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦

@Tyriar Tyriar disabled auto-merge March 18, 2021 15:30
@Tyriar Tyriar enabled auto-merge March 18, 2021 15:32
@Tyriar Tyriar merged commit 9a90f1f into xtermjs:master Mar 18, 2021
@Python-37 Python-37 deleted the Fix-bug-of-some-IMEs-cannot-input-in-terminal branch March 19, 2021 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants