Emoji fix #1890

juancampa · 2019-01-07T18:58:39Z

Problems

There were multiple problems with emoji

Opening the emoji dialog was possible but it appeared at a weird location. This was due to the textarea we use for input was not being kept in sync with the cursor position unless a composition (i.e. IME input) was underway.
No emoji was received by the terminal.

Solutions

Now the textarea always moves with the cursor.
Added handler for the input event, only for emoji handling. Also, the textarea needs to have a lineheight > 0 for this event to fire, at least in Chromium.

Testing

…ormer seems outdated atm

Tyriar · 2019-01-07T19:22:45Z

The position is still broken on Windows:

win+.

Actual input doesn't seem to work either, but that's probably related to shell/pty problems.

Tyriar · 2019-01-07T19:28:59Z

macOS/firefox

Opens the emoji ime in the wrong spot:

juancampa · 2019-01-07T19:32:00Z

The position is still broken on Windows:

Unfortunately I don't have a good way to test on Windows at the moment. Trying to get access to a remote desktop machine.

macOS/firefox

Looking into it, it seems to work sometimes 🤔

Tyriar · 2019-01-07T19:26:18Z

yarn.lock

@@ -26,6 +26,11 @@
  resolved "https://registry.yarnpkg.com/@types/chai/-/chai-3.5.2.tgz#c11cd2817d3a401b7ba0f5a420f35c56139b1c1e"
  integrity sha1-wRzSgX06QBt7oPWkIPNcVhObHB4=

+"@types/dom-inputevent@^1.0.3":


This should be undone since we're not using anymore

Tyriar · 2019-01-07T19:30:09Z

src/common/Types.ts

+ */
+export interface IInputEvent {
+  readonly data?: string;
+  readonly inputType?: string;


It's missing from the type probably because support is bad (no Firefox), do you know why it appears to work on Firefox despite this? https://developer.mozilla.org/en-US/docs/Web/API/InputEvent/inputType#Browser_compatibility

@Tyriar The data attribute is not supported on Firefox. As per my previous comment, inputting emojis somewhat works with Firefox even without the code on this branch: #469 (comment)

Interesting. I'll run it with the debugger to understand what's happening on Firefox

I did a bit of investigation on Firefox:

An alternative to data is to use event.target.value but only if we're clearing the textarea on every change

Firefox positioning of the Emoji popup is buggy in that, if you move the textarea around, the emoji popup's position (later, when it shows) will use the previous position of the textarea. A work-around I found is to manually set textarea.value to something (doesn't matter what), which seems to force firefox to recalculate the position used for emoji popup. Seems like a cache invalidation bug.

Firefox treats emoji input as composition while chrome doesn't 🤷‍♂️

Oh, and finally, and this might be a deal breaker, there's no inputType on Firefox, which we're using to detect when text is inserted vs. when text is pasted. There might be a way to work around this, by treating it as a composition but I haven't looked deeper into it

If we can't easily do it reliably on Firefox, I think at least doing it on Chrome would definitely be an improvement.

Tyriar · 2019-01-07T19:30:47Z

src/Emoji.ts

