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

Drop hidden textarea for input handling #78

Merged
merged 14 commits into from
Jun 7, 2016
Merged

Conversation

parisk
Copy link
Contributor

@parisk parisk commented Jun 2, 2016

This Pull Request drops completely the hidden textarea for input handling. It relies completely on keyboard event handling on the element attribute of the terminal. This simplifies the structure of the code and removes a few weird bugs of the previous implementation.

The only hack that had to be done here was to allow pasting with right click. Using contentEditable was not a choice as we would not be able to get rid of the default cursor applied on the terminal by the browser. To handle right-click paste I set the contentEditable attribute of the terminal to true only on right click and then unset it on mouseup this way the default cursor is only seen for an instance, while pasting on the terminal with right click is made possible.

This PR deals with the following issues:

@TDaglis
Copy link
Contributor

TDaglis commented Jun 2, 2016

I encountered various issues with this PR. I'll try to pinpoint their cause and document them here tomorrow.

@parisk
Copy link
Contributor Author

parisk commented Jun 3, 2016

Just pushed an update that fixes several pasting issues across different browsers. @TDaglis could you please take a look at Windows?

@TDaglis
Copy link
Contributor

TDaglis commented Jun 3, 2016

Sure. I'm on it

@TDaglis
Copy link
Contributor

TDaglis commented Jun 3, 2016

Issues I faced after the update:

  • the blinking caret is visible for a while after you click the terminal
  • the focus event is not triggered anymore (it should be moved from the textarea to element instead of being removed completely)
  • cut works 😅

parisk added 2 commits June 5, 2016 06:35
- Do not actually cut on cut
- Strip trailing whitespaces on copy (fix #66)
@parisk parisk force-pushed the drop-hidden-textarea branch from 0640743 to 8754f8d Compare June 5, 2016 04:01
@parisk
Copy link
Contributor Author

parisk commented Jun 5, 2016

Just pushed a commit that should fix all cut and copy issues (it also trims white spaces when copying; fixes #66) and emits the focus and blur events again 😊 .

The blinking caret is a result of leasing contentEditable for 5 seconds when the terminals that the user is about to paste. Maybe we could move the caret somewhere relatively invisible, but not sure if it is worth the effort at this time.

@akalipetis
Copy link
Contributor

LGTM 🐻 , apart from some strange Edge copy/paste issues.

Sometimes I'm able to copy on Edge, but I cannot paste at any time. I don't know if this is something we can take care of, just noticed it.

Code looks 👌 apart from minor comments.

@@ -30,3 +32,7 @@ term.on('key', function (key, ev) {
term.write(key);
}
});

term.on('paste', function (data, ev) {
term.write(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the indentation is off here.

@parisk parisk merged commit 180a7af into master Jun 7, 2016
@Tyriar
Copy link
Member

Tyriar commented Jun 11, 2016

See #124, no native text control means this makes the operating system's IME not activate. As far as I can tell this cannot be worked around as there are no hooks into the IME from JavaScript and the KeyboardEvent received contains the regular character.

@Tyriar Tyriar mentioned this pull request Jul 12, 2016
25 tasks
@Tyriar Tyriar deleted the drop-hidden-textarea branch October 31, 2016 19:47
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.

4 participants