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

Stop auto scrolling to bottom on output #336

Merged
merged 2 commits into from
Nov 4, 2016

Conversation

parisk
Copy link
Contributor

@parisk parisk commented Nov 3, 2016

This PR is a continuation of #326 by @jupe. I kept all original commits, to maintain the appropriate credits 😊 .

This PR introduces this new behavior:

  • When the user has scrolled up the terminal and output is being printed (by the back-end), the terminal now is not scrolling down to bottom, as it used to
  • The terminal scrolls down to bottom when the user presses a key

Closes #216.

/cc @Tyriar to take a look since you opened the initial issue and commented also in the original PR.

@parisk parisk added this to the 2.1.0 milestone Nov 3, 2016
@@ -2413,6 +2420,13 @@ Terminal.prototype.attachCustomKeydownHandler = function(customKeydownHandler) {
* @param {KeyboardEvent} ev The keydown event to be handled.
*/
Terminal.prototype.keyDown = function(ev) {
// Scroll down to prompt, whenever the user presses a key.
if (this.ybase !== this.ydisp) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this.userScrolling be set here?

Also I think having this at the start of keyDown means that the DOM could be updated twice in a row, once before the key is evaluated and once after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this.userScrolling be set here?

I am not sure if it should. Strictly speaking, the user is not scrolling here. We are scrolling to bottom for them, in order to be able to see their input.

Also I think having this at the start of keyDown means that the DOM could be updated twice in a row, once before the key is evaluated and once after.

That's true. I put it there because I thought that we should scroll down no matter how the key will be handled (sounds like a universal terminal behavior).

I think it's not bad that the DOM might get updated twice, since this will happen only if the terminal needs to scroll down, meaning that in the next typing there will be no scrolling of the viewport, thus no second (or first 😄 ) DOM update.

@Tyriar
Copy link
Member

Tyriar commented Nov 3, 2016

Would it be easy to write some unit tests for this? For example:

  • Key down always scrolls to bottom of buffer
  • Write scrolls down to bottom of buffer when it's already at the bottom
  • Write does not scroll down to bottom of buffer when not at the bottom

@parisk parisk self-assigned this Nov 4, 2016
@parisk parisk force-pushed the issue-#216-no-auto-scroll-on-output branch 2 times, most recently from 7345418 to d588ead Compare November 4, 2016 12:21
- Scroll to bottom on keydown (when scrolled up)
- Add test to ensure above behavior
@parisk parisk force-pushed the issue-#216-no-auto-scroll-on-output branch from d588ead to 0de3d83 Compare November 4, 2016 12:30
@parisk
Copy link
Contributor Author

parisk commented Nov 4, 2016

🆗 I moved on with the following:

  1. Used scrollToBottom instead of manipulating ydisp and ybase myself and emitting scroll
  2. Implemented test to make sure that terminal scrolls to bottom, when scrolled up and key is pressed
  3. Cleaned up and squashed commits in order to avoid touching the dist directory at all and keeping the initial commit of @jupe.

@Tyriar I don't think I could easily implement tests for write as it interacts with the DOM heavily.

Let me know if this is good to go.

var terminal = new Terminal();

// Do not process the keyDown event, to avoid side-effects
terminal.attachCustomKeydownHandler(function () {
Copy link
Member

Choose a reason for hiding this comment

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

Clever trick 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@parisk parisk merged commit 55ab366 into master Nov 4, 2016
@Tyriar Tyriar deleted the issue-#216-no-auto-scroll-on-output branch November 4, 2016 20:15
@parisk
Copy link
Contributor Author

parisk commented Nov 11, 2016

@onttoni just letting you know that we mentioned you for your commit in this PR in the 2.1 announcement. Thank you!

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.

2 participants