+//   https://stackoverflow.com/questions/30757193/
+// For a complete list of all assigned codepoints, go to:
+//   https://unicode.org/Public/emoji/11.0/emoji-data.txt
+export function isEmoji(str: string): boolean {


Let's move this into CharWidth.ts, this will later be renamed to Unicode*.ts so i think it makes sense to live there.

Tyriar · 2019-01-07T19:37:17Z

src/CompositionHelper.ts

-    if (!this._isComposing) {
-      return;
-    }
-
    if (this._terminal.buffer.isCursorInViewport) {


Why did you need to remove this? I don't think it's meant to move when composing (after it becomes visible)

Not sure I follow. If that were the case, shouldn't the if be if (this._isComposing). My understanding is that this if avoids unnecessarily moving the textarea when is not needed (not composing anything), but I might be wrong 😝

The idea here is that we need a textarea to follow the cursor at all times so that the emoji popup shows at the right position (right under the text area).

Thinking about this a bit more, it might be better to move the positioning of the textarea to Terminal.ts since it's not a composition thing anymore, and we wouldn't be moving the composition view unnecessarily (which I think is what you meant initially)

This function is only meant to do anything after compositionstart and before compositionend, I think you're right that maybe this should be moved out.

Tyriar · 2019-01-07T19:38:12Z

src/Emoji.ts

+// For a complete list of all assigned codepoints, go to:
+//   https://unicode.org/Public/emoji/11.0/emoji-data.txt
+export function isEmoji(str: string): boolean {
+  if (str.codePointAt === undefined) {


Please add:

// TODO: Remove when IE11 support is dropped

Tyriar · 2019-01-07T19:39:09Z

src/common/Types.ts

+ * There's a @types/dom-inputevent but at the moment it's outdated and missing
+ * some properties (e.g. inputType). So in the meantime we're using this instead
+ */
+export interface IInputEvent {


Let's call this IDomInputEvent to be clear as IKeyboardEvent above is an xterm.js thing

Tyriar · 2019-01-26T20:54:08Z

@jerch do you know why if you press space twice after inserting an emoji a . appears?

This even happens on master when you paste an emoji

Tyriar · 2019-05-21T22:02:22Z

Ping @juancampa, you'll have difficulty keeping this page up to date in Hyper if it doesn't get merged in

bet4it · 2019-05-28T09:58:21Z

src/CompositionHelper.ts

@@ -206,11 +202,13 @@ export class CompositionHelper {
      // Sync the textarea to the exact position of the composition view so the IME knows where the
      // text is.
      const compositionViewBounds = this._compositionView.getBoundingClientRect();
+      const width = Math.max(compositionViewBounds.width, this._terminal.charMeasure.width);
+      const height = Math.max(compositionViewBounds.height, this._terminal.charMeasure.height);


I'm sure these two lines are wrong.
It will cause bug totally unrelated with emoji.
I have tested it in my environment.
Hope you can reconsider it.
Maybe we need Math.min or something?

Tyriar · 2019-06-28T19:56:28Z

Closing off since this needed a bunch of changes and has diverged a lot. A new attempt at fixing should probably start on a fresh branch.

juancampa added 4 commits January 7, 2019 13:19

Adding support for emoji input

f0cf75f

More correct handling of emojis with IMEs and pasting

5cf9467

Short-circuit case for isEmoji.

48e02ab

Replacing @types/dom-inputevent with a custom IInputEvent since the f…

1133d0c

…ormer seems outdated atm

Tyriar mentioned this pull request Jan 7, 2019

Failing checks not showing up at all microsoft/vscode-pull-request-github#790

Closed

juancampa mentioned this pull request Jan 7, 2019

Adding support for emoji input vercel/xterm.js#6

Merged

6 tasks

Tyriar reviewed Jan 7, 2019

View reviewed changes

Tyriar added the work-in-progress Do not merge label Jan 7, 2019

This was referenced Feb 25, 2019

Support emoji IMEs #469

Closed

Fix emoji width #1951

Closed

bet4it suggested changes May 28, 2019

View reviewed changes

Tyriar mentioned this pull request May 28, 2019

True colors support vercel/hyper#3695

Closed

Tyriar closed this Jun 28, 2019

Tyriar added reference A closed issue/pr that is expected to be useful later as a reference and removed work-in-progress Do not merge labels Jun 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emoji fix #1890

Emoji fix #1890

juancampa commented Jan 7, 2019 •

edited

Loading

Tyriar commented Jan 7, 2019

Tyriar commented Jan 7, 2019

juancampa commented Jan 7, 2019 •

edited

Loading

Tyriar Jan 7, 2019

Tyriar Jan 7, 2019

epicfaace Jan 9, 2019

juancampa Jan 9, 2019

juancampa Jan 10, 2019

juancampa Jan 10, 2019

epicfaace Jan 11, 2019

Tyriar Jan 7, 2019

Tyriar Jan 7, 2019

juancampa Jan 9, 2019

juancampa Jan 9, 2019 •

edited

Loading

Tyriar Jan 26, 2019

Tyriar Jan 7, 2019

Tyriar Jan 7, 2019

Tyriar commented Jan 26, 2019

Tyriar commented May 21, 2019

bet4it May 28, 2019

Tyriar commented Jun 28, 2019

Emoji fix #1890

Emoji fix #1890

Conversation

juancampa commented Jan 7, 2019 • edited Loading

Problems

Solutions

Testing

Tyriar commented Jan 7, 2019

Tyriar commented Jan 7, 2019

juancampa commented Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juancampa Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tyriar commented Jan 26, 2019

Tyriar commented May 21, 2019

Choose a reason for hiding this comment

Tyriar commented Jun 28, 2019

juancampa commented Jan 7, 2019 •

edited

Loading

juancampa commented Jan 7, 2019 •

edited

Loading

juancampa Jan 9, 2019 •

edited

Loading