Bubble scroll events when scrolling beyond the bounds of the viewport #1449

ghost · 2018-05-14T03:20:04Z

The changes in this PR should allow the page to scroll when the terminal reaches it's top or bottom. If the terminal is scrolled then the events are canceled otherwise they bubble up.

ghost · 2018-05-14T03:30:21Z

src/Terminal.ts

-      this.viewport.onTouchMove(ev);
-      return this.cancel(ev);
+      if (<any>this.viewport.onTouchMove(ev) === false) {
+        return this.cancel(ev);


tsc wouldn't allow me to do a strict equality check without upcasting: cannot void === boolean. I'm not sure that this is the best way to handle it.

You would probably just want to changing onTouchMove/onWheel to always return a boolean

ghost · 2018-05-14T03:36:58Z

src/Viewport.ts

+   * Calls onScroll synchronously and determines whether the terminal was
+   * actually scrolled.
+   */
+  private scrollSync(amount: number): boolean {


This seems to be the best way to avoid code duplication as we can't really prevent the scroll event because changing _viewportElement.scrollTop is necessary? I don't really like the method name either, feedback please 😕

ghost · 2018-05-14T03:43:09Z

src/Viewport.ts

-    ev.preventDefault();
+    if (this.scrollSync(amount)) {
+      // Prevent the page from scrolling when the terminal scrolls
+      ev.preventDefault();


Is the ev.preventDefault() statement meant for third party consumers? The terminal.cancel method was always being called after these handlers.

Yeah feel free to remove this as it's handled in Terminal.cancel.

Tyriar · 2018-05-14T13:38:53Z

src/Terminal.ts

-      this.viewport.onTouchMove(ev);
-      return this.cancel(ev);
+      if (<any>this.viewport.onTouchMove(ev) === false) {
+        return this.cancel(ev);


You would probably just want to changing onTouchMove/onWheel to always return a boolean

Tyriar · 2018-05-14T13:38:56Z

src/Viewport.ts

@@ -99,7 +102,12 @@ export class Viewport implements IViewport {
   * terminal to scroll to it.
   * @param ev The scroll event.
   */
-  private _onScroll(ev: Event): void {
+  private _onScroll(ev?: Event): void {
+    // Prevent duplicate event handling


Oh the double call is prevented here, this sort of state between async callbacks should be avoided at all costs imo. What's the duplicate event handling we're concerned about?

OnTouchMove/onWheel both modify _viewportElement.scrollTop which causes the scroll event to fire on _viewportElement during the next event loop. If we call onScroll manually on the current event loop to determine whether the viewport was actually scrolled then the scroll event will still fire thus onScroll will get called a second time. We could remove this check if we don't modify _viewportElement.scrollTop but it's necessary?

nvm that, I missed what you were saying and now it doesn't seem necessary to prevent the handling.

Tyriar · 2018-05-14T13:42:33Z

src/Viewport.ts

@@ -179,13 +202,16 @@ export class Viewport implements IViewport {
   * Handles the touchmove event, scrolling the viewport if the position shifted.
   * @param ev The touch event.
   */
-  public onTouchMove(ev: TouchEvent): void {
-    let deltaY = this._lastTouchY - ev.touches[0].pageY;
+  public onTouchMove(ev: TouchEvent): boolean | void {


Can't we just return a boolean on this and then leave event cancellation up to this part?

xterm.js/src/Terminal.ts

Lines 1091 to 1109 in 7a9f291

// allow wheel scrolling in

// the shell for example

on(el, 'wheel', (ev: WheelEvent) => {

if (this.mouseEvents) return;

this.viewport.onWheel(ev);

return this.cancel(ev);

});

on(el, 'touchstart', (ev: TouchEvent) => {

if (this.mouseEvents) return;

this.viewport.onTouchStart(ev);

return this.cancel(ev);

});

on(el, 'touchmove', (ev: TouchEvent) => {

if (this.mouseEvents) return;

this.viewport.onTouchMove(ev);

return this.cancel(ev);

});

Tyriar · 2018-05-14T17:11:15Z

src/Viewport.ts

+  private scrollSync(amount: number): boolean {
+    const oldYDisp = this._terminal.buffer.ydisp;
+    this._viewportElement.scrollTop += amount;
+    this._onScroll();


I don't think this._onScroll() shouldn't be needed in this function as it will fire when this._viewportElement.scrollTop changes?

The event handler delegation is complicating things.

No, It wouldn't be needed if we could determine whether or not to cancel the events in onTouchMove/onWheel without it and without code duplication. The only other way that I can tell is to copy code from the _onScroll and _terminal.scrollLines functions to scrollSync but onScroll would still be called anyway.

The problem is that you can't check to see if buffer.ydisp has changed until after onScroll executes but that won't happen before returning from the original event handlers without calling it manually.

And maybe I'm over complicating things 😜

I haven't exactly tested to see if it this._onScroll is called immediately (during the current event loop) upon changing _viewportElement.scrollTop. However, I wouldn't think so... lol

I'll check the spec and get back to you on that.

I didn't find exactly what I was looking for in the specs however if we're to believe this mdn article about the event loop then when the event is triggered by modifying _viewportElement.scrollTop a new message is added to the message queue but won't be processed until the current message is processed to it's completion which is the end of the current event loop. See "Run-to-completion" in the linked article.

Can we bounds check this._viewportElement.scrollTop? If it's 0 we don't need to scroll, I think there's a way to check if it's at the end as well.

The bounds checking should be performed on the buffer and not _viewportElement.scrollTop? https://github.com/xtermjs/xterm.js/blob/master/src/Terminal.ts#L1213

We would be duplicating terminal.scrollLines?

@pro-src that would probably be better, as long as no sync issues occur.

ghost · 2018-05-14T23:27:15Z

@Tyriar updated.

ghost · 2018-05-14T23:30:54Z

src/Terminal.ts

-      return this.cancel(ev);
+      if (this.viewport.onWheel(ev)) {
+        return this.cancel(ev);
+      }
    });

    on(el, 'touchstart', (ev: TouchEvent) => {


The touchstart event is still always canceled.

Tyriar · 2018-05-15T14:49:46Z

I just tested the PR on Ubuntu 18.04/Chrome and all the scroll events are bubbling up, even when the terminal is being scrolled

ghost · 2018-05-15T15:52:09Z

Thanks for testing. I'll pull in changes from master and push an update when it's working. I'm writing tests for this as well.

ghost · 2018-05-18T02:56:43Z

I'm kinda stuck... Any ideas as to what's causing the issue with the PR? The tests pass but it still doesn't work.

Tyriar · 2018-05-18T18:47:42Z

@pro-src I don't think there's much testing in this area. Can you repro the issue I was seeing? Could be your browser if not?

ghost · 2018-05-24T03:17:43Z

@pro-src I don't think there's much testing in this area. Can you repro the issue I was seeing? Could be your browser if not?

@Tyriar, I am able to reproduce that issue but the only in-browser testing I've done has been on unsupported mobile browsers. The node tests that I wrote are passing though. I don't know that I'll revisit this anytime soon.

Tyriar · 2018-09-08T20:35:55Z

Closing for housekeeping purposes, the PR is archived if we need the diff in the future.

ghost commented May 14, 2018

View reviewed changes

Tyriar requested changes May 14, 2018

View reviewed changes

Tyriar changed the title ~~Improve scrolling experience~~ Bubble scroll events when scrolling beyond the bounds of the viewport May 14, 2018

Tyriar self-assigned this May 14, 2018

Tyriar reviewed May 14, 2018

View reviewed changes

ghost commented May 14, 2018

View reviewed changes

xtermjs deleted a comment from coveralls May 15, 2018

Bubble scroll events when scrolling beyond the viewport

2fe5d49

Tyriar added the work-in-progress Do not merge label May 21, 2018

Tyriar closed this Sep 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bubble scroll events when scrolling beyond the bounds of the viewport #1449

Bubble scroll events when scrolling beyond the bounds of the viewport #1449

ghost commented May 14, 2018 •

edited by ghost

Loading

ghost May 14, 2018

Tyriar May 14, 2018

ghost May 14, 2018

ghost May 14, 2018

Tyriar May 14, 2018

Tyriar May 14, 2018

Tyriar May 14, 2018

ghost May 14, 2018

ghost May 14, 2018

Tyriar May 14, 2018

Tyriar May 14, 2018

ghost May 14, 2018 •

edited by ghost

Loading

ghost May 14, 2018

ghost May 14, 2018

Tyriar May 14, 2018

ghost May 14, 2018

Tyriar May 15, 2018

ghost commented May 14, 2018

ghost May 14, 2018

Tyriar commented May 15, 2018

ghost commented May 15, 2018

ghost commented May 18, 2018

Tyriar commented May 18, 2018

ghost commented May 24, 2018

Tyriar commented Sep 8, 2018

	// allow wheel scrolling in
	// the shell for example
	on(el, 'wheel', (ev: WheelEvent) => {
	if (this.mouseEvents) return;
	this.viewport.onWheel(ev);
	return this.cancel(ev);
	});

	on(el, 'touchstart', (ev: TouchEvent) => {
	if (this.mouseEvents) return;
	this.viewport.onTouchStart(ev);
	return this.cancel(ev);
	});

	on(el, 'touchmove', (ev: TouchEvent) => {
	if (this.mouseEvents) return;
	this.viewport.onTouchMove(ev);
	return this.cancel(ev);
	});

Bubble scroll events when scrolling beyond the bounds of the viewport #1449

Bubble scroll events when scrolling beyond the bounds of the viewport #1449

Conversation

ghost commented May 14, 2018 • edited by ghost 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

Choose a reason for hiding this comment

ghost May 14, 2018 • edited by ghost 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

ghost commented May 14, 2018

Choose a reason for hiding this comment

Tyriar commented May 15, 2018

ghost commented May 15, 2018

ghost commented May 18, 2018

Tyriar commented May 18, 2018

ghost commented May 24, 2018

Tyriar commented Sep 8, 2018

ghost commented May 14, 2018 •

edited by ghost

Loading

ghost May 14, 2018 •

edited by ghost

Loading