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 scroll functionality for mobile #747

Merged
merged 2 commits into from
Jul 4, 2017
Merged

Add scroll functionality for mobile #747

merged 2 commits into from
Jul 4, 2017

Conversation

anishathalye
Copy link
Contributor

@anishathalye anishathalye commented Jul 1, 2017

This patch adds functionality to be able to scroll on mobile (tested with an iOS device).

Part of #594

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 69.473% when pulling a1fa2c2 on anishathalye:master into 0b3d1e5 on sourcelair:master.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

This is a good start for touch support! Let's move forward with this after the changes are fixed up.

Eventually a nicer solution will be better so that ballistic scroll is properly supported. I just tried it out and it seems to work really well in my branch #748. I'm a little concerned about browser support with the event redirection though so that needs a bunch of verification.

src/Viewport.ts Outdated
@@ -34,6 +34,7 @@ export class Viewport {
this.terminal.on('scroll', this.syncScrollArea.bind(this));
this.terminal.on('resize', this.syncScrollArea.bind(this));
this.viewportElement.addEventListener('scroll', this.onScroll.bind(this));
this.touchY = null;
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to declare this as a private member above to remove the compile warning. A better name might also be lastTouchY:

private lastTouchY: number;

src/xterm.js Outdated
if (self.mouseEvents) return;
self.viewport.onTouchStart(ev);
return self.cancel(ev);
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing a ); at the end

src/xterm.js Outdated
if (self.mouseEvents) return;
self.viewport.onTouchMove(ev);
return self.cancel(ev);
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing a ); at the end

@Tyriar Tyriar modified the milestones: 2.8.0, 2.9.0 Jul 2, 2017
@anishathalye
Copy link
Contributor Author

Thanks for the feedback! I rebased onto the current master branch and also made the changes you asked for.

(for some reason, GitHub doesn't seem to recognize my commit --amend as a change...)

Let me know if there's anything else I should change.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 69.412% when pulling 6223342 on anishathalye:master into 9ddabb3 on sourcelair:master.

@Tyriar
Copy link
Member

Tyriar commented Jul 4, 2017

@anishathalye yeah with GitHub you typically add individual "fix" commits after a PR is made as amending a commit that has already been commented on can turn out a bit weird in GH. The repo owners can squash the commits if they want when it's merged.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 69.412% when pulling 3e8ddd1 on anishathalye:master into 9ddabb3 on sourcelair:master.

@Tyriar Tyriar modified the milestones: 2.8.0, 2.9.0 Jul 4, 2017
@Tyriar Tyriar merged commit 67bc045 into xtermjs:master Jul 4, 2017
@Tyriar Tyriar self-assigned this Jul 4, 2017
@tbodt
Copy link

tbodt commented Nov 9, 2017

The problem with this is it doesn't support the iOS scroll momentum thing. I've gotten that to work by removing these listeners, using z-index to put the viewport div above the rows div, and putting -webkit-overflow-scrolling: touch on the viewport div. Unfortunately, putting the viewport div above the rows div breaks selection.

@tbodt
Copy link

tbodt commented Nov 9, 2017

Actually, it turns out that setting the z-index is unnecessary, and not setting the z-index makes selection work. So this is the perfect solution. (For iOS, at least.)

@Tyriar
Copy link
Member

Tyriar commented Dec 11, 2017

@tbodt #594 was kept open since ballistic scroll doesn't work yet.

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.

None yet

4 participants