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.

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

Add textarea back and support IMEs #175

Merged
merged 27 commits into from
Jul 22, 2016
Merged

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Jul 12, 2016

Korean input using Korean (Hangul) (IBus) on Ubuntu 16.04

image

Japanese input using Japanese (Anthy) (IBus) on Ubuntu 16.04

image

image

Covered in this PR:

Some of the testing that needs to be done on final version:

  • Copy using ctrl+ins
  • Paste using shift+ins and ctrl+shift+v
  • Inputting without IME continues to work as expected
  • Inputting via Korean IME
    • Test: ㅇㅇ (automated)
    • Test: 아아 (automated)
    • Test: 앙앙 (automated)
    • Pressing arrow keys to finalize composition
    • Pressing enter to finalize and execute composition
    • Numbers and punctuation can be input while IME is active (see remaining issues)
  • Inputting via Japanese IME
    • Test multiple character compositions (automated)
    • Test hiragana -> katakana conversion (automated)
    • Test hiragana -> kanji conversion (automated)
  • Inputting non-composition character (eg. 1) after composition character (eg. ㅇ) (automated)
  • Check support on Windows (tested using Microsoft Korean IME and Google Japanese IME, works fine except for the fact that the terminal cursor disappears which makes the composition appear on top of text)
  • Check support on OS X (tested using Apple's 2-Set Korean IME)
  • Check browser support (seems to behave consistently across browsers)
    • Chrome 51 (Linux, OS X, Windows)
    • Firefox 48 (Linux)
    • Safari 9.1.1 (OS X)

Fixes #124 (Terminal doesn't accept input via IME)

Remaining issues:

  • Currently the composition view is placed on top of the cursor, it's not aligned currently due to Lines containing unicode characters differ in height #149 (Lines containing unicode characters differ in height)
  • The composition view can appear on top of text on Windows only because the cursor seems to disappear. Input works fine, it's just that it can overlap on text.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 13, 2016

@parisk please review when you get a chance, so far I've only tested this on Korean but it's probably the most complex. I'll be able to test multi-character compositions with Japanese but it will likely be small tweaks inside CompositionHelper to get this working if it's not already.

The code itself is very well sectioned off from the rest so you don't have to worry so much about it introducing instability when not using an IME. A section is added to the top of Terminal.prototype.keyDown which prevents the key event going through if the isComposing flag is active. There is also a call at the end of refresh to position the composition view element on top of the cursor. All other changes not in CompositionHelper are just reverting the textarea change.

@parisk
Copy link
Contributor

parisk commented Jul 13, 2016

Thanks a lot for this @Tyriar! Right now I will focus on what is being needed to release 1.0.0 by the end of the week (e.g. adding a CONTRIBUTING.md file, updating the README, etc.) and I will get on top of this starting next week at most.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 13, 2016

@parisk thanks, probably a good idea to release v1 before pushing a change this big. If we can land this within a week it will get a bunch of testing in our Insider's build before the stable release.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 14, 2016

Ok done and tested, at least under Chrome 51 and Ubuntu 16.04. I moved even more into the CompositionHelper so now the only changes made outside are:

  • Creating the CompositionHelper
  • Attaching events to it
  • Calling into CompositionHelper.prototype.keydown manually at the beginning of Terminal.prototype.keyDown

@yuvipanda
Copy link
Contributor

(I can also help test this at paws.wmflabs.org once it lands!)

@Tyriar
Copy link
Member Author

Tyriar commented Jul 18, 2016

@parisk good to go 👍 the only remaining issue is the IME helper can popup in the wrong spot on Windows due to the cursor disappearing. The fix for that likely lies in pty.js or winpty though. Input works just fine, it's just it that text being input can overlap on top of other text.

@parisk
Copy link
Contributor

parisk commented Jul 20, 2016

Thanks a lot @Tyriar! Moving on with testing on OS X.

this.textarea.setAttribute('autocapitalize', 'off');
this.textarea.setAttribute('spellcheck', 'false');
this.textarea.tabIndex = 0;
this.textarea.onfocus = function() {
Copy link
Contributor

@parisk parisk Jul 20, 2016

Choose a reason for hiding this comment

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

Could you please use this.textarea.addEventListener('focus', ...) instead here please?

@parisk
Copy link
Contributor

parisk commented Jul 20, 2016

🆗 , I made my first comments on code styling and I have a few more on functionality on OS X.

  1. Cannot paste using right click. Quick hack proposal: Reposition the helper textarea below the mouse position on mousedown if the right click is being pressed. This should trigger the contextmenu event on the textarea and the user should be able to paste using right click.

  2. Cannot enter emojis. Xterm ignores emojis completely when attempting to enter them with the native input method on OS X. Well this is definitely non-blocking for merging this PR (no matter how much I love emojis in the terminal 😍 ), but thought about mentioning.

    In general this PR behaves really well and is exceptionally written and documented! 👍 @Tyriar!

@Tyriar
Copy link
Member Author

Tyriar commented Jul 21, 2016

@parisk so I took a shot at fixing the paste on right click behavior and while I got it to work by positioning the textarea under the cursor and then redirecting the event to the textarea and cancelling the original event, it removes the selection which breaks means you cannot copy using right click. Here's the super messy/hacky code I used to accomplish this:

--- a/src/xterm.js
+++ b/src/xterm.js
@@ -1220,7 +1220,41 @@
         };
       }

+function getOffset( el ) {
+    var _x = 0;
+    var _y = 0;
+    while( el && !isNaN( el.offsetLeft ) && !isNaN( el.offsetTop ) ) {
+        _x += el.offsetLeft - el.scrollLeft;
+        _y += el.offsetTop - el.scrollTop;
+        el = el.offsetParent;
+    }
+    return { top: _y, left: _x };
+}
+
+self.textarea.addEventListener('mousedown', function(ev) {
+  console.log('textarea mousedown', ev);
+ev.stopPropagation();
+});
+
       on(el, 'mousedown', function(ev) {
+
+        self.textarea.style.position = 'absolute';
+var offset = getOffset(self.element);
+        self.textarea.style.left = (ev.pageX - offset.left - 10) + 'px';
+        self.textarea.style.top = (ev.pageY - offset.top - 10) + 'px';
+self.textarea.style.width = '20px';
+self.textarea.style.height = '20px';
+self.textarea.style.zIndex = '0';
+        redirectMouseEvent(ev, self.textarea);
+
+function redirectMouseEvent(event, el) {
+  el.dispatchEvent(new MouseEvent(event.type, event));
+}
+ev.preventDefault();
+    ev.stopPropagation();
+    return;
+
+
         if (!self.mouseEvents) return;

         // send the button

Perhaps this should be deferred until selection is handled in a non-native way which would fix #68 and #69. I think that's how hterm behaves and it's how the VS Code editor maintains selection no matter where the focus is, seems like the right solution without compromising.

There is always ctrl+shift+v and cmd+v to paste now which are the defaults on Linux/Mac.

I'd prefer to get this in and accept the regression with the context menu for now as this change needs to get in for VS Code 1.4 (code freeze/testing starts on the 25th).

@parisk
Copy link
Contributor

parisk commented Jul 22, 2016

🆗 let's move forward with merging this right now and I will create an issue in order to fix copy and paste with mouse until the next release (1.1.0) at 22 August 2016.

@parisk parisk merged commit 10fbf0d into xtermjs:master Jul 22, 2016
@Tyriar
Copy link
Member Author

Tyriar commented Jul 22, 2016

4595a18 actually broke the IMEs as it changed focus/blur behavior on the textarea, investigating...

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.

3 participants